Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2581193002:
Remove CRWWebController currentNavigationURL (Closed)
DescriptionRemove CRWWebController currentNavigationURL
This method returns the VirtualURL of the current navigation and is used at some
places where GetURL should be used instead.
Remove the method to avoid confusion.
Further CLs will audit which usage should be VirtualURL and which should be URL.
BUG=671964
Committed: https://crrev.com/f96b83410bafa951fcddfef3d225964937bac581
Cr-Commit-Position: refs/heads/master@{#439445}
Patch Set 1 #
Total comments: 10
Patch Set 2 : feedback #Messages
Total messages: 15 (8 generated)
Description was changed from ========== Remove CRWWebController GetCurrentNavigationURL This method returns the VirtualURL of the current navigation and is use BUG=671964 ========== to ========== Remove CRWWebController GetCurrentNavigationURL This method returns the VirtualURL of the current navigation and is used at some places where GetURL should be used instead. Remove the method to avoid confusion. Further CLs will audit which usage should be VirtualURL and which should be URL. BUG=671964 ==========
olivierrobin@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
Description was changed from ========== Remove CRWWebController GetCurrentNavigationURL This method returns the VirtualURL of the current navigation and is used at some places where GetURL should be used instead. Remove the method to avoid confusion. Further CLs will audit which usage should be VirtualURL and which should be URL. BUG=671964 ========== to ========== Remove CRWWebController currentNavigationURL This method returns the VirtualURL of the current navigation and is used at some places where GetURL should be used instead. Remove the method to avoid confusion. Further CLs will audit which usage should be VirtualURL and which should be URL. BUG=671964 ==========
Thank you for doing this. lgtm! https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:1835: GURL navigationURL = item ? item->GetVirtualURL() : GURL::EmptyGURL(); s/navigationURL/targetURL and drop |targetURL| variable on line 1838 https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2014: [self currentNavItem]->SetVirtualURL([nativeContent virtualURL]); nit: s/[self currentNavItem]/item https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2107: web::NavigationItem* item = [self currentNavItem]; Please drop this. https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:4825: if (allowCache) { How about this?: web::NavigationItem* item = [self currentNavItem]; if (allowCache && item) { _expectedReconstructionURL = item->GetVirtualURL(); } else { _expectedReconstructionURL = GURL::EmptyGURL(); } https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:5664: web::NavigationItem* item = [self currentNavItem]; Do you need this variable? Can you use use |currentItem| instead?.
lgtm with Eugene's nits. Thanks for 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 kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2581193002/#ps20001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done, thanks https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:1835: GURL navigationURL = item ? item->GetVirtualURL() : GURL::EmptyGURL(); On 2016/12/16 17:12:48, Eugene But wrote: > s/navigationURL/targetURL and drop |targetURL| variable on line 1838 Done. https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2014: [self currentNavItem]->SetVirtualURL([nativeContent virtualURL]); On 2016/12/16 17:12:48, Eugene But wrote: > nit: s/[self currentNavItem]/item Done. https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2107: web::NavigationItem* item = [self currentNavItem]; On 2016/12/16 17:12:48, Eugene But wrote: > Please drop this. Done. https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:4825: if (allowCache) { On 2016/12/16 17:12:48, Eugene But wrote: > How about this?: > > web::NavigationItem* item = [self currentNavItem]; > if (allowCache && item) { > _expectedReconstructionURL = item->GetVirtualURL(); > } else { > _expectedReconstructionURL = GURL::EmptyGURL(); > } Done. https://codereview.chromium.org/2581193002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:5664: web::NavigationItem* item = [self currentNavItem]; On 2016/12/16 17:12:48, Eugene But wrote: > Do you need this variable? Can you use use |currentItem| instead?. Done.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482140970710240,
"parent_rev": "07334adbfccf6d13b5ac616cf8eb8f0942d684f3", "commit_rev":
"f73d31431a7e1fd7067c0eed3f50bc0d3aedc854"}
Message was sent while issue was closed.
Description was changed from ========== Remove CRWWebController currentNavigationURL This method returns the VirtualURL of the current navigation and is used at some places where GetURL should be used instead. Remove the method to avoid confusion. Further CLs will audit which usage should be VirtualURL and which should be URL. BUG=671964 ========== to ========== Remove CRWWebController currentNavigationURL This method returns the VirtualURL of the current navigation and is used at some places where GetURL should be used instead. Remove the method to avoid confusion. Further CLs will audit which usage should be VirtualURL and which should be URL. BUG=671964 Review-Url: https://codereview.chromium.org/2581193002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove CRWWebController currentNavigationURL This method returns the VirtualURL of the current navigation and is used at some places where GetURL should be used instead. Remove the method to avoid confusion. Further CLs will audit which usage should be VirtualURL and which should be URL. BUG=671964 Review-Url: https://codereview.chromium.org/2581193002 ========== to ========== Remove CRWWebController currentNavigationURL This method returns the VirtualURL of the current navigation and is used at some places where GetURL should be used instead. Remove the method to avoid confusion. Further CLs will audit which usage should be VirtualURL and which should be URL. BUG=671964 Committed: https://crrev.com/f96b83410bafa951fcddfef3d225964937bac581 Cr-Commit-Position: refs/heads/master@{#439445} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f96b83410bafa951fcddfef3d225964937bac581 Cr-Commit-Position: refs/heads/master@{#439445} |
