New issue
Advanced search Search tips

Issue 685969 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Reading List: Opening an offline item in a new tab DCHECKS on closing.

Project Member Reported by olivierrobin@chromium.org, Jan 27 2017

Issue description

When instantiating a OfflinePageNativeContent in the BVC, currentWebState is wrong if the offline page is opened in a new tab.

Sets the webState in web_controller instead.
 
Components: UI>Browser>ReaderMode
Labels: ReleaseBlock-Stable M-57 Hotlist-ReadingList
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a

commit d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a
Author: olivierrobin <olivierrobin@chromium.org>
Date: Fri Jan 27 20:35:26 2017

Pass the correct webState to the nativeContent.

If an offline content is created in a new tab, currentWebState in the
BVC will return the webState from the original tab (created tab is in
background).

Pass the webState from the webController instead.

BUG= 685969 

Review-Url: https://codereview.chromium.org/2655463014
Cr-Commit-Position: refs/heads/master@{#446753}

[modify] https://crrev.com/d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a/ios/chrome/browser/ui/contextual_search/contextual_search_results_view.mm
[modify] https://crrev.com/d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a/ios/chrome/browser/ui/preload_controller.mm
[modify] https://crrev.com/d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a/ios/web/public/test/fakes/test_native_content_provider.mm
[modify] https://crrev.com/d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a/ios/web/public/web_state/ui/crw_native_content_provider.h
[modify] https://crrev.com/d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a/ios/web/web_state/ui/crw_web_controller.mm

Labels: Merge-Request-57
Status: Fixed (was: Started)
Project Member

Comment 4 by sheriffbot@chromium.org, Jan 27 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 5 by sheriffbot@chromium.org, Jan 31 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
@olivierrobin, What would be a good way to test this?



1 Visit a wikipedia page A
2 Add to reading list a page B that is linked from A. Fait for it to be distilled.
3 Go back to A.
Switch to Air plane mode

Check that long pressing on link to B and open in new tab/Open in incognito correctly opens offline version in new tab.
Check that there is no problem while closing the tab or navigating from offline version.

Status: Verified (was: Fixed)
Verified in 58.0.2998.0 canary, iPhone 6+ iOS 10.2, iPad mini 10.1
Followed steps in comment #7
Looks good.
Labels: -Merge-Approved-57 merge-merged-57
https://codereview.chromium.org/2660333006/
Labels: -merge-merged-57 merge-merged-2987
Verified in 57.0.2987.96 dev, iPhone 6 plus  iOS 10.2, iPad mini 10.3
Looks good.

Sign in to add a comment