Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2711683002:
(Set)IsOverridingUserAgent should be called on VisibleItem (Closed)
|
Created:
3 years, 10 months ago by liaoyuke Modified:
3 years, 10 months ago CC:
chromium-reviews, marq+watch_chromium.org, Eugene But (OOO till 7-30), pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description(Set)IsOverridingUserAgent should be called on VisibleItem
As explained in the associated bug, it always makes sense to call
IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item
instead of current item. Thus this CL changes CurrentItem to VisibleItem
wherever necessary.
Besides, this CL also does the following refactoring to have
NavigationManagerImpl encapsulate VisibleItem for calling
(Set)IsOverridingUserAgent() to provide one more layer of abstraction.
BUG=693794
Review-Url: https://codereview.chromium.org/2711683002
Cr-Commit-Position: refs/heads/master@{#452570}
Committed: https://chromium.googlesource.com/chromium/src/+/21359189a7883d38a531e8681cc250bdf301fa85
Patch Set 1 #
Total comments: 19
Patch Set 2 : Addressed feedback #
Total comments: 2
Patch Set 3 : Rename function names #
Total comments: 13
Patch Set 4 : Addressed feedback #Patch Set 5 : Remove IsUsingDesktopUserAgent in NavigationManagerImpl #
Total comments: 7
Patch Set 6 : Addressed feedback #Patch Set 7 : Update function name #
Total comments: 6
Patch Set 8 : Remove getter for usesDesktopUserAgent property #
Total comments: 8
Patch Set 9 : Change to call property method using dot #
Messages
Total messages: 37 (11 generated)
liaoyuke@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
Hi Eugene, Kurt, PTAL. Thank you very much! https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (left): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:501: currentItem->GetURL(), URL)); Confirmed with Kurt and it is safe to change all currentItem to visibleItem.
I will let Kurt to do first pass review
The logic looks good, but I have a few naming suggestions to make the code clearer. https://codereview.chromium.org/2711683002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2711683002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1528: - (BOOL)useDesktopUserAgent { The naming of this property is a little confusing, as it's using an imperative voice (i.e. by the name of the function, I would expect that calling this would instruct the Tab to begin using a desktop UA). Can we rename the property as follows? @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) BOOL usingDesktopUserAgent? - (BOOL)usingDesktopUserAgent { return [self navigationManager]->IsUsingDesktopUserAgent(); } https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:79: BOOL _useDesktopUserAgentForNextPendingItem; I missed this while reviewing your other CL, but can you remove this since it's no longer used? https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/navigati... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:149: bool GetUseDesktopUserAgent() const; This is similarly confusingly named. Howe about "IsUsingDesktopUserAgent()"? https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:155: void OverrideDesktopUserAgentForNextPendingItem(); Since these functions are called by Tab (i.e. in the Chrome layer), they need to be in NavigationManager instead of NavigationManagerImpl. Currently, Tab includes the NavigationManagerImpl header, but eventually it will depend only on NavigationManager. Please update the CL description after changing this back to the public interface. https://codereview.chromium.org/2711683002/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2365: - (BOOL)useDesktopUserAgent { This property is only used once in |-webViewWithConfiguration:|. Let's just remove the property and inline the call to the NavigationManager function here.
https://codereview.chromium.org/2711683002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2711683002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1528: - (BOOL)useDesktopUserAgent { On 2017/02/22 01:08:40, kkhorimoto_ wrote: > The naming of this property is a little confusing, as it's using an imperative > voice (i.e. by the name of the function, I would expect that calling this would > instruct the Tab to begin using a desktop UA). Can we rename the property as > follows? > > @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) BOOL > usingDesktopUserAgent? > > - (BOOL)usingDesktopUserAgent { > return [self navigationManager]->IsUsingDesktopUserAgent(); > } Oops, made a mistake, the implementation would be: - (BOOL)isUsingDesktopUserAgent { return [self navigationManager]->IsUsingDesktopUserAgent(); }
Description was changed from ========== (Set)IsOverridingUserAgent should be called on VisibleItem As explained in the associated bug, it always makes sense to call IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item instead of current item. Thus this CL changes CurrentItem to VisibleItem wherever necessary. Besides, this CL also does the following refactoring: 1. Have NavigationManagerImpl encapsulate VisibleItem for calling (Set)IsOverridingUserAgent() to provide one more layer of abstraction. 2. Move user agent code out of NavigationManger as they are only needed in NavigationManagerImpl. BUG=693794 ========== to ========== (Set)IsOverridingUserAgent should be called on VisibleItem As explained in the associated bug, it always makes sense to call IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item instead of current item. Thus this CL changes CurrentItem to VisibleItem wherever necessary. Besides, this CL also does the following refactoring to have NavigationManagerImpl encapsulate VisibleItem for calling (Set)IsOverridingUserAgent() to provide one more layer of abstraction. BUG=693794 ==========
PTAL. Thanks! https://codereview.chromium.org/2711683002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2711683002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1528: - (BOOL)useDesktopUserAgent { On 2017/02/22 01:08:40, kkhorimoto_ wrote: > The naming of this property is a little confusing, as it's using an imperative > voice (i.e. by the name of the function, I would expect that calling this would > instruct the Tab to begin using a desktop UA). Can we rename the property as > follows? > > @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) BOOL > usingDesktopUserAgent? > > - (BOOL)usingDesktopUserAgent { > return [self navigationManager]->IsUsingDesktopUserAgent(); > } yeah, I also don't like the name of the function, but in Android, it's named getUseDesktopUserAgent https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Do we want to follow Android's name or change it to isUsingDesktopUserAgent? https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:79: BOOL _useDesktopUserAgentForNextPendingItem; On 2017/02/22 01:08:40, kkhorimoto_ wrote: > I missed this while reviewing your other CL, but can you remove this since it's > no longer used? Done. https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/navigati... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:149: bool GetUseDesktopUserAgent() const; On 2017/02/22 01:08:40, kkhorimoto_ wrote: > This is similarly confusingly named. Howe about "IsUsingDesktopUserAgent()"? Given that Android uses this name, do we still want to change it? https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:155: void OverrideDesktopUserAgentForNextPendingItem(); On 2017/02/22 01:08:40, kkhorimoto_ wrote: > Since these functions are called by Tab (i.e. in the Chrome layer), they need to > be in NavigationManager instead of NavigationManagerImpl. Currently, Tab > includes the NavigationManagerImpl header, but eventually it will depend only on > NavigationManager. Please update the CL description after changing this back to > the public interface. Got it. Thank you for explaining. https://codereview.chromium.org/2711683002/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2365: - (BOOL)useDesktopUserAgent { On 2017/02/22 01:08:40, kkhorimoto_ wrote: > This property is only used once in |-webViewWithConfiguration:|. Let's just > remove the property and inline the call to the NavigationManager function here. Done.
https://codereview.chromium.org/2711683002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2711683002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1528: - (BOOL)useDesktopUserAgent { On 2017/02/22 01:37:43, liaoyuke wrote: > On 2017/02/22 01:08:40, kkhorimoto_ wrote: > > The naming of this property is a little confusing, as it's using an imperative > > voice (i.e. by the name of the function, I would expect that calling this > would > > instruct the Tab to begin using a desktop UA). Can we rename the property as > > follows? > > > > @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) BOOL > > usingDesktopUserAgent? > > > > - (BOOL)usingDesktopUserAgent { > > return [self navigationManager]->IsUsingDesktopUserAgent(); > > } > > yeah, I also don't like the name of the function, but in Android, it's named > getUseDesktopUserAgent > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > Do we want to follow Android's name or change it to isUsingDesktopUserAgent? I think we should go ahead and use isUsingDesktopUserAgent. content// and Android have been around for longer, which is why we often use them as a reference for implementation details, but naming/style doesn't have to match exactly. Given that renaming this would increase readability of the code, I'd say this is a good place where we should diverge. Also, just a heads up...when defining BOOL properties where the getter is different from the property name, you use the property name when accessing via dot notation and the getter name when accessing using selector notation: @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) BOOL usingDesktopUserAgent; should be accessed as: tab.usingDesktopUserAgent OR [tab isUsingDesktopUserAgent] https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/navigati... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:149: bool GetUseDesktopUserAgent() const; On 2017/02/22 01:37:43, liaoyuke wrote: > On 2017/02/22 01:08:40, kkhorimoto_ wrote: > > This is similarly confusingly named. Howe about "IsUsingDesktopUserAgent()"? > > Given that Android uses this name, do we still want to change it? Yes, I think we should change it. https://codereview.chromium.org/2711683002/diff/20001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2711683002/diff/20001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:150: bool GetUseDesktopUserAgent() const; This should also be moved to the public NavigationManager interface, for the same reason as you moved the other function.
Totally agree that we should not be afraid to diverge if we are doing something better than Content and Android. PTAL! Thank you very much for the detailed note and explanation! https://codereview.chromium.org/2711683002/diff/20001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2711683002/diff/20001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:150: bool GetUseDesktopUserAgent() const; On 2017/02/22 01:54:33, kkhorimoto_ wrote: > This should also be moved to the public NavigationManager interface, for the > same reason as you moved the other function. Done.
lgtm!
https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (left): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:501: currentItem->GetURL(), URL)); On 2017/02/22 00:12:32, liaoyuke wrote: > Confirmed with Kurt and it is safe to change all currentItem to visibleItem. Why is this visible? Should this be lastCommittedItem instead? https://codereview.chromium.org/2711683002/diff/40001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.h (right): https://codereview.chromium.org/2711683002/diff/40001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:138: @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) Should this be usesUsingDesktopUserAgent ? "BOOL uses" name can be found in UIKit and Foundation: BOOL usesFontLeading BOOL usesEvenOddFillRule BOOL usesMetricSystem BOOL usesGroupingSeparator BOOL usesSignificantDigits BOOL usesStrongWriteBarrier BOOL usesWeakReadAndWriteBarriers while "BOOL using" is in never used in those frameworks. https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:424: return GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent(); Why Visible? Should this be LastCommittedItem? According to the comments this represents "currently loaded page", which is LastCommittedItem, not visible item. https://codereview.chromium.org/2711683002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4201: self.navigationManagerImpl->IsUsingDesktopUserAgent()); Is it guaranteed that |self.navigationManagerImpl| never returns null here?
https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (left): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:501: currentItem->GetURL(), URL)); On 2017/02/22 21:57:41, Eugene But wrote: > On 2017/02/22 00:12:32, liaoyuke wrote: > > Confirmed with Kurt and it is safe to change all currentItem to visibleItem. > Why is this visible? Should this be lastCommittedItem instead? Since a page showing an interstitial can't do a pushState, we know that if both the DCHECKs above pass, then the visible item is equal to the last committed item. I guess we should just use the lastCommittedItem here to avoid doing unnecessary logic in GetVisibleItem. https://codereview.chromium.org/2711683002/diff/40001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.h (right): https://codereview.chromium.org/2711683002/diff/40001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:138: @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) On 2017/02/22 21:57:41, Eugene But wrote: > Should this be usesUsingDesktopUserAgent ? > > "BOOL uses" name can be found in UIKit and Foundation: > BOOL usesFontLeading > BOOL usesEvenOddFillRule > BOOL usesMetricSystem > BOOL usesGroupingSeparator > BOOL usesSignificantDigits > BOOL usesStrongWriteBarrier > BOOL usesWeakReadAndWriteBarriers > > while "BOOL using" is in never used in those frameworks. I'm fine with "uses" or "using", as long as it's not "useDesktopUserAgent" :) https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:424: return GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent(); On 2017/02/22 21:57:42, Eugene But wrote: > Why Visible? Should this be LastCommittedItem? According to the comments this > represents "currently loaded page", which is LastCommittedItem, not visible > item. When reviewing, I was looking at the places this is used, which is in the BVC to enable/disable the desktop UA option. If there's a user-initiated navigation happening and the user taps on the "use desktop user agent" button, then presumably, they wanted to load the page they typed out using the desktop UA, which may be the visible item if it's taking a long time to load. Maybe to solve this problem, BVC should be manually inspecting the visible item rather than writing logic here that's used specifically for the BVC's purposes.
https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:424: return GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent(); On 2017/02/22 22:54:15, kkhorimoto_ wrote: > On 2017/02/22 21:57:42, Eugene But wrote: > > Why Visible? Should this be LastCommittedItem? According to the comments this > > represents "currently loaded page", which is LastCommittedItem, not visible > > item. > > When reviewing, I was looking at the places this is used, which is in the BVC to > enable/disable the desktop UA option. If there's a user-initiated navigation > happening and the user taps on the "use desktop user agent" button, then > presumably, they wanted to load the page they typed out using the desktop UA, > which may be the visible item if it's taking a long time to load. Maybe to > solve this problem, BVC should be manually inspecting the visible item rather > than writing logic here that's used specifically for the BVC's purposes. Makes sense. Yuke, please update header comments to reflect what method does.
PTAL. Thanks! https://codereview.chromium.org/2711683002/diff/40001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.h (right): https://codereview.chromium.org/2711683002/diff/40001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:138: @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) On 2017/02/22 22:54:15, kkhorimoto_ wrote: > On 2017/02/22 21:57:41, Eugene But wrote: > > Should this be usesUsingDesktopUserAgent ? > > > > "BOOL uses" name can be found in UIKit and Foundation: > > BOOL usesFontLeading > > BOOL usesEvenOddFillRule > > BOOL usesMetricSystem > > BOOL usesGroupingSeparator > > BOOL usesSignificantDigits > > BOOL usesStrongWriteBarrier > > BOOL usesWeakReadAndWriteBarriers > > > > while "BOOL using" is in never used in those frameworks. > > I'm fine with "uses" or "using", as long as it's not "useDesktopUserAgent" :) Acknowledged. https://codereview.chromium.org/2711683002/diff/40001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:138: @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) On 2017/02/22 21:57:41, Eugene But wrote: > Should this be usesUsingDesktopUserAgent ? > > "BOOL uses" name can be found in UIKit and Foundation: > BOOL usesFontLeading > BOOL usesEvenOddFillRule > BOOL usesMetricSystem > BOOL usesGroupingSeparator > BOOL usesSignificantDigits > BOOL usesStrongWriteBarrier > BOOL usesWeakReadAndWriteBarriers > > while "BOOL using" is in never used in those frameworks. Done. https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:424: return GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent(); On 2017/02/22 22:54:15, kkhorimoto_ wrote: > On 2017/02/22 21:57:42, Eugene But wrote: > > Why Visible? Should this be LastCommittedItem? According to the comments this > > represents "currently loaded page", which is LastCommittedItem, not visible > > item. > > When reviewing, I was looking at the places this is used, which is in the BVC to > enable/disable the desktop UA option. If there's a user-initiated navigation > happening and the user taps on the "use desktop user agent" button, then > presumably, they wanted to load the page they typed out using the desktop UA, > which may be the visible item if it's taking a long time to load. Maybe to > solve this problem, BVC should be manually inspecting the visible item rather > than writing logic here that's used specifically for the BVC's purposes. Acknowledged. https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:424: return GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent(); On 2017/02/22 22:54:15, kkhorimoto_ wrote: > On 2017/02/22 21:57:42, Eugene But wrote: > > Why Visible? Should this be LastCommittedItem? According to the comments this > > represents "currently loaded page", which is LastCommittedItem, not visible > > item. > > When reviewing, I was looking at the places this is used, which is in the BVC to > enable/disable the desktop UA option. If there's a user-initiated navigation > happening and the user taps on the "use desktop user agent" button, then > presumably, they wanted to load the page they typed out using the desktop UA, > which may be the visible item if it's taking a long time to load. Maybe to > solve this problem, BVC should be manually inspecting the visible item rather > than writing logic here that's used specifically for the BVC's purposes. Isn't it the case that CRWWebController also uses this method to decide how to build the web view? Given that user agent status should always be queried on visible item, I actually think it makes sense to wrap this method inside NavigationManagerImpl, otherwise, every time someone wants to get the user agent status, one has to think whether the caller should query the last committedItem or currentItem or visibleItem, which is quite error prone. https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:424: return GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent(); On 2017/02/22 23:13:56, Eugene But wrote: > On 2017/02/22 22:54:15, kkhorimoto_ wrote: > > On 2017/02/22 21:57:42, Eugene But wrote: > > > Why Visible? Should this be LastCommittedItem? According to the comments > this > > > represents "currently loaded page", which is LastCommittedItem, not visible > > > item. > > > > When reviewing, I was looking at the places this is used, which is in the BVC > to > > enable/disable the desktop UA option. If there's a user-initiated navigation > > happening and the user taps on the "use desktop user agent" button, then > > presumably, they wanted to load the page they typed out using the desktop UA, > > which may be the visible item if it's taking a long time to load. Maybe to > > solve this problem, BVC should be manually inspecting the visible item rather > > than writing logic here that's used specifically for the BVC's purposes. > Makes sense. Yuke, please update header comments to reflect what method does. Done. https://codereview.chromium.org/2711683002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4201: self.navigationManagerImpl->IsUsingDesktopUserAgent()); On 2017/02/22 21:57:42, Eugene But wrote: > Is it guaranteed that |self.navigationManagerImpl| never returns null here? Good catch!
https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:424: return GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent(); > Given that user agent status should always be queried on visible item, I > actually think it makes sense to wrap this method inside NavigationManagerImpl, > otherwise, every time someone wants to get the user agent status, one has to > think whether the caller should query the last committedItem or currentItem or > visibleItem, which is quite error prone. So this statement made me rethink this approach. Querying |IsOverridingUserAgent| on NavigationItem is actually not error prone, because the caller can clearly tell if they query the flag on LastCommitted, Pending or Visible Item. Querying |IsOverridingUserAgent| on NavigationManager is error prone, because people would assume that this code "just does the right thing", but I can imagine cases when callers actually need to query UA flag on current page, not on pending page. How about removing |IsUsingDesktopUserAgent| from navigation manager, so clients have to explicitly specify which item they need.
On 2017/02/23 00:03:14, Eugene But wrote: > https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... > File ios/web/navigation/navigation_manager_impl.mm (right): > > https://codereview.chromium.org/2711683002/diff/40001/ios/web/navigation/navi... > ios/web/navigation/navigation_manager_impl.mm:424: return GetVisibleItem() && > GetVisibleItem()->IsOverridingUserAgent(); > > Given that user agent status should always be queried on visible item, I > > actually think it makes sense to wrap this method inside > NavigationManagerImpl, > > otherwise, every time someone wants to get the user agent status, one has to > > think whether the caller should query the last committedItem or currentItem or > > visibleItem, which is quite error prone. > So this statement made me rethink this approach. Querying > |IsOverridingUserAgent| on NavigationItem is actually not error prone, because > the caller can clearly tell if they query the flag on LastCommitted, Pending or > Visible Item. Querying |IsOverridingUserAgent| on NavigationManager is error > prone, because people would assume that this code "just does the right thing", > but I can imagine cases when callers actually need to query UA flag on current > page, not on pending page. How about removing |IsUsingDesktopUserAgent| from > navigation manager, so clients have to explicitly specify which item they need. I agree if visible item is not the only item on which UA flag should be queried.
PTAL. Thanks!
https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (left): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:501: currentItem->GetURL(), URL)); On 2017/02/22 22:54:15, kkhorimoto_ wrote: > On 2017/02/22 21:57:41, Eugene But wrote: > > On 2017/02/22 00:12:32, liaoyuke wrote: > > > Confirmed with Kurt and it is safe to change all currentItem to visibleItem. > > Why is this visible? Should this be lastCommittedItem instead? > > Since a page showing an interstitial can't do a pushState, we know that if both > the DCHECKs above pass, then the visible item is equal to the last committed > item. I guess we should just use the lastCommittedItem here to avoid doing > unnecessary logic in GetVisibleItem. Yuke, please address this comment. https://codereview.chromium.org/2711683002/diff/80001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.h (right): https://codereview.chromium.org/2711683002/diff/80001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:137: // Whether or not desktop user agent is used for the currently loaded page. Please update the comment, because visibleItem does not represent "currently loaded page". https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4205: web::NavigationItem* visibleItem = Is there a reason for removing |useDesktopUserAgent| method? The old code looked cleaner. https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4208: BOOL isUsingDesktopUseragent = shouldUseDesktopUserAgent Agent capitalized, |is| prefix is good for method names, not for variable names in Objective-C.
PTAL. Thanks! https://codereview.chromium.org/2711683002/diff/80001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.h (right): https://codereview.chromium.org/2711683002/diff/80001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:137: // Whether or not desktop user agent is used for the currently loaded page. On 2017/02/23 00:54:23, Eugene But wrote: > Please update the comment, because visibleItem does not represent "currently > loaded page". Done. https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4205: web::NavigationItem* visibleItem = On 2017/02/23 00:54:24, Eugene But wrote: > Is there a reason for removing |useDesktopUserAgent| method? The old code looked > cleaner. Done. https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4208: BOOL isUsingDesktopUseragent = On 2017/02/23 00:54:24, Eugene But wrote: > shouldUseDesktopUserAgent > > Agent capitalized, |is| prefix is good for method names, not for variable names > in Objective-C. Acknowledged.
https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4205: web::NavigationItem* visibleItem = On 2017/02/23 01:10:14, liaoyuke wrote: > On 2017/02/23 00:54:24, Eugene But wrote: > > Is there a reason for removing |useDesktopUserAgent| method? The old code > looked > > cleaner. > > Done. I suggested removing the old property because of the same reason we wanted to remove NavigationManager::IsUsingDesktopuserAgent(); it's less error prone to look inspect a specified item from the NavigationManager than abstracting away to another function. Since this is the only place it's used in this class, it seemed a better solution to include the logic here. If we do want to reintroduce the property though, we should use the same naming scheme that we ultimately used in Tab, as |useDesktopUserAgent| is poorly named.
On 2017/02/23 01:16:25, kkhorimoto_ wrote: > https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... > File ios/web/web_state/ui/crw_web_controller.mm (right): > > https://codereview.chromium.org/2711683002/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_web_controller.mm:4205: web::NavigationItem* > visibleItem = > On 2017/02/23 01:10:14, liaoyuke wrote: > > On 2017/02/23 00:54:24, Eugene But wrote: > > > Is there a reason for removing |useDesktopUserAgent| method? The old code > > looked > > > cleaner. > > > > Done. > > I suggested removing the old property because of the same reason we wanted to > remove NavigationManager::IsUsingDesktopuserAgent(); it's less error prone to > look inspect a specified item from the NavigationManager than abstracting away > to another function. Since this is the only place it's used in this class, it > seemed a better solution to include the logic here. If we do want to > reintroduce the property though, we should use the same naming scheme that we > ultimately used in Tab, as |useDesktopUserAgent| is poorly named. Hey Kurt, We think it's reasonable to drop the method in the previous round because the code was simple. But now code has become more complicated after adding the logic to access visible item and nil check, so that's why I added the method back. Thank you for the careful review and catching the poor name thing, I will change it right away :)
Yuke, please address comment for crw_session_controller.mm https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (left): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:501: currentItem->GetURL(), URL)); On 2017/02/23 00:54:23, Eugene But wrote: > On 2017/02/22 22:54:15, kkhorimoto_ wrote: > > On 2017/02/22 21:57:41, Eugene But wrote: > > > On 2017/02/22 00:12:32, liaoyuke wrote: > > > > Confirmed with Kurt and it is safe to change all currentItem to > visibleItem. > > > Why is this visible? Should this be lastCommittedItem instead? > > > > Since a page showing an interstitial can't do a pushState, we know that if > both > > the DCHECKs above pass, then the visible item is equal to the last committed > > item. I guess we should just use the lastCommittedItem here to avoid doing > > unnecessary logic in GetVisibleItem. > Yuke, please address this comment. Yuke? https://codereview.chromium.org/2711683002/diff/120001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab.h (right): https://codereview.chromium.org/2711683002/diff/120001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.h:138: @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) Sorry. Did not notice before. Drop isUsingDesktopUserAgent, it's not correct name for |usesDesktopUserAgent| property. https://codereview.chromium.org/2711683002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:463: @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) Same comment here. https://codereview.chromium.org/2711683002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2344: web::NavigationItem* visibleItem = nit: How about this?: if (!self.navigationManagerImpl) return NO; auto visibleItem = self.navigationManagerImpl->GetVisibleItem(); return visibleItem && visibleItem->IsOverridingUserAgent();
Patchset #8 (id:140001) has been deleted
PTAL. Thank you very much! https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (left): https://codereview.chromium.org/2711683002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:501: currentItem->GetURL(), URL)); On 2017/02/23 02:36:59, Eugene But wrote: > On 2017/02/23 00:54:23, Eugene But wrote: > > On 2017/02/22 22:54:15, kkhorimoto_ wrote: > > > On 2017/02/22 21:57:41, Eugene But wrote: > > > > On 2017/02/22 00:12:32, liaoyuke wrote: > > > > > Confirmed with Kurt and it is safe to change all currentItem to > > visibleItem. > > > > Why is this visible? Should this be lastCommittedItem instead? > > > > > > Since a page showing an interstitial can't do a pushState, we know that if > > both > > > the DCHECKs above pass, then the visible item is equal to the last committed > > > item. I guess we should just use the lastCommittedItem here to avoid doing > > > unnecessary logic in GetVisibleItem. > > Yuke, please address this comment. > Yuke? Sorry, missed this one. Done! https://codereview.chromium.org/2711683002/diff/120001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab.h (right): https://codereview.chromium.org/2711683002/diff/120001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.h:138: @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) On 2017/02/23 02:36:59, Eugene But wrote: > Sorry. Did not notice before. Drop isUsingDesktopUserAgent, it's not correct > name for |usesDesktopUserAgent| property. Hey Kurt, FYI, we drop |isUsingDesktopUserAgent| because it's incorrect as using v.s. uses, which would break KVC. https://codereview.chromium.org/2711683002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:463: @property(nonatomic, readonly, getter=isUsingDesktopUserAgent) On 2017/02/23 02:36:59, Eugene But wrote: > Same comment here. Done. https://codereview.chromium.org/2711683002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2344: web::NavigationItem* visibleItem = On 2017/02/23 02:36:59, Eugene But wrote: > nit: How about this?: > > if (!self.navigationManagerImpl) > return NO; > > auto visibleItem = self.navigationManagerImpl->GetVisibleItem(); > return visibleItem && visibleItem->IsOverridingUserAgent(); Done.
lgtm with changes. Thank you for your patience. https://codereview.chromium.org/2711683002/diff/160001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2711683002/diff/160001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.mm:1571: if (![self navigationManager]) nit: self.navigationManager because it's a property https://codereview.chromium.org/2711683002/diff/160001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.mm:1574: web::NavigationItem* visibleItem = [self navigationManager]->GetVisibleItem(); ditto https://codereview.chromium.org/2711683002/diff/160001/ios/web/navigation/crw... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2711683002/diff/160001/ios/web/navigation/crw... ios/web/navigation/crw_session_controller.mm:499: lastCommittedItem->GetURL(), URL)); web::NavigationItem* lastCommittedItem = self.lastCommittedItem; https://codereview.chromium.org/2711683002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:462: // Whether or not desktop user agent is used for the currently visible page. s/currently visible page/visibleItem Because currently visible page is represented by lastCommittedItem.
Thank you all for the patience in reviewing the code! https://codereview.chromium.org/2711683002/diff/160001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2711683002/diff/160001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.mm:1571: if (![self navigationManager]) On 2017/02/23 18:00:11, Eugene But wrote: > nit: self.navigationManager because it's a property Done. https://codereview.chromium.org/2711683002/diff/160001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.mm:1574: web::NavigationItem* visibleItem = [self navigationManager]->GetVisibleItem(); On 2017/02/23 18:00:12, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2711683002/diff/160001/ios/web/navigation/crw... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2711683002/diff/160001/ios/web/navigation/crw... ios/web/navigation/crw_session_controller.mm:499: lastCommittedItem->GetURL(), URL)); On 2017/02/23 18:00:12, Eugene But wrote: > web::NavigationItem* lastCommittedItem = self.lastCommittedItem; Done. https://codereview.chromium.org/2711683002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2711683002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:462: // Whether or not desktop user agent is used for the currently visible page. On 2017/02/23 18:00:12, Eugene But wrote: > s/currently visible page/visibleItem > > Because currently visible page is represented by lastCommittedItem. Done.
The CQ bit was checked by liaoyuke@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: This issue passed the CQ dry run.
The CQ bit was checked by liaoyuke@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/2711683002/#ps180001 (title: "Change to call property method using dot")
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": 180001, "attempt_start_ts": 1487876063198290,
"parent_rev": "d6aa7632954e8cf05354b270a63973e3483e1afb", "commit_rev":
"21359189a7883d38a531e8681cc250bdf301fa85"}
Message was sent while issue was closed.
Description was changed from ========== (Set)IsOverridingUserAgent should be called on VisibleItem As explained in the associated bug, it always makes sense to call IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item instead of current item. Thus this CL changes CurrentItem to VisibleItem wherever necessary. Besides, this CL also does the following refactoring to have NavigationManagerImpl encapsulate VisibleItem for calling (Set)IsOverridingUserAgent() to provide one more layer of abstraction. BUG=693794 ========== to ========== (Set)IsOverridingUserAgent should be called on VisibleItem As explained in the associated bug, it always makes sense to call IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item instead of current item. Thus this CL changes CurrentItem to VisibleItem wherever necessary. Besides, this CL also does the following refactoring to have NavigationManagerImpl encapsulate VisibleItem for calling (Set)IsOverridingUserAgent() to provide one more layer of abstraction. BUG=693794 Review-Url: https://codereview.chromium.org/2711683002 Cr-Commit-Position: refs/heads/master@{#452570} Committed: https://chromium.googlesource.com/chromium/src/+/21359189a7883d38a531e8681cc2... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/21359189a7883d38a531e8681cc2...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:180001) has been created in https://codereview.chromium.org/2711733007/ by gchatz@chromium.org. The reason for reverting is: Causes ToolsPopupMenuTestCase.testToolsMenuRequestDesktopNetwork to fail with error: ../../ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:109: error: -[ToolsPopupMenuTestCase testToolsMenuRequestDesktopNetwork] : Exception: NoMatchingElementException. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
