Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2582373002:
Call registerLoadRequest with URL instead of VirtualURL. (Closed)
DescriptionCall registerLoadRequest with URL instead of VirtualURL.
registerLoadRequest must take a URL as it is comparing it with a URL.
BUG=671964
Committed: https://crrev.com/e948fa4c8f393ae2b69ba38183632e698cde8cfe
Cr-Commit-Position: refs/heads/master@{#439756}
Patch Set 1 #Patch Set 2 : fix virtual #
Total comments: 3
Patch Set 3 : registerLoadRequest #
Total comments: 6
Patch Set 4 : only registerLoadRequest #Messages
Total messages: 26 (16 generated)
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...
olivierrobin@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
https://codereview.chromium.org/2582373002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2582373002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2003: [self registerLoadRequest:virtualURL All other calls to this method take the virtualURL https://codereview.chromium.org/2582373002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2103: [self createWebUIForURL:currentURL]; I think it is OK to build webUI based on URL.
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...)
Description was changed from ========== Native provider must take the URL instead of the VirtualURL. Except when it is presenting an error NativeContent. BUG=671964 ========== to ========== Use URL instead of the VirtualURL when needed. Native Provider must take a URL except when it is presenting an error NativeContent. registerLoadRequest must take a URL as it is comparing it with a URL. BUG=671964 ==========
The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2582373002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2582373002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2003: [self registerLoadRequest:virtualURL On 2016/12/19 12:18:59, Olivier Robin wrote: > All other calls to this method take the virtualURL Obsolete. I add to change this everywhere as registerLoadRequest puts this URL in the URL field of the entry.
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: This issue passed the CQ dry run.
Thank you Olivier. Please wait for Kurt's comments as well. lgtm https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1992: const GURL targetURL = item ? item->GetURL() : GURL::EmptyGURL(); This looks right to me and I tested that Native Content works as expected for about://terms https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2089: const GURL currentURL = item ? item->GetURL() : GURL::EmptyGURL(); This looks right to me and I tested that WebUI works as expected for about://version page. https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:5632: currentItem ? currentItem->GetURL() : GURL::EmptyGURL(); Changing |loadRequestForCurrentNavigationItem| looks correct to me, though I would probably pref to land this separately from the previous 2 changes (so if it breaks something we can revert fewer changes).
https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2002: [self registerLoadRequest:targetURL I can split the CL, but please note that because of this call, the two CL will be dependent.
On 2016/12/19 20:09:55, Olivier Robin wrote: > https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... > File ios/web/web_state/ui/crw_web_controller.mm (right): > > https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_web_controller.mm:2002: [self > registerLoadRequest:targetURL > I can split the CL, but please note that because of this call, the two CL will > be dependent. Thanks. Ideally it would be great to hear from Kurt to avoid extra work.
These all look like appropriate places to use URL instead of VirtualURL. lgtm https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2002: [self registerLoadRequest:targetURL On 2016/12/19 20:09:55, Olivier Robin wrote: > I can split the CL, but please note that because of this call, the two CL will > be dependent. You can upload dependent CLs by using 'git-new-branch --upstream-current'
Thanks. I will try to split this CL in two. and submit the first part. Note that for the other usages of VirtualURL, I estimated that VirtualURL was the right choice, but they may also need a review by somebody knowing the navigation stack. I will let the bug open and assigned to kkhorimoto_ to have another check on them. https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2582373002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2002: [self registerLoadRequest:targetURL On 2016/12/19 21:11:54, kkhorimoto_ wrote: > On 2016/12/19 20:09:55, Olivier Robin wrote: > > I can split the CL, but please note that because of this call, the two CL will > > be dependent. > > You can upload dependent CLs by using 'git-new-branch --upstream-current' OK. Splitting the CL will also mean adapt the unittests and revert the adapt in second CL (see patch set 2 in this CL).. I will check if the failure is coherent or if the two changes are really interdependant.
Description was changed from ========== Use URL instead of the VirtualURL when needed. Native Provider must take a URL except when it is presenting an error NativeContent. registerLoadRequest must take a URL as it is comparing it with a URL. BUG=671964 ========== to ========== Call registerLoadRequest with URL instead of VirtualURL. registerLoadRequest must take a URL as it is comparing it with a URL. BUG=671964 ==========
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2582373002/#ps60001 (title: "only registerLoadRequest")
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": 60001, "attempt_start_ts": 1482223669400830,
"parent_rev": "2ca2223e0b83b38c6c7ac3caa220d7c0c4ada229", "commit_rev":
"686f6f3da6de5f84a98dd84358d853239b26ab2c"}
Message was sent while issue was closed.
Description was changed from ========== Call registerLoadRequest with URL instead of VirtualURL. registerLoadRequest must take a URL as it is comparing it with a URL. BUG=671964 ========== to ========== Call registerLoadRequest with URL instead of VirtualURL. registerLoadRequest must take a URL as it is comparing it with a URL. BUG=671964 Review-Url: https://codereview.chromium.org/2582373002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Call registerLoadRequest with URL instead of VirtualURL. registerLoadRequest must take a URL as it is comparing it with a URL. BUG=671964 Review-Url: https://codereview.chromium.org/2582373002 ========== to ========== Call registerLoadRequest with URL instead of VirtualURL. registerLoadRequest must take a URL as it is comparing it with a URL. BUG=671964 Committed: https://crrev.com/e948fa4c8f393ae2b69ba38183632e698cde8cfe Cr-Commit-Position: refs/heads/master@{#439756} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e948fa4c8f393ae2b69ba38183632e698cde8cfe Cr-Commit-Position: refs/heads/master@{#439756} |
