(Set)IsOverridingUserAgent should be called on VisibleItem

ios/web/navigation/navigation_manager_impl.mm

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/web/navigation/navigation_manager_impl.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 402 matching lines @@
     // Result may be out of bounds, so stop trying to skip redirect items and
     // simply add the remainder.
     result += offset;
     if (result < 0 /* overflow */)
       result = INT_MAX;
   }
 
   return result;
 }
 
+bool NavigationManagerImpl::IsUsingDesktopUserAgent() const {
+  return GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent();

[Eugene But (OOO till 7-30), 2017/02/22 21:57:42]

Why Visible? Should this be LastCommittedItem? According to the comments this
represents "currently loaded page", which is LastCommittedItem, not visible
item.

[kkhorimoto, 2017/02/22 22:54:15]

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.

[Eugene But (OOO till 7-30), 2017/02/22 23:13:56]

Makes sense. Yuke, please update header comments to reflect what method does.

[liaoyuke, 2017/02/22 23:48:31]

Done.

[liaoyuke, 2017/02/22 23:48:31]

Acknowledged.

[liaoyuke, 2017/02/22 23:48:31]

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.

[Eugene But (OOO till 7-30), 2017/02/23 00:03:13]

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.

+}
+
 void NavigationManagerImpl::OverrideDesktopUserAgentForNextPendingItem() {
   if (GetPendingItem())
     GetPendingItem()->SetIsOverridingUserAgent(true);
   else
     override_desktop_user_agent_for_next_pending_item_ = true;
 }
 
 bool NavigationManagerImpl::IsRedirectItemAtIndex(int index) const {
   DCHECK_GT(index, 0);
   DCHECK_LT(index, GetItemCount());
   ui::PageTransition transition = GetItemAtIndex(index)->GetTransitionType();
   return transition & ui::PAGE_TRANSITION_IS_REDIRECT_MASK;
 }
 
 }  // namespace web