Created web::UserAgentType.

Created web::UserAgentType.

Previously, NavigationItems contained a bool denoting whether a
desktop user agent is used.  However, the user agent as a concept does
not apply to app-specific URLs, which are not loaded using HTTP
requests.  This CL introduces a UserAgentType enum, which contains the
values NONE, MOBILE, and DESKTOP in order to differentiate between
app-specific navigations and those using mobile/desktop user agents.

This should assist with logic pertaining to whether to expose the
option to use the desktop/mobile user agent, as app-specific URLs are
handled by the enum type.

BUG=none

Review-Url: https://codereview.chromium.org/2705293014
Cr-Commit-Position: refs/heads/master@{#454426}
Committed: https://chromium.googlesource.com/chromium/src/+/d79e11f6225265fcf1d5884b969796a0377dee8a

[kkhorimoto, 2017-02-25 00:57:02]

kkhorimoto@chromium.org changed reviewers:
+ eugenebut@chromium.org

[liaoyuke, 2017-02-25 01:28:18]

liaoyuke@chromium.org changed reviewers:
+ liaoyuke@chromium.org

[liaoyuke, 2017-02-25 01:28:19]

Drive by. Thank you for creating the UserAgentType!

https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/chrome_web_client.mm (right):

https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/chrome_web_client.mm:115: return std::string();
Do we really need this return statement?

https://codereview.chromium.org/2705293014/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:190: if (pending_item) {
Can we change this to:

if (!pending_item)
    return;

to decrease the depth of nested if statements?

https://codereview.chromium.org/2705293014/diff/20001/ios/web/public/user_age...
File ios/web/public/user_agent.h (right):

https://codereview.chromium.org/2705293014/diff/20001/ios/web/public/user_age...
ios/web/public/user_agent.h:17: // The default user agent type.  Used to specify
a mobile browser user agent.
Is it intentional to have multiple white spaces between "." and "Used"?

https://codereview.chromium.org/2705293014/diff/20001/ios/web/public/user_age...
ios/web/public/user_agent.h:20: // Used when requesting the desktop version of a
page.
Can we make the comments consistent between MOBILE and DESKTOP?

Maybe change this to // Used to specify a desktop browser user agent?

[kkhorimoto, 2017-02-25 01:35:05]

https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/chrome_web_client.mm (right):

https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/chrome_web_client.mm:115: return std::string();
On 2017/02/25 01:28:19, liaoyuke wrote:
> Do we really need this return statement?

Yes, NOTREACHED() does nothing in non-debug builds, so we still need a return
statement in order for it to compile.

https://codereview.chromium.org/2705293014/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:190: if (pending_item) {
On 2017/02/25 01:28:19, liaoyuke wrote:
> Can we change this to:
> 
> if (!pending_item)
>     return;
> 
> to decrease the depth of nested if statements?

Done.

https://codereview.chromium.org/2705293014/diff/20001/ios/web/public/user_age...
File ios/web/public/user_agent.h (right):

https://codereview.chromium.org/2705293014/diff/20001/ios/web/public/user_age...
ios/web/public/user_agent.h:17: // The default user agent type.  Used to specify
a mobile browser user agent.
On 2017/02/25 01:28:19, liaoyuke wrote:
> Is it intentional to have multiple white spaces between "." and "Used"?

Yes.  It's a common practice to increase readability in monospaced fonts,
although not enforced by the style guide or anything.

https://codereview.chromium.org/2705293014/diff/20001/ios/web/public/user_age...
ios/web/public/user_agent.h:20: // Used when requesting the desktop version of a
page.
On 2017/02/25 01:28:19, liaoyuke wrote:
> Can we make the comments consistent between MOBILE and DESKTOP?
> 
> Maybe change this to // Used to specify a desktop browser user agent?

Done.

[liaoyuke, 2017-02-25 01:40:56]

https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/chrome_web_client.mm (right):

https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/chrome_web_client.mm:115: return std::string();
On 2017/02/25 01:35:05, kkhorimoto_ wrote:
> On 2017/02/25 01:28:19, liaoyuke wrote:
> > Do we really need this return statement?
> 
> Yes, NOTREACHED() does nothing in non-debug builds, so we still need a return
> statement in order for it to compile.

I think it will compile even without this return, because this function always
returns something at the end. Could you please double-check?

[liaoyuke, 2017-02-25 01:44:29]

Or should we change this to a DCHECK_NEQ(type, web::UserAgentType::NONE)?

On Fri, Feb 24, 2017 at 5:40 PM <liaoyuke@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
> File ios/chrome/browser/web/chrome_web_client.mm (right):
>
>
>
https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
> ios/chrome/browser/web/chrome_web_client.mm:115: return std::string();
> On 2017/02/25 01:35:05, kkhorimoto_ wrote:
> > On 2017/02/25 01:28:19, liaoyuke wrote:
> > > Do we really need this return statement?
> >
> > Yes, NOTREACHED() does nothing in non-debug builds, so we still need a
> return
> > statement in order for it to compile.
>
> I think it will compile even without this return, because this function
> always returns something at the end. Could you please double-check?
>
> https://codereview.chromium.org/2705293014/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Eugene But (OOO till 7-30), 2017-02-25 01:51:52]

Thank you for doing this!

https://codereview.chromium.org/2705293014/diff/40001/ios/chrome/browser/web/...
File ios/chrome/browser/web/chrome_web_client.mm (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/chrome_web_client.mm:115: return std::string();
DCHECK_NE(type, web::UserAgentType::NONE);

See Chromium Style Guide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2705293014/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/chrome_web_client.mm:133: return
web::BuildUserAgentFromProduct(product);
Optional nit: Since you touching this.
return web::BuildUserAgentFromProduct(GetProduct());

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:204:
last_web_item->GetUserAgentType() == UserAgentType::DESKTOP);
How about adding one more local variable for "(last_web_item &&
last_web_item->GetUserAgentType() == UserAgentType::DESKTOP)"?

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:443: NavigationItem*
NavigationManagerImpl::GetLastCommittedWebItem() const {
From this name I would think that it returns the last http or https Item (see
f.e. UrlHasWebScheme). How about GetLastCommittedNonAppSpecificItem?

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:446: if (index < 0 || index >=
static_cast<int>(items.size()))
Can index even be larger than size?

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:449: while (index >= 0) {
Do you want to use for-loop here?:

WebClient* client = GetWebClient();
NavigationItemList items = [session_controller_ items];
for (int i = GetCurrentItemIndex(); 0 <= i; i--) {
  if (!client->IsAppSpecificURL(items[index]->GetVirtualURL()))
    return item;
}
return nullptr;

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:520: 
There was quite a few logic changes in the code. Could you please write tests to
cover those and a new method.

https://codereview.chromium.org/2705293014/diff/40001/ios/web/public/crw_navi...
File ios/web/public/crw_navigation_item_storage.h (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/public/crw_navi...
ios/web/public/crw_navigation_item_storage.h:49: extern NSString* const
kNavigationItemStorageUseDesktopUserAgentDeprecatedKey;
Do you want to add TODO for deprecating this key?

https://codereview.chromium.org/2705293014/diff/40001/ios/web/public/user_age...
File ios/web/public/user_agent.h (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/public/user_age...
ios/web/public/user_agent.h:25: std::string
GetUserAgentTypeDescription(UserAgentType type);
Could you please add tests for these.

EXPECT_EQ(NONE,
GetUserAgentTypeWithDescription(GetUserAgentTypeDescription(NONE)))
etc...

https://codereview.chromium.org/2705293014/diff/40001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:2406: if (item->GetUserAgentType() !=
fromItem->GetUserAgentType())
If new UA is NONE, then page reconstruction is unnecessary, right?

[kkhorimoto, 2017-02-25 01:52:09]

https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/chrome_web_client.mm (right):

https://codereview.chromium.org/2705293014/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/chrome_web_client.mm:115: return std::string();
On 2017/02/25 01:40:55, liaoyuke wrote:
> On 2017/02/25 01:35:05, kkhorimoto_ wrote:
> > On 2017/02/25 01:28:19, liaoyuke wrote:
> > > Do we really need this return statement?
> > 
> > Yes, NOTREACHED() does nothing in non-debug builds, so we still need a
return
> > statement in order for it to compile.
> 
> I think it will compile even without this return, because this function always
> returns something at the end. Could you please double-check?

It will compile, but it will return the wrong value.  If we allowed it to keep
going without an early return, the function will return the UA for
UserAgentType::MOBILE, which is not what was requested for the input parameter.

[kkhorimoto, 2017-02-27 23:29:04]

https://codereview.chromium.org/2705293014/diff/40001/ios/chrome/browser/web/...
File ios/chrome/browser/web/chrome_web_client.mm (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/chrome_web_client.mm:115: return std::string();
On 2017/02/25 01:51:52, Eugene But wrote:
> DCHECK_NE(type, web::UserAgentType::NONE);
> 
> See Chromium Style Guide:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 

Hmm, I had not seen those guidelines before.  I guess since there isn't any harm
in returning an inaccurate UA string here, I'll just use the DCHECK.

https://codereview.chromium.org/2705293014/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/chrome_web_client.mm:133: return
web::BuildUserAgentFromProduct(product);
On 2017/02/25 01:51:52, Eugene But wrote:
> Optional nit: Since you touching this.
> return web::BuildUserAgentFromProduct(GetProduct());

Done.

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:204:
last_web_item->GetUserAgentType() == UserAgentType::DESKTOP);
On 2017/02/25 01:51:52, Eugene But wrote:
> How about adding one more local variable for "(last_web_item &&
> last_web_item->GetUserAgentType() == UserAgentType::DESKTOP)"?

Done.

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:443: NavigationItem*
NavigationManagerImpl::GetLastCommittedWebItem() const {
On 2017/02/25 01:51:52, Eugene But wrote:
> From this name I would think that it returns the last http or https Item (see
> f.e. UrlHasWebScheme). How about GetLastCommittedNonAppSpecificItem?

Done.

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:446: if (index < 0 || index >=
static_cast<int>(items.size()))
On 2017/02/25 01:51:52, Eugene But wrote:
> Can index even be larger than size?

No, it would have DCHECKED when we set it to an invalid value, so I'm leaving it
out of this check.

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:449: while (index >= 0) {
On 2017/02/25 01:51:52, Eugene But wrote:
> Do you want to use for-loop here?:
> 
> WebClient* client = GetWebClient();
> NavigationItemList items = [session_controller_ items];
> for (int i = GetCurrentItemIndex(); 0 <= i; i--) {
>   if (!client->IsAppSpecificURL(items[index]->GetVirtualURL()))
>     return item;
> }
> return nullptr;

Since we already fetch the current index when performing the check above, I
didn't want to fetch it again to initialize the for loop.  Writing for loops
with an empty initialization section seems hacky to me, so I'm leaving it as a
while loop.

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:520: 
On 2017/02/25 01:51:52, Eugene But wrote:
> There was quite a few logic changes in the code. Could you please write tests
to
> cover those and a new method.

Done.

https://codereview.chromium.org/2705293014/diff/40001/ios/web/public/crw_navi...
File ios/web/public/crw_navigation_item_storage.h (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/public/crw_navi...
ios/web/public/crw_navigation_item_storage.h:49: extern NSString* const
kNavigationItemStorageUseDesktopUserAgentDeprecatedKey;
On 2017/02/25 01:51:52, Eugene But wrote:
> Do you want to add TODO for deprecating this key?

Done.

https://codereview.chromium.org/2705293014/diff/40001/ios/web/public/user_age...
File ios/web/public/user_agent.h (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/public/user_age...
ios/web/public/user_agent.h:25: std::string
GetUserAgentTypeDescription(UserAgentType type);
On 2017/02/25 01:51:52, Eugene But wrote:
> Could you please add tests for these.
> 
> EXPECT_EQ(NONE,
> GetUserAgentTypeWithDescription(GetUserAgentTypeDescription(NONE)))
> etc...

Done.

https://codereview.chromium.org/2705293014/diff/40001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:2406: if (item->GetUserAgentType() !=
fromItem->GetUserAgentType())
On 2017/02/25 01:51:52, Eugene But wrote:
> If new UA is NONE, then page reconstruction is unnecessary, right?

Done.

[Eugene But (OOO till 7-30), 2017-02-27 23:55:19]

https://codereview.chromium.org/2705293014/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl.mm:212: url_ = original_request_url_;
Why? The old code and NavigationEntryImpl is not doing this. on iOS we should
aim for having similar interface to content.

https://codereview.chromium.org/2705293014/diff/80001/ios/web/public/navigati...
File ios/web/public/navigation_item.h (right):

https://codereview.chromium.org/2705293014/diff/80001/ios/web/public/navigati...
ios/web/public/navigation_item.h:118: virtual void
SetUserAgentType(UserAgentType type) = 0;
Sorry, only now I realized that content has
SetIsOverridingUserAgent/GetIsOverridingUserAgent. The goal for ios/web is to be
similar to content where possible and ideally at some point we want to have the
same interfaces. This change is a move to the opposite direction. Can you talk
to content folks and ask if they like your changes? If so then we should change
both, content and web.

[liaoyuke, 2017-03-01 01:33:56]

https://codereview.chromium.org/2705293014/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl.mm:212: url_ = original_request_url_;
On 2017/02/27 23:55:18, Eugene But wrote:
> Why? The old code and NavigationEntryImpl is not doing this. on iOS we should
> aim for having similar interface to content.

Actually, now I think this is bad. We may justify that it's OK to add side
effects to the setUserAgentType function in tools_menu_view_controller, but
definitely not here, if you take a look at other getters and setters in this
file, they are pretty clean, and only does one thing that the function name
implies.

By the way, even if we are doing this, shouldn't url_ be set as the original url
of last user item instead of the original url of visible item?

[kkhorimoto, 2017-03-01 18:29:25]

https://codereview.chromium.org/2705293014/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl.mm:212: url_ = original_request_url_;
On 2017/03/01 01:33:56, liaoyuke wrote:
> On 2017/02/27 23:55:18, Eugene But wrote:
> > Why? The old code and NavigationEntryImpl is not doing this. on iOS we
should
> > aim for having similar interface to content.
> 
> Actually, now I think this is bad. We may justify that it's OK to add side
> effects to the setUserAgentType function in tools_menu_view_controller, but
> definitely not here, if you take a look at other getters and setters in this
> file, they are pretty clean, and only does one thing that the function name
> implies.
> 
> By the way, even if we are doing this, shouldn't url_ be set as the original
url
> of last user item instead of the original url of visible item?

I've reverted this change, instead only setting |user_agent_type_| and doing the
DCHECK below.  The motivation for adding this was to ensure that changing the UA
type would erase details of redirection due to UA sniffing (i.e. www.page.com =>
m.page.com), and would force users of this NavigationItem to request the correct
URL when retrying the load.  When the UA reload code is rewritten by Yuke, we'll
need to ensure that |original_request_url_| is called for the load instead of
the virtual URL.

https://codereview.chromium.org/2705293014/diff/80001/ios/web/public/navigati...
File ios/web/public/navigation_item.h (right):

https://codereview.chromium.org/2705293014/diff/80001/ios/web/public/navigati...
ios/web/public/navigation_item.h:118: virtual void
SetUserAgentType(UserAgentType type) = 0;
On 2017/02/27 23:55:18, Eugene But wrote:
> Sorry, only now I realized that content has
> SetIsOverridingUserAgent/GetIsOverridingUserAgent. The goal for ios/web is to
be
> similar to content where possible and ideally at some point we want to have
the
> same interfaces. This change is a move to the opposite direction. Can you talk
> to content folks and ask if they like your changes? If so then we should
change
> both, content and web.

I've added a TODO to introduce the same change to content// since it's a bit out
of scope for this CL.  Landing this first will unblock Yuke's UA refactoring.

[kkhorimoto, 2017-03-02 02:45:40]

PTAL so we can unblock Yuke.  I've added a TODO for copying implementation to
content//

[Eugene But (OOO till 7-30), 2017-03-02 03:36:34]

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:449: while (index >= 0) {
On 2017/02/27 23:29:04, kkhorimoto_ wrote:
> On 2017/02/25 01:51:52, Eugene But wrote:
> > Do you want to use for-loop here?:
> > 
> > WebClient* client = GetWebClient();
> > NavigationItemList items = [session_controller_ items];
> > for (int i = GetCurrentItemIndex(); 0 <= i; i--) {
> >   if (!client->IsAppSpecificURL(items[index]->GetVirtualURL()))
> >     return item;
> > }
> > return nullptr;
> 
> Since we already fetch the current index when performing the check above, I
> didn't want to fetch it again to initialize the for loop.  Writing for loops
> with an empty initialization section seems hacky to me, so I'm leaving it as a
> while loop.
You actually don't need that check above. You can keep it your way, but this
while loop looks confusing. This code has initialization, condition and
increment, and for loop puts a structure to it.

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl.mm:199: if
(pending_item->GetUserAgentType() != UserAgentType::NONE) {
What if |pending_item->GetUserAgentType()| has DESKTOP UA? Do we need to do
anything?

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl.mm:207:
override_desktop_user_agent_for_next_pending_item_ ||
Can you restructure the code, so if
|override_desktop_user_agent_for_next_pending_item_| is true, then you don't
look up for GetLastCommittedNonAppSpecificItem?

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:523: // Test that the
first item's UA type is propagated to the second.
Do you want to use linebreaks for the code to separate logical pieces?
Linebreaks and more comments could improve the readability. F.e.
addItem1
commitItem1

verifyitem1

additem2
commitItem2

verifyitem2

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:560:
GURL("testnativecontent://native1"), Referrer(),
Please use kTestNativeContentScheme instead of hardcoding the scheme

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:563: web::NavigationItem*
nativeItem1 =
native_item1

[kkhorimoto, 2017-03-02 04:12:55]

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl.mm:199: if
(pending_item->GetUserAgentType() != UserAgentType::NONE) {
On 2017/03/02 03:36:34, Eugene But wrote:
> What if |pending_item->GetUserAgentType()| has DESKTOP UA? Do we need to do
> anything?

pending_item->GetUserAgentType() will never be DESKTOP here.  It'll be either
MOBILE (default) or NONE (native pages).   I've added a comment here
elaborating; it'll be clearer once CRWSessionController's addPendingItem code
gets folded into NavigationManagerImpl.

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl.mm:207:
override_desktop_user_agent_for_next_pending_item_ ||
On 2017/03/02 03:36:34, Eugene But wrote:
> Can you restructure the code, so if
> |override_desktop_user_agent_for_next_pending_item_| is true, then you don't
> look up for GetLastCommittedNonAppSpecificItem?

Done.

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:523: // Test that the
first item's UA type is propagated to the second.
On 2017/03/02 03:36:34, Eugene But wrote:
> Do you want to use linebreaks for the code to separate logical pieces?
> Linebreaks and more comments could improve the readability. F.e.
> addItem1
> commitItem1
> 
> verifyitem1
> 
> additem2
> commitItem2
> 
> verifyitem2

Done.

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:560:
GURL("testnativecontent://native1"), Referrer(),
On 2017/03/02 03:36:34, Eugene But wrote:
> Please use kTestNativeContentScheme instead of hardcoding the scheme

Done.

https://codereview.chromium.org/2705293014/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:563: web::NavigationItem*
nativeItem1 =
On 2017/03/02 03:36:34, Eugene But wrote:
> native_item1

Done.

[Eugene But (OOO till 7-30), 2017-03-02 06:44:06]

Thanks! lgtm

https://codereview.chromium.org/2705293014/diff/120001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/120001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl.mm:196: // UserAgentType::DESKTOP.
Would it make sense to DCHECK_NE(ending_item->GetUserAgentType(),
UserAgentType::DESKTOP) ?

[liaoyuke, 2017-03-02 19:08:09]

On 2017/03/02 06:44:06, Eugene But wrote:
> Thanks! lgtm
> 
>
https://codereview.chromium.org/2705293014/diff/120001/ios/web/navigation/nav...
> File ios/web/navigation/navigation_manager_impl.mm (right):
> 
>
https://codereview.chromium.org/2705293014/diff/120001/ios/web/navigation/nav...
> ios/web/navigation/navigation_manager_impl.mm:196: // UserAgentType::DESKTOP.
> Would it make sense to DCHECK_NE(ending_item->GetUserAgentType(),
> UserAgentType::DESKTOP) ?

Thank you again for doing this Kurt! lgtm

[kkhorimoto, 2017-03-02 19:35:18]

https://codereview.chromium.org/2705293014/diff/120001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2705293014/diff/120001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl.mm:196: // UserAgentType::DESKTOP.
On 2017/03/02 06:44:06, Eugene But wrote:
> Would it make sense to DCHECK_NE(ending_item->GetUserAgentType(),
> UserAgentType::DESKTOP) ?

Done.

[kkhorimoto, 2017-03-02 19:35:23]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-03-02 19:35:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
liaoyuke@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2705293014/#ps140001
(title: "Added DCHECK")

[commit-bot: I haz the power, 2017-03-02 19:35:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 19:51:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 19:51:42]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[kkhorimoto, 2017-03-02 21:49:02]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-03-02 21:49:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
liaoyuke@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2705293014/#ps160001
(title: "rebase + compile fix")

[commit-bot: I haz the power, 2017-03-02 21:49:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 23:20:14]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488491342536910,
"parent_rev": "fcf6850e3c9e5bb732a6925605d03b5644bd9534", "commit_rev":
"d79e11f6225265fcf1d5884b969796a0377dee8a"}

[commit-bot: I haz the power, 2017-03-02 23:21:01]

Description was changed from

==========
Created web::UserAgentType.

Previously, NavigationItems contained a bool denoting whether a
desktop user agent is used.  However, the user agent as a concept does
not apply to app-specific URLs, which are not loaded using HTTP
requests.  This CL introduces a UserAgentType enum, which contains the
values NONE, MOBILE, and DESKTOP in order to differentiate between
app-specific navigations and those using mobile/desktop user agents.

This should assist with logic pertaining to whether to expose the
option to use the desktop/mobile user agent, as app-specific URLs are
handled by the enum type.

BUG=none
==========

to

==========
Created web::UserAgentType.

Previously, NavigationItems contained a bool denoting whether a
desktop user agent is used.  However, the user agent as a concept does
not apply to app-specific URLs, which are not loaded using HTTP
requests.  This CL introduces a UserAgentType enum, which contains the
values NONE, MOBILE, and DESKTOP in order to differentiate between
app-specific navigations and those using mobile/desktop user agents.

This should assist with logic pertaining to whether to expose the
option to use the desktop/mobile user agent, as app-specific URLs are
handled by the enum type.

BUG=none

Review-Url: https://codereview.chromium.org/2705293014
Cr-Commit-Position: refs/heads/master@{#454426}
Committed:
https://chromium.googlesource.com/chromium/src/+/d79e11f6225265fcf1d5884b9697...
==========

[commit-bot: I haz the power, 2017-03-02 23:21:04]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/d79e11f6225265fcf1d5884b9697...