Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2824523002:
Remove usage of Tab's |url| property from BrowserViewController. (Closed)
|
Created:
3 years, 8 months ago by kkhorimoto Modified:
3 years, 6 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove usage of Tab's |url| property from BrowserViewController.
BUG=546208
Review-Url: https://codereview.chromium.org/2824523002
Cr-Commit-Position: refs/heads/master@{#476365}
Committed: https://chromium.googlesource.com/chromium/src/+/b110b266724abff80f6f9b3d910768444a2db1ff
Patch Set 1 #
Total comments: 16
Patch Set 2 : fixed test mock #
Messages
Total messages: 18 (9 generated)
Description was changed from ========== Eliminate usage of Tab's |url| property from BrowserViewController. BUG=546208 ========== to ========== Remove usage of Tab's |url| property from BrowserViewController. BUG=546208 ==========
kkhorimoto@chromium.org changed reviewers: + eugenebut@chromium.org, rohitrao@chromium.org, sdefresne@chromium.org
I've annotated with my reasoning for each change https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1110: return URL.is_valid() && !web::GetWebClient()->IsAppSpecificURL(URL); We want to share based on the visible content, so I think last committed is good here. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1589: if (tab.lastCommittedURL == GURL(kChromeUINewTabURL) && !_isOffTheRecord && If a Tab was just added, it's either a new tab, a window.open tab, or an open-in-new-tab context menu tab. It can only be an NTP if it's the first option, and native content URLs are never pending, so last committed is sufficient. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:2530: const GURL& committedURL = [_model currentTab].lastCommittedURL; Context menu is specific to visible content, so committed is sufficient here. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:3119: if (!currentTab || currentTab.lastCommittedURL != url || Native URLs are never pending, so last committed is sufficient. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:4473: web::Referrer referrer([strongTab lastCommittedURL], View source is specific to visible content, so last committed is sufficient. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:4521: if (currentTab && UrlHasChromeScheme(currentTab.lastCommittedURL)) { chrome:// scheme URLs are never pending, so last committed is sufficient https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:4977: node->url() == [_model currentTab].lastCommittedURL) { Depends on what we decide for https://codereview.chromium.org/2816203002; this should match the accessor we use there.
I'm not sure about |relinquishedToolbarController|, but everything else looks correct to me. lgtm (someone else should weight on relinquishedToolbarController) https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1110: return URL.is_valid() && !web::GetWebClient()->IsAppSpecificURL(URL); On 2017/04/14 18:48:19, kkhorimoto_ wrote: > We want to share based on the visible content, so I think last committed is good > here. This matches with adding bookmark functionality, so I agree https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1589: if (tab.lastCommittedURL == GURL(kChromeUINewTabURL) && !_isOffTheRecord && On 2017/04/14 18:48:19, kkhorimoto_ wrote: > If a Tab was just added, it's either a new tab, a window.open tab, or an > open-in-new-tab context menu tab. It can only be an NTP if it's the first > option, and native content URLs are never pending, so last committed is > sufficient. In the worst case this will only be a cosmetic bug, but this looks correct to me. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:2530: const GURL& committedURL = [_model currentTab].lastCommittedURL; On 2017/04/14 18:48:19, kkhorimoto_ wrote: > Context menu is specific to visible content, so committed is sufficient here. This change fixes a bug. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:3119: if (!currentTab || currentTab.lastCommittedURL != url || Do you want to use webState->GetLastCommmittedURL? Otherwise looks correct to me. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:4473: web::Referrer referrer([strongTab lastCommittedURL], On 2017/04/14 18:48:18, kkhorimoto_ wrote: > View source is specific to visible content, so last committed is sufficient. Looks correct, and this is also debug-only feature. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:4521: if (currentTab && UrlHasChromeScheme(currentTab.lastCommittedURL)) { On 2017/04/14 18:48:19, kkhorimoto_ wrote: > chrome:// scheme URLs are never pending, so last committed is sufficient I think WebUI URLs can be pending. https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:4977: node->url() == [_model currentTab].lastCommittedURL) { On 2017/04/14 18:48:19, kkhorimoto_ wrote: > Depends on what we decide for https://codereview.chromium.org/2816203002; this > should match the accessor we use there. This matches Desktop, so should be fine.
https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:4521: if (currentTab && UrlHasChromeScheme(currentTab.lastCommittedURL)) { On 2017/04/14 21:18:24, Eugene But wrote: > On 2017/04/14 18:48:19, kkhorimoto_ wrote: > > chrome:// scheme URLs are never pending, so last committed is sufficient > I think WebUI URLs can be pending. NTP is the only native page that's a ToolbarOwner, but I think pending might be safer to use here. Actually, we just changed the logic that specifies exactly when we swap toolbars. Does the logic here need to match?
https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2824523002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:4521: if (currentTab && UrlHasChromeScheme(currentTab.lastCommittedURL)) { On 2017/05/09 15:18:38, rohitrao (ping after 24h) wrote: > On 2017/04/14 21:18:24, Eugene But wrote: > > On 2017/04/14 18:48:19, kkhorimoto_ wrote: > > > chrome:// scheme URLs are never pending, so last committed is sufficient > > I think WebUI URLs can be pending. > > NTP is the only native page that's a ToolbarOwner, but I think pending might be > safer to use here. > > Actually, we just changed the logic that specifies exactly when we swap > toolbars. Does the logic here need to match? The toolbar visibility is updated when a navigation is committed, so last committed should work for that.
The CQ bit was checked by kkhorimoto@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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
fixed test mock
The CQ bit was checked by kkhorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2824523002/#ps20001 (title: "fixed test mock")
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": 20001, "attempt_start_ts": 1496341432804720,
"parent_rev": "5fdd27d882d65b574c74fdc6997536891405aceb", "commit_rev":
"64b68b1febc1f355016c2e6e2ef94f5b1b4f7a6a"}
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496341432804720,
"parent_rev": "05f56d412142ccc4f2d3d1b0defa2a05ee9a25bc", "commit_rev":
"b110b266724abff80f6f9b3d910768444a2db1ff"}
Message was sent while issue was closed.
Description was changed from ========== Remove usage of Tab's |url| property from BrowserViewController. BUG=546208 ========== to ========== Remove usage of Tab's |url| property from BrowserViewController. BUG=546208 Review-Url: https://codereview.chromium.org/2824523002 Cr-Commit-Position: refs/heads/master@{#476365} Committed: https://chromium.googlesource.com/chromium/src/+/b110b266724abff80f6f9b3d9107... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b110b266724abff80f6f9b3d9107... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
