Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2762113002:
Reading List iOS: Adapt loading offline for new reload process (Closed)
|
Created:
3 years, 9 months ago by Olivier Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, baxley+watch_chromium.org, pkl (ping after 24h if needed), ios-reviews+web_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, stkhapugin, huangml+watch_chromium.org, liaoyuke+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReading List iOS: Adapt loading offline for new reload process
Loading offline pages relies on being able to reload page after
changing the URL.
Adapt to the new reload process which does not allow that.
Also add EG tests for catching future problem.
BUG=703393, 703206
Review-Url: https://codereview.chromium.org/2762113002
Cr-Commit-Position: refs/heads/master@{#460354}
Committed: https://chromium.googlesource.com/chromium/src/+/f266b4e8d109a11ac0e3fae0b4c8500281e7c457
Patch Set 1 #Patch Set 2 : clean #Patch Set 3 : fix DCHECK #
Total comments: 1
Patch Set 4 : factor code #
Total comments: 5
Patch Set 5 : update comment #
Total comments: 12
Patch Set 6 : feedback #
Total comments: 7
Patch Set 7 : feedback #Patch Set 8 : fix tests #Patch Set 9 : rebase #
Messages
Total messages: 57 (27 generated)
Description was changed from ========== Fix loading offline page BUG= ========== to ========== Reading List iOS: Adapt loading offline for new reload process Loading offline pages relies on being able to reload page after changing the URL. Adapt to the new reload process which does not allow that. Also add EG tests for catching future problem. BUG=703393, 703206 ==========
olivierrobin@chromium.org changed reviewers: + jif@chromium.org
PTAL
https://codereview.chromium.org/2762113002/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right): https://codereview.chromium.org/2762113002/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:366: // Setup a server serving a distillable page at http://potato with the title Move the code creating the server and the pages to a helper function, and re-use that function in |testSavingToReadingList|. Doing the same for all the shared code would be great. Example for lines 403-418: VerifyPage(PAGE_OFFLINE, "localhost:8080/potato/");
Thanks PTAL
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/2762113002/diff/60001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (left): https://codereview.chromium.org/2762113002/diff/60001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:298: navigationManager->Reload(web::ReloadType::NORMAL, The fact that Reload does not work anymore looks like a bug. Yuke's CL was a refactoring and behavior change was not intentional and should be treated as a bug. https://codereview.chromium.org/2762113002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2762113002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1804: // This method is allowed to handle reload only for transient items, which Could you please merge your comment into existing comment: "This method is allowed to handle reload only for transient items and native content, which is essentially loading the same URL again."
olivierrobin@chromium.org changed reviewers: + liaoyuke@chromium.org
Thanks https://codereview.chromium.org/2762113002/diff/60001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (left): https://codereview.chromium.org/2762113002/diff/60001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:298: navigationManager->Reload(web::ReloadType::NORMAL, On 2017/03/21 15:48:19, Eugene But wrote: > The fact that Reload does not work anymore looks like a bug. Yuke's CL was a > refactoring and behavior change was not intentional and should be treated as a > bug. The problem here is that navigationManager->Reload calls CRWWebController reload. At that point, there is an error page, so CRWWebController calls ErrorPageContent reload. The item defined here is lost. https://codereview.chromium.org/2762113002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2762113002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1804: // This method is allowed to handle reload only for transient items, which On 2017/03/21 15:48:19, Eugene But wrote: > Could you please merge your comment into existing comment: > > "This method is allowed to handle reload only for transient items and native > content, which is essentially loading the same URL again." Done.
lgtm https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right): https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:219: // Adds an the current page to the Reading List. s/an // https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:304: [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewContainingText( line 303-312 sort of duplicate the lines 281-293 https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:319: IDR_IOS_OMNIBOX_OFFLINE), test that the image is IDR_IOS_OMNIBOX_HTTP when online?
Let's see if Yuke can make a fix in web layer. If they decide that current reload behavior is WAI, then this is lgtm https://codereview.chromium.org/2762113002/diff/60001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (left): https://codereview.chromium.org/2762113002/diff/60001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:298: navigationManager->Reload(web::ReloadType::NORMAL, On 2017/03/21 16:41:31, Olivier Robin wrote: > On 2017/03/21 15:48:19, Eugene But wrote: > > The fact that Reload does not work anymore looks like a bug. Yuke's CL was a > > refactoring and behavior change was not intentional and should be treated as a > > bug. > > The problem here is that > navigationManager->Reload calls CRWWebController reload. > At that point, there is an error page, so CRWWebController calls > ErrorPageContent reload. > The item defined here is lost. Thank you for explanation. I asked Kurt if this is WAI on the bug. https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:298: navigationManager->GoToIndex( Do you want to add comments explaining why |GoToIndex| instead of |Reload|? https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right): https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:259: std::string pageTitle = "Tomato"; s/pageTitle/page_title Same comment for other variables inside C++ methods https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:277: // Test that the correct version of kDistillableURL is displayed. s/Test/Tests
Thanks. I will wait until tomorrow to see if there is a web/ fix. Even if there is one, I will land the egtest part. https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:298: navigationManager->GoToIndex( On 2017/03/21 17:10:24, Eugene But wrote: > Do you want to add comments explaining why |GoToIndex| instead of |Reload|? Done. https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right): https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:219: // Adds an the current page to the Reading List. On 2017/03/21 17:09:50, jif wrote: > s/an // Done. https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:259: std::string pageTitle = "Tomato"; On 2017/03/21 17:10:24, Eugene But wrote: > s/pageTitle/page_title > > Same comment for other variables inside C++ methods Done. https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:277: // Test that the correct version of kDistillableURL is displayed. On 2017/03/21 17:10:24, Eugene But wrote: > s/Test/Tests Done. https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:304: [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewContainingText( On 2017/03/21 17:09:50, jif wrote: > line 303-312 sort of duplicate the lines 281-293 For the one that is present, yes, for the other one, no. An I liked to have a absent/present logic instead of having a scenario dependant matcher. https://codereview.chromium.org/2762113002/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:319: IDR_IOS_OMNIBOX_OFFLINE), On 2017/03/21 17:09:50, jif wrote: > test that the image is IDR_IOS_OMNIBOX_HTTP when online? The image is not displayed.
kkhorimoto@chromium.org changed reviewers: + kkhorimoto@chromium.org
not lgtm https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); This is not correct. Using LoadURLWithParams() will erase the forward items in the NavigationManager. I think that after my CL, the original Reload() call will work correctly, since the restored online URL will go through the web view codepath rather than attempting to reload the OfflinePageNativeContent.
https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); On 2017/03/21 20:52:39, kkhorimoto_ wrote: > This is not correct. Using LoadURLWithParams() will erase the forward items in > the NavigationManager. I think that after my CL, the original Reload() call > will work correctly, since the restored online URL will go through the web view > codepath rather than attempting to reload the OfflinePageNativeContent. FYI, this will not erase the forward items if |_virtualURL| == |_entryURL| because in |-restoreOnlineURL| the URL of the lastCommittedItem has been changed to |_entryURL|, then |LoadURLWithParams| kicks off a new load to add a new pending item whose URL is |_virtualURL|, and since the to be added pending item has the same URL as last committed item, the pending item will not be created. Without pending item, |CommitPendingItem| or |ClearForwardItems| will not be called. But anyway, Kurt is right about that Reload() should be sufficient here, and in general, I think we should avoid calling LoadURLWIthParams with ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if one needs to reload, one should call |Reload| explicitly. Thank you for fixing this. lgtm once Kurt's comment is addressed :)
https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); On 2017/03/21 22:49:09, liaoyuke wrote: > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > This is not correct. Using LoadURLWithParams() will erase the forward items > in > > the NavigationManager. I think that after my CL, the original Reload() call > > will work correctly, since the restored online URL will go through the web > view > > codepath rather than attempting to reload the OfflinePageNativeContent. > > FYI, this will not erase the forward items if |_virtualURL| == |_entryURL| > because in |-restoreOnlineURL| the URL of the lastCommittedItem has been changed > to |_entryURL|, then |LoadURLWithParams| kicks off a new load to add a new > pending item whose URL is |_virtualURL|, and since the to be added pending item > has the same URL as last committed item, the pending item will not be created. > Without pending item, |CommitPendingItem| or |ClearForwardItems| will not be > called. > > But anyway, Kurt is right about that Reload() should be sufficient here, and in > general, I think we should avoid calling LoadURLWIthParams with > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if one needs to > reload, one should call |Reload| explicitly. > > Thank you for fixing this. lgtm once Kurt's comment is addressed :) I don't think that |_virtualURL| will ever be equal to |_entryURL|, since the virtual URL is a web URL and the entry URL is a chrome-scheme native controller URL used to load offline pages. But in any case, I think that using the fixed Reload() will be the best approach here. https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: item->SetVirtualURL(_entryURL); You might want to consider SetVirtualURL(GURL::EmptyGURL()) since NavigationItem::GetVirtualURL() will return the URL if the virtual URL is empty. This is what we should be using for the "default" state, as setting the virtual URL may introduce bugs where this NavigationItem's URL is updated, but the virtual URL is still |_entryURL|. That may accidentally introduce a URL spoofing bug where the URL that's loaded is to a different page than the URL that is displayed in the omnibox.
https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); On 2017/03/21 22:56:29, kkhorimoto_ wrote: > On 2017/03/21 22:49:09, liaoyuke wrote: > > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > > This is not correct. Using LoadURLWithParams() will erase the forward items > > in > > > the NavigationManager. I think that after my CL, the original Reload() call > > > will work correctly, since the restored online URL will go through the web > > view > > > codepath rather than attempting to reload the OfflinePageNativeContent. > > > > FYI, this will not erase the forward items if |_virtualURL| == |_entryURL| > > because in |-restoreOnlineURL| the URL of the lastCommittedItem has been > changed > > to |_entryURL|, then |LoadURLWithParams| kicks off a new load to add a new > > pending item whose URL is |_virtualURL|, and since the to be added pending > item > > has the same URL as last committed item, the pending item will not be created. > > Without pending item, |CommitPendingItem| or |ClearForwardItems| will not be > > called. > > > > But anyway, Kurt is right about that Reload() should be sufficient here, and > in > > general, I think we should avoid calling LoadURLWIthParams with > > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if one needs to > > reload, one should call |Reload| explicitly. > > > > Thank you for fixing this. lgtm once Kurt's comment is addressed :) > > I don't think that |_virtualURL| will ever be equal to |_entryURL|, since the > virtual URL is a web URL and the entry URL is a chrome-scheme native controller > URL used to load offline pages. But in any case, I think that using the fixed > Reload() will be the best approach here. My mistake, |_entryURL| is not chrome:// scheme. We should still use NavigationManager::Reload() though :)
The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); On 2017/03/21 23:02:16, kkhorimoto_ wrote: > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > On 2017/03/21 22:49:09, liaoyuke wrote: > > > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > > > This is not correct. Using LoadURLWithParams() will erase the forward > items > > > in > > > > the NavigationManager. I think that after my CL, the original Reload() > call > > > > will work correctly, since the restored online URL will go through the web > > > view > > > > codepath rather than attempting to reload the OfflinePageNativeContent. > > > > > > FYI, this will not erase the forward items if |_virtualURL| == |_entryURL| > > > because in |-restoreOnlineURL| the URL of the lastCommittedItem has been > > changed > > > to |_entryURL|, then |LoadURLWithParams| kicks off a new load to add a new > > > pending item whose URL is |_virtualURL|, and since the to be added pending > > item > > > has the same URL as last committed item, the pending item will not be > created. > > > Without pending item, |CommitPendingItem| or |ClearForwardItems| will not be > > > called. > > > > > > But anyway, Kurt is right about that Reload() should be sufficient here, and > > in > > > general, I think we should avoid calling LoadURLWIthParams with > > > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if one needs to > > > reload, one should call |Reload| explicitly. > > > > > > Thank you for fixing this. lgtm once Kurt's comment is addressed :) > > > > I don't think that |_virtualURL| will ever be equal to |_entryURL|, since the > > virtual URL is a web URL and the entry URL is a chrome-scheme native > controller > > URL used to load offline pages. But in any case, I think that using the fixed > > Reload() will be the best approach here. > > My mistake, |_entryURL| is not chrome:// scheme. We should still use > NavigationManager::Reload() though :) Done. https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: item->SetVirtualURL(_entryURL); On 2017/03/21 22:56:29, kkhorimoto_ wrote: > You might want to consider SetVirtualURL(GURL::EmptyGURL()) since > NavigationItem::GetVirtualURL() will return the URL if the virtual URL is empty. > This is what we should be using for the "default" state, as setting the virtual > URL may introduce bugs where this NavigationItem's URL is updated, but the > virtual URL is still |_entryURL|. That may accidentally introduce a URL > spoofing bug where the URL that's loaded is to a different page than the URL > that is displayed in the omnibox. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
btw, could you please format the description of the CL so that it meets the 73 characters length requirement? Thanks! On Wed, Mar 22, 2017 at 6:33 AM <olivierrobin@chromium.org> wrote: > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > (right): > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: > _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); > On 2017/03/21 23:02:16, kkhorimoto_ wrote: > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > On 2017/03/21 22:49:09, liaoyuke wrote: > > > > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > > > > This is not correct. Using LoadURLWithParams() will erase the > forward > > items > > > > in > > > > > the NavigationManager. I think that after my CL, the original > Reload() > > call > > > > > will work correctly, since the restored online URL will go > through the web > > > > view > > > > > codepath rather than attempting to reload the > OfflinePageNativeContent. > > > > > > > > FYI, this will not erase the forward items if |_virtualURL| == > |_entryURL| > > > > because in |-restoreOnlineURL| the URL of the lastCommittedItem > has been > > > changed > > > > to |_entryURL|, then |LoadURLWithParams| kicks off a new load to > add a new > > > > pending item whose URL is |_virtualURL|, and since the to be added > pending > > > item > > > > has the same URL as last committed item, the pending item will not > be > > created. > > > > Without pending item, |CommitPendingItem| or |ClearForwardItems| > will not be > > > > called. > > > > > > > > But anyway, Kurt is right about that Reload() should be sufficient > here, and > > > in > > > > general, I think we should avoid calling LoadURLWIthParams with > > > > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if one > needs to > > > > reload, one should call |Reload| explicitly. > > > > > > > > Thank you for fixing this. lgtm once Kurt's comment is addressed > :) > > > > > > I don't think that |_virtualURL| will ever be equal to |_entryURL|, > since the > > > virtual URL is a web URL and the entry URL is a chrome-scheme native > > controller > > > URL used to load offline pages. But in any case, I think that using > the fixed > > > Reload() will be the best approach here. > > > > My mistake, |_entryURL| is not chrome:// scheme. We should still use > > NavigationManager::Reload() though :) > > Done. > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: > item->SetVirtualURL(_entryURL); > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > You might want to consider SetVirtualURL(GURL::EmptyGURL()) since > > NavigationItem::GetVirtualURL() will return the URL if the virtual URL > is empty. > > This is what we should be using for the "default" state, as setting > the virtual > > URL may introduce bugs where this NavigationItem's URL is updated, but > the > > virtual URL is still |_entryURL|. That may accidentally introduce a > URL > > spoofing bug where the URL that's loaded is to a different page than > the URL > > that is displayed in the omnibox. > > Done. > > https://codereview.chromium.org/2762113002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Reading List iOS: Adapt loading offline for new reload process Loading offline pages relies on being able to reload page after changing the URL. Adapt to the new reload process which does not allow that. Also add EG tests for catching future problem. BUG=703393, 703206 ========== to ========== Reading List iOS: Adapt loading offline for new reload process Loading offline pages relies on being able to reload page after changing the URL. Adapt to the new reload process which does not allow that. Also add EG tests for catching future problem. BUG=703393, 703206 ==========
On 2017/03/22 16:16:33, liaoyuke wrote: > btw, could you please format the description of the CL so that it meets the > 73 characters length requirement? > > Thanks! > > On Wed, Mar 22, 2017 at 6:33 AM <mailto:olivierrobin@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > > (right): > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: > > _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); > > On 2017/03/21 23:02:16, kkhorimoto_ wrote: > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > On 2017/03/21 22:49:09, liaoyuke wrote: > > > > > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > > > > > This is not correct. Using LoadURLWithParams() will erase the > > forward > > > items > > > > > in > > > > > > the NavigationManager. I think that after my CL, the original > > Reload() > > > call > > > > > > will work correctly, since the restored online URL will go > > through the web > > > > > view > > > > > > codepath rather than attempting to reload the > > OfflinePageNativeContent. > > > > > > > > > > FYI, this will not erase the forward items if |_virtualURL| == > > |_entryURL| > > > > > because in |-restoreOnlineURL| the URL of the lastCommittedItem > > has been > > > > changed > > > > > to |_entryURL|, then |LoadURLWithParams| kicks off a new load to > > add a new > > > > > pending item whose URL is |_virtualURL|, and since the to be added > > pending > > > > item > > > > > has the same URL as last committed item, the pending item will not > > be > > > created. > > > > > Without pending item, |CommitPendingItem| or |ClearForwardItems| > > will not be > > > > > called. > > > > > > > > > > But anyway, Kurt is right about that Reload() should be sufficient > > here, and > > > > in > > > > > general, I think we should avoid calling LoadURLWIthParams with > > > > > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if one > > needs to > > > > > reload, one should call |Reload| explicitly. > > > > > > > > > > Thank you for fixing this. lgtm once Kurt's comment is addressed > > :) > > > > > > > > I don't think that |_virtualURL| will ever be equal to |_entryURL|, > > since the > > > > virtual URL is a web URL and the entry URL is a chrome-scheme native > > > controller > > > > URL used to load offline pages. But in any case, I think that using > > the fixed > > > > Reload() will be the best approach here. > > > > > > My mistake, |_entryURL| is not chrome:// scheme. We should still use > > > NavigationManager::Reload() though :) > > > > Done. > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: > > item->SetVirtualURL(_entryURL); > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > You might want to consider SetVirtualURL(GURL::EmptyGURL()) since > > > NavigationItem::GetVirtualURL() will return the URL if the virtual URL > > is empty. > > > This is what we should be using for the "default" state, as setting > > the virtual > > > URL may introduce bugs where this NavigationItem's URL is updated, but > > the > > > virtual URL is still |_entryURL|. That may accidentally introduce a > > URL > > > spoofing bug where the URL that's loaded is to a different page than > > the URL > > > that is displayed in the omnibox. > > > > Done. > > > > https://codereview.chromium.org/2762113002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Guideline (https://www.chromium.org/developers/contributing-code) is 80 characters. Done anyway.
On 2017/03/22 16:59:43, Olivier Robin wrote: > On 2017/03/22 16:16:33, liaoyuke wrote: > > btw, could you please format the description of the CL so that it meets the > > 73 characters length requirement? > > > > Thanks! > > > > On Wed, Mar 22, 2017 at 6:33 AM <mailto:olivierrobin@chromium.org> wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > > > (right): > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: > > > _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); > > > On 2017/03/21 23:02:16, kkhorimoto_ wrote: > > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > > On 2017/03/21 22:49:09, liaoyuke wrote: > > > > > > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > > > > > > This is not correct. Using LoadURLWithParams() will erase the > > > forward > > > > items > > > > > > in > > > > > > > the NavigationManager. I think that after my CL, the original > > > Reload() > > > > call > > > > > > > will work correctly, since the restored online URL will go > > > through the web > > > > > > view > > > > > > > codepath rather than attempting to reload the > > > OfflinePageNativeContent. > > > > > > > > > > > > FYI, this will not erase the forward items if |_virtualURL| == > > > |_entryURL| > > > > > > because in |-restoreOnlineURL| the URL of the lastCommittedItem > > > has been > > > > > changed > > > > > > to |_entryURL|, then |LoadURLWithParams| kicks off a new load to > > > add a new > > > > > > pending item whose URL is |_virtualURL|, and since the to be added > > > pending > > > > > item > > > > > > has the same URL as last committed item, the pending item will not > > > be > > > > created. > > > > > > Without pending item, |CommitPendingItem| or |ClearForwardItems| > > > will not be > > > > > > called. > > > > > > > > > > > > But anyway, Kurt is right about that Reload() should be sufficient > > > here, and > > > > > in > > > > > > general, I think we should avoid calling LoadURLWIthParams with > > > > > > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if one > > > needs to > > > > > > reload, one should call |Reload| explicitly. > > > > > > > > > > > > Thank you for fixing this. lgtm once Kurt's comment is addressed > > > :) > > > > > > > > > > I don't think that |_virtualURL| will ever be equal to |_entryURL|, > > > since the > > > > > virtual URL is a web URL and the entry URL is a chrome-scheme native > > > > controller > > > > > URL used to load offline pages. But in any case, I think that using > > > the fixed > > > > > Reload() will be the best approach here. > > > > > > > > My mistake, |_entryURL| is not chrome:// scheme. We should still use > > > > NavigationManager::Reload() though :) > > > > > > Done. > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: > > > item->SetVirtualURL(_entryURL); > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > You might want to consider SetVirtualURL(GURL::EmptyGURL()) since > > > > NavigationItem::GetVirtualURL() will return the URL if the virtual URL > > > is empty. > > > > This is what we should be using for the "default" state, as setting > > > the virtual > > > > URL may introduce bugs where this NavigationItem's URL is updated, but > > > the > > > > virtual URL is still |_entryURL|. That may accidentally introduce a > > > URL > > > > spoofing bug where the URL that's loaded is to a different page than > > > the URL > > > > that is displayed in the omnibox. > > > > > > Done. > > > > > > https://codereview.chromium.org/2762113002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Guideline (https://www.chromium.org/developers/contributing-code) is 80 > characters. > Done anyway. It's actually 72. You can see this guide line when using "git cl upload". The reason is that git log eats 8 characters so 80-8 == 72 :)
On 2017/03/22 17:35:18, Eugene But wrote: > On 2017/03/22 16:59:43, Olivier Robin wrote: > > On 2017/03/22 16:16:33, liaoyuke wrote: > > > btw, could you please format the description of the CL so that it meets the > > > 73 characters length requirement? > > > > > > Thanks! > > > > > > On Wed, Mar 22, 2017 at 6:33 AM <mailto:olivierrobin@chromium.org> wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > > > > (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: > > > > _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); > > > > On 2017/03/21 23:02:16, kkhorimoto_ wrote: > > > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > > > On 2017/03/21 22:49:09, liaoyuke wrote: > > > > > > > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > > > > > > > This is not correct. Using LoadURLWithParams() will erase the > > > > forward > > > > > items > > > > > > > in > > > > > > > > the NavigationManager. I think that after my CL, the original > > > > Reload() > > > > > call > > > > > > > > will work correctly, since the restored online URL will go > > > > through the web > > > > > > > view > > > > > > > > codepath rather than attempting to reload the > > > > OfflinePageNativeContent. > > > > > > > > > > > > > > FYI, this will not erase the forward items if |_virtualURL| == > > > > |_entryURL| > > > > > > > because in |-restoreOnlineURL| the URL of the lastCommittedItem > > > > has been > > > > > > changed > > > > > > > to |_entryURL|, then |LoadURLWithParams| kicks off a new load to > > > > add a new > > > > > > > pending item whose URL is |_virtualURL|, and since the to be added > > > > pending > > > > > > item > > > > > > > has the same URL as last committed item, the pending item will not > > > > be > > > > > created. > > > > > > > Without pending item, |CommitPendingItem| or |ClearForwardItems| > > > > will not be > > > > > > > called. > > > > > > > > > > > > > > But anyway, Kurt is right about that Reload() should be sufficient > > > > here, and > > > > > > in > > > > > > > general, I think we should avoid calling LoadURLWIthParams with > > > > > > > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if one > > > > needs to > > > > > > > reload, one should call |Reload| explicitly. > > > > > > > > > > > > > > Thank you for fixing this. lgtm once Kurt's comment is addressed > > > > :) > > > > > > > > > > > > I don't think that |_virtualURL| will ever be equal to |_entryURL|, > > > > since the > > > > > > virtual URL is a web URL and the entry URL is a chrome-scheme native > > > > > controller > > > > > > URL used to load offline pages. But in any case, I think that using > > > > the fixed > > > > > > Reload() will be the best approach here. > > > > > > > > > > My mistake, |_entryURL| is not chrome:// scheme. We should still use > > > > > NavigationManager::Reload() though :) > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: > > > > item->SetVirtualURL(_entryURL); > > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > > You might want to consider SetVirtualURL(GURL::EmptyGURL()) since > > > > > NavigationItem::GetVirtualURL() will return the URL if the virtual URL > > > > is empty. > > > > > This is what we should be using for the "default" state, as setting > > > > the virtual > > > > > URL may introduce bugs where this NavigationItem's URL is updated, but > > > > the > > > > > virtual URL is still |_entryURL|. That may accidentally introduce a > > > > URL > > > > > spoofing bug where the URL that's loaded is to a different page than > > > > the URL > > > > > that is displayed in the omnibox. > > > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2762113002/ > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > Guideline (https://www.chromium.org/developers/contributing-code) is 80 > > characters. > > Done anyway. > It's actually 72. You can see this guide line when using "git cl upload". The > reason is that git log eats 8 characters so 80-8 == 72 :) So should we not update https://www.chromium.org/developers/contributing-code ?
On 2017/03/22 17:36:15, Olivier Robin wrote: > On 2017/03/22 17:35:18, Eugene But wrote: > > On 2017/03/22 16:59:43, Olivier Robin wrote: > > > On 2017/03/22 16:16:33, liaoyuke wrote: > > > > btw, could you please format the description of the CL so that it meets > the > > > > 73 characters length requirement? > > > > > > > > Thanks! > > > > > > > > On Wed, Mar 22, 2017 at 6:33 AM <mailto:olivierrobin@chromium.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: > > > > > _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); > > > > > On 2017/03/21 23:02:16, kkhorimoto_ wrote: > > > > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > > > > On 2017/03/21 22:49:09, liaoyuke wrote: > > > > > > > > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > > > > > > > > This is not correct. Using LoadURLWithParams() will erase the > > > > > forward > > > > > > items > > > > > > > > in > > > > > > > > > the NavigationManager. I think that after my CL, the original > > > > > Reload() > > > > > > call > > > > > > > > > will work correctly, since the restored online URL will go > > > > > through the web > > > > > > > > view > > > > > > > > > codepath rather than attempting to reload the > > > > > OfflinePageNativeContent. > > > > > > > > > > > > > > > > FYI, this will not erase the forward items if |_virtualURL| == > > > > > |_entryURL| > > > > > > > > because in |-restoreOnlineURL| the URL of the lastCommittedItem > > > > > has been > > > > > > > changed > > > > > > > > to |_entryURL|, then |LoadURLWithParams| kicks off a new load to > > > > > add a new > > > > > > > > pending item whose URL is |_virtualURL|, and since the to be added > > > > > pending > > > > > > > item > > > > > > > > has the same URL as last committed item, the pending item will not > > > > > be > > > > > > created. > > > > > > > > Without pending item, |CommitPendingItem| or |ClearForwardItems| > > > > > will not be > > > > > > > > called. > > > > > > > > > > > > > > > > But anyway, Kurt is right about that Reload() should be sufficient > > > > > here, and > > > > > > > in > > > > > > > > general, I think we should avoid calling LoadURLWIthParams with > > > > > > > > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if one > > > > > needs to > > > > > > > > reload, one should call |Reload| explicitly. > > > > > > > > > > > > > > > > Thank you for fixing this. lgtm once Kurt's comment is addressed > > > > > :) > > > > > > > > > > > > > > I don't think that |_virtualURL| will ever be equal to |_entryURL|, > > > > > since the > > > > > > > virtual URL is a web URL and the entry URL is a chrome-scheme native > > > > > > controller > > > > > > > URL used to load offline pages. But in any case, I think that using > > > > > the fixed > > > > > > > Reload() will be the best approach here. > > > > > > > > > > > > My mistake, |_entryURL| is not chrome:// scheme. We should still use > > > > > > NavigationManager::Reload() though :) > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: > > > > > item->SetVirtualURL(_entryURL); > > > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > > > You might want to consider SetVirtualURL(GURL::EmptyGURL()) since > > > > > > NavigationItem::GetVirtualURL() will return the URL if the virtual URL > > > > > is empty. > > > > > > This is what we should be using for the "default" state, as setting > > > > > the virtual > > > > > > URL may introduce bugs where this NavigationItem's URL is updated, but > > > > > the > > > > > > virtual URL is still |_entryURL|. That may accidentally introduce a > > > > > URL > > > > > > spoofing bug where the URL that's loaded is to a different page than > > > > > the URL > > > > > > that is displayed in the omnibox. > > > > > > > > > > Done. > > > > > > > > > > https://codereview.chromium.org/2762113002/ > > > > > > > > > > > > > -- > > > > You received this message because you are subscribed to the Google Groups > > > > "Chromium-reviews" group. > > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > Guideline (https://www.chromium.org/developers/contributing-code) is 80 > > > characters. > > > Done anyway. > > It's actually 72. You can see this guide line when using "git cl upload". The > > reason is that git log eats 8 characters so 80-8 == 72 :) > > So should we not update https://www.chromium.org/developers/contributing-code ? FYI, Kurt is OOO, so you may want to TBR this CL.
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org, eugenebut@chromium.org, liaoyuke@chromium.org Link to the patchset: https://codereview.chromium.org/2762113002/#ps140001 (title: "fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: kkhorimoto@chromium.org. Please make sure to get positive review before another CQ attempt.
Description was changed from ========== Reading List iOS: Adapt loading offline for new reload process Loading offline pages relies on being able to reload page after changing the URL. Adapt to the new reload process which does not allow that. Also add EG tests for catching future problem. BUG=703393, 703206 ========== to ========== Reading List iOS: Adapt loading offline for new reload process Loading offline pages relies on being able to reload page after changing the URL. Adapt to the new reload process which does not allow that. Also add EG tests for catching future problem. BUG=703393, 703206 TBR=kkhorimoto_ ==========
The CQ bit was checked by olivierrobin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: kkhorimoto@chromium.org. Please make sure to get positive review before another CQ attempt.
Description was changed from ========== Reading List iOS: Adapt loading offline for new reload process Loading offline pages relies on being able to reload page after changing the URL. Adapt to the new reload process which does not allow that. Also add EG tests for catching future problem. BUG=703393, 703206 TBR=kkhorimoto_ ========== to ========== Reading List iOS: Adapt loading offline for new reload process Loading offline pages relies on being able to reload page after changing the URL. Adapt to the new reload process which does not allow that. Also add EG tests for catching future problem. BUG=703393, 703206 ==========
On 2017/03/22 20:08:05, liaoyuke wrote: > On 2017/03/22 17:36:15, Olivier Robin wrote: > > On 2017/03/22 17:35:18, Eugene But wrote: > > > On 2017/03/22 16:59:43, Olivier Robin wrote: > > > > On 2017/03/22 16:16:33, liaoyuke wrote: > > > > > btw, could you please format the description of the CL so that it meets > > the > > > > > 73 characters length requirement? > > > > > > > > > > Thanks! > > > > > > > > > > On Wed, Mar 22, 2017 at 6:33 AM <mailto:olivierrobin@chromium.org> > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > > > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: > > > > > > _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); > > > > > > On 2017/03/21 23:02:16, kkhorimoto_ wrote: > > > > > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > > > > > On 2017/03/21 22:49:09, liaoyuke wrote: > > > > > > > > > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > > > > > > > > > This is not correct. Using LoadURLWithParams() will erase the > > > > > > forward > > > > > > > items > > > > > > > > > in > > > > > > > > > > the NavigationManager. I think that after my CL, the original > > > > > > Reload() > > > > > > > call > > > > > > > > > > will work correctly, since the restored online URL will go > > > > > > through the web > > > > > > > > > view > > > > > > > > > > codepath rather than attempting to reload the > > > > > > OfflinePageNativeContent. > > > > > > > > > > > > > > > > > > FYI, this will not erase the forward items if |_virtualURL| == > > > > > > |_entryURL| > > > > > > > > > because in |-restoreOnlineURL| the URL of the lastCommittedItem > > > > > > has been > > > > > > > > changed > > > > > > > > > to |_entryURL|, then |LoadURLWithParams| kicks off a new load to > > > > > > add a new > > > > > > > > > pending item whose URL is |_virtualURL|, and since the to be > added > > > > > > pending > > > > > > > > item > > > > > > > > > has the same URL as last committed item, the pending item will > not > > > > > > be > > > > > > > created. > > > > > > > > > Without pending item, |CommitPendingItem| or |ClearForwardItems| > > > > > > will not be > > > > > > > > > called. > > > > > > > > > > > > > > > > > > But anyway, Kurt is right about that Reload() should be > sufficient > > > > > > here, and > > > > > > > > in > > > > > > > > > general, I think we should avoid calling LoadURLWIthParams with > > > > > > > > > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if > one > > > > > > needs to > > > > > > > > > reload, one should call |Reload| explicitly. > > > > > > > > > > > > > > > > > > Thank you for fixing this. lgtm once Kurt's comment is addressed > > > > > > :) > > > > > > > > > > > > > > > > I don't think that |_virtualURL| will ever be equal to > |_entryURL|, > > > > > > since the > > > > > > > > virtual URL is a web URL and the entry URL is a chrome-scheme > native > > > > > > > controller > > > > > > > > URL used to load offline pages. But in any case, I think that > using > > > > > > the fixed > > > > > > > > Reload() will be the best approach here. > > > > > > > > > > > > > > My mistake, |_entryURL| is not chrome:// scheme. We should still use > > > > > > > NavigationManager::Reload() though :) > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: > > > > > > item->SetVirtualURL(_entryURL); > > > > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > > > > You might want to consider SetVirtualURL(GURL::EmptyGURL()) since > > > > > > > NavigationItem::GetVirtualURL() will return the URL if the virtual > URL > > > > > > is empty. > > > > > > > This is what we should be using for the "default" state, as setting > > > > > > the virtual > > > > > > > URL may introduce bugs where this NavigationItem's URL is updated, > but > > > > > > the > > > > > > > virtual URL is still |_entryURL|. That may accidentally introduce a > > > > > > URL > > > > > > > spoofing bug where the URL that's loaded is to a different page than > > > > > > the URL > > > > > > > that is displayed in the omnibox. > > > > > > > > > > > > Done. > > > > > > > > > > > > https://codereview.chromium.org/2762113002/ > > > > > > > > > > > > > > > > -- > > > > > You received this message because you are subscribed to the Google > Groups > > > > > "Chromium-reviews" group. > > > > > To unsubscribe from this group and stop receiving emails from it, send > an > > > > email > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > Guideline (https://www.chromium.org/developers/contributing-code) is 80 > > > > characters. > > > > Done anyway. > > > It's actually 72. You can see this guide line when using "git cl upload". > The > > > reason is that git log eats 8 characters so 80-8 == 72 :) > > > > So should we not update https://www.chromium.org/developers/contributing-code > ? > > FYI, Kurt is OOO, so you may want to TBR this CL. I know, but you cannot TBR a not LGTMed CL. So we have to wait until he comes back from vacations.
On 2017/03/23 09:13:11, Olivier Robin wrote: > On 2017/03/22 20:08:05, liaoyuke wrote: > > On 2017/03/22 17:36:15, Olivier Robin wrote: > > > On 2017/03/22 17:35:18, Eugene But wrote: > > > > On 2017/03/22 16:59:43, Olivier Robin wrote: > > > > > On 2017/03/22 16:16:33, liaoyuke wrote: > > > > > > btw, could you please format the description of the CL so that it > meets > > > the > > > > > > 73 characters length requirement? > > > > > > > > > > > > Thanks! > > > > > > > > > > > > On Wed, Mar 22, 2017 at 6:33 AM <mailto:olivierrobin@chromium.org> > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > > > > File > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > > > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: > > > > > > > _webState->GetNavigationManager()->LoadURLWithParams(reloadParams); > > > > > > > On 2017/03/21 23:02:16, kkhorimoto_ wrote: > > > > > > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > > > > > > On 2017/03/21 22:49:09, liaoyuke wrote: > > > > > > > > > > On 2017/03/21 20:52:39, kkhorimoto_ wrote: > > > > > > > > > > > This is not correct. Using LoadURLWithParams() will erase > the > > > > > > > forward > > > > > > > > items > > > > > > > > > > in > > > > > > > > > > > the NavigationManager. I think that after my CL, the > original > > > > > > > Reload() > > > > > > > > call > > > > > > > > > > > will work correctly, since the restored online URL will go > > > > > > > through the web > > > > > > > > > > view > > > > > > > > > > > codepath rather than attempting to reload the > > > > > > > OfflinePageNativeContent. > > > > > > > > > > > > > > > > > > > > FYI, this will not erase the forward items if |_virtualURL| == > > > > > > > |_entryURL| > > > > > > > > > > because in |-restoreOnlineURL| the URL of the > lastCommittedItem > > > > > > > has been > > > > > > > > > changed > > > > > > > > > > to |_entryURL|, then |LoadURLWithParams| kicks off a new load > to > > > > > > > add a new > > > > > > > > > > pending item whose URL is |_virtualURL|, and since the to be > > added > > > > > > > pending > > > > > > > > > item > > > > > > > > > > has the same URL as last committed item, the pending item will > > not > > > > > > > be > > > > > > > > created. > > > > > > > > > > Without pending item, |CommitPendingItem| or > |ClearForwardItems| > > > > > > > will not be > > > > > > > > > > called. > > > > > > > > > > > > > > > > > > > > But anyway, Kurt is right about that Reload() should be > > sufficient > > > > > > > here, and > > > > > > > > > in > > > > > > > > > > general, I think we should avoid calling LoadURLWIthParams > with > > > > > > > > > > ui::PAGE_TRANSITION_RELOAD because it's counter-intuitive, if > > one > > > > > > > needs to > > > > > > > > > > reload, one should call |Reload| explicitly. > > > > > > > > > > > > > > > > > > > > Thank you for fixing this. lgtm once Kurt's comment is > addressed > > > > > > > :) > > > > > > > > > > > > > > > > > > I don't think that |_virtualURL| will ever be equal to > > |_entryURL|, > > > > > > > since the > > > > > > > > > virtual URL is a web URL and the entry URL is a chrome-scheme > > native > > > > > > > > controller > > > > > > > > > URL used to load offline pages. But in any case, I think that > > using > > > > > > > the fixed > > > > > > > > > Reload() will be the best approach here. > > > > > > > > > > > > > > > > My mistake, |_entryURL| is not chrome:// scheme. We should still > use > > > > > > > > NavigationManager::Reload() though :) > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/diff/100001/ios/chrome/browser/ui/... > > > > > > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: > > > > > > > item->SetVirtualURL(_entryURL); > > > > > > > On 2017/03/21 22:56:29, kkhorimoto_ wrote: > > > > > > > > You might want to consider SetVirtualURL(GURL::EmptyGURL()) since > > > > > > > > NavigationItem::GetVirtualURL() will return the URL if the virtual > > URL > > > > > > > is empty. > > > > > > > > This is what we should be using for the "default" state, as > setting > > > > > > > the virtual > > > > > > > > URL may introduce bugs where this NavigationItem's URL is updated, > > but > > > > > > > the > > > > > > > > virtual URL is still |_entryURL|. That may accidentally introduce > a > > > > > > > URL > > > > > > > > spoofing bug where the URL that's loaded is to a different page > than > > > > > > > the URL > > > > > > > > that is displayed in the omnibox. > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2762113002/ > > > > > > > > > > > > > > > > > > > -- > > > > > > You received this message because you are subscribed to the Google > > Groups > > > > > > "Chromium-reviews" group. > > > > > > To unsubscribe from this group and stop receiving emails from it, send > > an > > > > > email > > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > Guideline (https://www.chromium.org/developers/contributing-code) is 80 > > > > > characters. > > > > > Done anyway. > > > > It's actually 72. You can see this guide line when using "git cl upload". > > The > > > > reason is that git log eats 8 characters so 80-8 == 72 :) > > > > > > So should we not update > https://www.chromium.org/developers/contributing-code > > ? > > > > FYI, Kurt is OOO, so you may want to TBR this CL. > > I know, but you cannot TBR a not LGTMed CL. So we have to wait until he comes > back from vacations. Another option is to re-upload the CL and TBR the new CL.
lgtm! sorry, I was OOO for most of last week
lgtm! sorry, I was OOO for most of last week
The CQ bit was checked by olivierrobin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liaoyuke@chromium.org, jif@chromium.org, kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2762113002/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490780389034700,
"parent_rev": "011484c5417e6760493d222f678fcae0f6a94232", "commit_rev":
"f266b4e8d109a11ac0e3fae0b4c8500281e7c457"}
Message was sent while issue was closed.
Description was changed from ========== Reading List iOS: Adapt loading offline for new reload process Loading offline pages relies on being able to reload page after changing the URL. Adapt to the new reload process which does not allow that. Also add EG tests for catching future problem. BUG=703393, 703206 ========== to ========== Reading List iOS: Adapt loading offline for new reload process Loading offline pages relies on being able to reload page after changing the URL. Adapt to the new reload process which does not allow that. Also add EG tests for catching future problem. BUG=703393, 703206 Review-Url: https://codereview.chromium.org/2762113002 Cr-Commit-Position: refs/heads/master@{#460354} Committed: https://chromium.googlesource.com/chromium/src/+/f266b4e8d109a11ac0e3fae0b4c8... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f266b4e8d109a11ac0e3fae0b4c8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
