implement user agent override option.

implement user agent override option.

This CL implements the UserAgentOverrideOption enum class in 
NavigationManager to replace the 
override_desktop_user_agent_for_next_pending_item_ flag, and also added
corresponding unit tests to verify that UserAgentOverrideOption is 
working as intended.

BUG=692303
Review-Url: https://codereview.chromium.org/2779383002
Cr-Commit-Position: refs/heads/master@{#460918}
Committed: https://chromium.googlesource.com/chromium/src/+/60323989ade77aa658a4e5b4974b6673b14fc399

[liaoyuke, 2017-03-29 23:40:40]

Description was changed from

==========
implement user agent override option.

This CL implements the UserAgentOverrideOption enum class in NavigationManager
to replace the override... flag, and added corresponding unit tests to verify
that UserAgentOverrideOption is working as intended.

BUG=
==========

to

==========
implement user agent override option.

This CL implements the UserAgentOverrideOption enum class in 
NavigationManager to replace the 
override_desktop_user_agent_for_next_pending_item_ flag, and also added
corresponding unit tests to verify that UserAgentOverrideOption is 
working as intended.

BUG=
==========

[liaoyuke, 2017-03-29 23:57:05]

Patchset #2 (id:20001) has been deleted

[liaoyuke, 2017-03-29 23:57:08]

Patchset #1 (id:1) has been deleted

[liaoyuke, 2017-03-29 23:59:29]

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

[liaoyuke, 2017-03-30 00:01:05]

Hey Eugene, Kurt,

PTAL.

FYI: this CL is independent from the transient item one.

Take your time. Thank you very much!

[kkhorimoto, 2017-03-30 02:30:28]

The general approach looks good, but I'm confused about how this fits together
with our goal with this feature.  It looks like we're still creating a new
NavigationItem when reloading for the different user agent type, so switching UA
type from MOBILE => DESKTOP => MOBILE will result in three NavigationItems with
those UA types in the session history.  Is this what's intended?  Are we no
longer calling NavigationManager::Reload() with the ORIGINAL_REQUEST_URL?

https://codereview.chromium.org/2779383002/diff/40001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.h (left):

https://codereview.chromium.org/2779383002/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:177: bool
override_desktop_user_agent_for_next_pending_item_;
Yay, I'm happy to see this go away :)

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

https://codereview.chromium.org/2779383002/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:203: if
(user_agent_override_option == UserAgentOverrideOption::DESKTOP) {
NIT: Should we just use a switch statement here?

[liaoyuke, 2017-03-30 02:43:55]

Description was changed from

==========
implement user agent override option.

This CL implements the UserAgentOverrideOption enum class in 
NavigationManager to replace the 
override_desktop_user_agent_for_next_pending_item_ flag, and also added
corresponding unit tests to verify that UserAgentOverrideOption is 
working as intended.

BUG=
==========

to

==========
implement user agent override option.

This CL implements the UserAgentOverrideOption enum class in 
NavigationManager to replace the 
override_desktop_user_agent_for_next_pending_item_ flag, and also added
corresponding unit tests to verify that UserAgentOverrideOption is 
working as intended.

BUG=692303
==========

[liaoyuke, 2017-03-30 02:51:24]

On 2017/03/30 02:30:28, kkhorimoto_ wrote:
> The general approach looks good, but I'm confused about how this fits together
> with our goal with this feature.  It looks like we're still creating a new
> NavigationItem when reloading for the different user agent type, so switching
UA
> type from MOBILE => DESKTOP => MOBILE will result in three NavigationItems
with
> those UA types in the session history.  Is this what's intended?  Are we no
> longer calling NavigationManager::Reload() with the ORIGINAL_REQUEST_URL?
> 
>
https://codereview.chromium.org/2779383002/diff/40001/ios/web/navigation/navi...
> File ios/web/navigation/navigation_manager_impl.h (left):
> 
>
https://codereview.chromium.org/2779383002/diff/40001/ios/web/navigation/navi...
> ios/web/navigation/navigation_manager_impl.h:177: bool
> override_desktop_user_agent_for_next_pending_item_;
> Yay, I'm happy to see this go away :)
> 
>
https://codereview.chromium.org/2779383002/diff/40001/ios/web/navigation/navi...
> File ios/web/navigation/navigation_manager_impl.mm (right):
> 
>
https://codereview.chromium.org/2779383002/diff/40001/ios/web/navigation/navi...
> ios/web/navigation/navigation_manager_impl.mm:203: if
> (user_agent_override_option == UserAgentOverrideOption::DESKTOP) {
> NIT: Should we just use a switch statement here?

Sorry for the confusion. This step is actually part of our end goal. 

We wanted to avoid creating new navigation items, which means that we should
also get rid of override_desktop_user_agent_for_next_pending_item_, but getting
rid of  override_desktop_user_agent_for_next_pending_item_ directly would break
pre-load, so this CL focus on removing
override_desktop_user_agent_for_next_pending_item_ and implement user agent
override option to fix pre-load. Then, in my next CL, I will switch from
|LoadURLWithParams| to |Reload|.

Does it make sense?

[kkhorimoto, 2017-03-30 19:04:02]

Yep, that clears it up for me, thanks! lgtm

[Eugene But (OOO till 7-30), 2017-03-30 19:21:25]

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

https://codereview.chromium.org/2779383002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:222: default:
There is no need for default, which prevents compiler warning about unhandled
cases

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

https://codereview.chromium.org/2779383002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:584: 
Do you want to add2 more tests:
1.) Adds INHERIT after MOBILE
2.) Adds INHERIT after DESKTOP

https://codereview.chromium.org/2779383002/diff/60001/ios/web/public/navigati...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2779383002/diff/60001/ios/web/public/navigati...
ios/web/public/navigation_manager.h:35: // Inherit the user agent type from the
last committed non-app specific item
What if there are no committed items?

https://codereview.chromium.org/2779383002/diff/60001/ios/web/public/navigati...
ios/web/public/navigation_manager.h:43: DESKTOP
Optional nit: it's a good practice to have comma after last item. Sp the future
changes will not touch this line.

[liaoyuke, 2017-03-30 19:54:18]

Patchset #3 (id:80001) has been deleted

[liaoyuke, 2017-03-30 19:56:25]

PTAL.

Thank you very much!

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

https://codereview.chromium.org/2779383002/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:203: if
(user_agent_override_option == UserAgentOverrideOption::DESKTOP) {
On 2017/03/30 02:30:28, kkhorimoto_ wrote:
> NIT: Should we just use a switch statement here?

Done.

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

https://codereview.chromium.org/2779383002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:222: default:
On 2017/03/30 19:21:24, Eugene But wrote:
> There is no need for default, which prevents compiler warning about unhandled
> cases

I see, but the original intention of default is to add NOTREACHED() to prevent
surprises, is there a way to keep NOTREACHED() without default?

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

https://codereview.chromium.org/2779383002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:584: 
On 2017/03/30 19:21:24, Eugene But wrote:
> Do you want to add2 more tests:
> 1.) Adds INHERIT after MOBILE
> 2.) Adds INHERIT after DESKTOP

Done.

https://codereview.chromium.org/2779383002/diff/60001/ios/web/public/navigati...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2779383002/diff/60001/ios/web/public/navigati...
ios/web/public/navigation_manager.h:35: // Inherit the user agent type from the
last committed non-app specific item
On 2017/03/30 19:21:24, Eugene But wrote:
> What if there are no committed items?

Done.

https://codereview.chromium.org/2779383002/diff/60001/ios/web/public/navigati...
ios/web/public/navigation_manager.h:43: DESKTOP
On 2017/03/30 19:21:24, Eugene But wrote:
> Optional nit: it's a good practice to have comma after last item. Sp the
future
> changes will not touch this line.

Done.

[Eugene But (OOO till 7-30), 2017-03-30 20:42:31]

lgtm

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

https://codereview.chromium.org/2779383002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:222: default:
On 2017/03/30 19:56:25, liaoyuke wrote:
> On 2017/03/30 19:21:24, Eugene But wrote:
> > There is no need for default, which prevents compiler warning about
unhandled
> > cases
> 
> I see, but the original intention of default is to add NOTREACHED() to prevent
> surprises, is there a way to keep NOTREACHED() without default?
This NOTREACHED will never be executed. NOTREACHED is debug only so it can
happen only on local builds. If a new item is added to UserAgentOverrideOption
and not handled in this switch case then the local build will not even compile.
So there should be no "surprises" :)

[liaoyuke, 2017-03-30 20:50:29]

On 2017/03/30 20:42:31, Eugene But wrote:
> lgtm
> 
>
https://codereview.chromium.org/2779383002/diff/60001/ios/web/navigation/navi...
> File ios/web/navigation/navigation_manager_impl.mm (right):
> 
>
https://codereview.chromium.org/2779383002/diff/60001/ios/web/navigation/navi...
> ios/web/navigation/navigation_manager_impl.mm:222: default:
> On 2017/03/30 19:56:25, liaoyuke wrote:
> > On 2017/03/30 19:21:24, Eugene But wrote:
> > > There is no need for default, which prevents compiler warning about
> unhandled
> > > cases
> > 
> > I see, but the original intention of default is to add NOTREACHED() to
prevent
> > surprises, is there a way to keep NOTREACHED() without default?
> This NOTREACHED will never be executed. NOTREACHED is debug only so it can
> happen only on local builds. If a new item is added to UserAgentOverrideOption
> and not handled in this switch case then the local build will not even
compile.
> So there should be no "surprises" :)

Makes sense! thanks!

[liaoyuke, 2017-03-30 20:52:09]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[liaoyuke, 2017-03-30 20:52:53]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 20:53:38]

Dry run: 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-30 20:58:51]

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

[commit-bot: I haz the power, 2017-03-30 20:58:52]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[liaoyuke, 2017-03-30 21:15:47]

Patchset #4 (id:120001) has been deleted

[liaoyuke, 2017-03-30 21:19:07]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 21:20:21]

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

[liaoyuke, 2017-03-30 21:28:22]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[liaoyuke, 2017-03-30 21:28:57]

Patchset #4 (id:140001) has been deleted

[liaoyuke, 2017-03-30 21:29:18]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 21:29:31]

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

[liaoyuke, 2017-03-30 22:12:47]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 22:13:23]

Dry run: 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-30 22:44:57]

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

[commit-bot: I haz the power, 2017-03-30 22:44:58]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-03-30 22:48:22]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-03-30 22:48:22]

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/2779383002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-30 22:48:58]

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-30 23:13:44]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490914102125060,
"parent_rev": "026a2f4a1f023de9bf72674db9c88cdcf651418e", "commit_rev":
"60323989ade77aa658a4e5b4974b6673b14fc399"}

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

Description was changed from

==========
implement user agent override option.

This CL implements the UserAgentOverrideOption enum class in 
NavigationManager to replace the 
override_desktop_user_agent_for_next_pending_item_ flag, and also added
corresponding unit tests to verify that UserAgentOverrideOption is 
working as intended.

BUG=692303
==========

to

==========
implement user agent override option.

This CL implements the UserAgentOverrideOption enum class in 
NavigationManager to replace the 
override_desktop_user_agent_for_next_pending_item_ flag, and also added
corresponding unit tests to verify that UserAgentOverrideOption is 
working as intended.

BUG=692303

Review-Url: https://codereview.chromium.org/2779383002
Cr-Commit-Position: refs/heads/master@{#460918}
Committed:
https://chromium.googlesource.com/chromium/src/+/60323989ade77aa658a4e5b4974b...
==========

[commit-bot: I haz the power, 2017-03-30 23:14:31]

Committed patchset #4 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/60323989ade77aa658a4e5b4974b...