Implement request mobile site.

Implement request mobile site.

This CL implements the "Request Mobile Site" functionality to enable
switching from the desktop version of a page to mobile version.

BUG=678047
Review-Url: https://codereview.chromium.org/2789433006
Cr-Commit-Position: refs/heads/master@{#464046}
Committed: https://chromium.googlesource.com/chromium/src/+/6ab36201c67153ba617a6a9817e22bc3d303f927

[liaoyuke, 2017-04-07 18:45:33]

Patchset #1 (id:1) has been deleted

[liaoyuke, 2017-04-07 18:45:38]

Patchset #1 (id:20001) has been deleted

[liaoyuke, 2017-04-07 18:47:29]

Description was changed from

==========
implement request mobile site

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

to

==========
Implement request mobile site.

This CL implements the "Request Mobile Site" functionality to enable
switching from the desktop version of a page to mobile version.

BUG=678047
==========

[liaoyuke, 2017-04-07 18:47:29]

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

[liaoyuke, 2017-04-07 19:04:16]

Last CL. TPAL!

Thank you very much!

[liaoyuke, 2017-04-07 19:05:21]

CC Sergio and Rohit who are working on refactoring tools popup menu.

[liaoyuke, 2017-04-10 22:35:53]

PTAL.

Thank you very much!

[Eugene But (OOO till 7-30), 2017-04-11 00:13:40]

On 2017/04/10 22:35:53, liaoyuke wrote:
> PTAL.
> 
> Thank you very much!
Sorry for the delay, I'd like Kurt to do first pass review for this.

[kkhorimoto, 2017-04-11 01:09:42]

lgtm with a couple nits.  sorry for the delay!

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.h (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.h:242: // Removes the web view and loads the
original url of the last non-redirect item
We aren't removing the web view anymore, are we?  Can't we just set the
|customUserAgent| property of the WKWebView?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4238: [[_model currentTab]
reloadWithUserAgentType:userAgentType];
Let's just call this in-line in the switch statement to avoid the clutter of
adding another 1-line function.

[Eugene But (OOO till 7-30), 2017-04-11 02:11:57]

Generally looks good. Have a few minor comments.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.h (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.h:242: // Removes the web view and loads the
original url of the last non-redirect item
In any case Tab should not make any assumptions about implementation details or
even that WKWebView is used.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:1383: lastNonRedirectItem =
navigationManager->GetPendingItem();
Should this be GetVisibleItem instead? Pending url is not necessarily displayed
in the omnibox.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:133: // Tests
requesting desktop site of a page works and the user agent sticks for
This is really great set of tests, do you want to move all UA tests to a
separate file?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:133: // Tests
requesting desktop site of a page works and the user agent sticks for
nit: |Tests that| ?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:134: // next
navigations in the same tab.
nit: |the next navigations|?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:142: [[EarlGrey
selectElementWithMatcher:chrome_test_util::WebViewContainingText(
Do you want to add |using chrome_test_util::WebViewContainingText| to improve
the formatting?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:161: // Tests
requesting desktop site of a page works and desktop user agent doesn't
nit: |Tests the requesting|?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:162: // stick in new
tab.
nit: s/doesn't stick in new tab/does not propagate to the new tab ?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:190: // Tests
requesting desktop site of a page works and going back re-opens mobile
|Tests that requesting|

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:307: [ChromeEarlGrey
loadURL:GURL("chrome://version")];
chrome://version is not native but WebUI. Do you want to rename the test?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:320: // Tests
requesting desktop site works correctly for script-based desktop/mobile
What is "script-based desktop/mobile differentation."? Also how is this test
different from the previous tests?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:339: // Tests
requesting mobile site works correctly for script-based desktop/mobile
Same questions here.

[sczs, 2017-04-11 02:26:55]

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

[sczs, 2017-04-11 02:26:55]

Just a quick comment, when trying to understand the new feature I realized that
ToolsMenuContext was renamed to ToolsMenuConfiguration on a previous CL but the
Header guards were not updated.

Other than that the ToolsMenu cleaning LGTM

[Eugene But (OOO till 7-30), 2017-04-11 14:41:39]

On 2017/04/11 02:26:55, sczs wrote:
> Just a quick comment, when trying to understand the new feature I realized
that
> ToolsMenuContext was renamed to ToolsMenuConfiguration on a previous CL but
the
> Header guards were not updated.
Thanks for catching this! Yuke, you can use tools/git/move_source_file.py next
time, which moves the file, updates GN and renames guards.

[liaoyuke, 2017-04-11 17:25:07]

On 2017/04/11 14:41:39, Eugene But wrote:
> On 2017/04/11 02:26:55, sczs wrote:
> > Just a quick comment, when trying to understand the new feature I realized
> that
> > ToolsMenuContext was renamed to ToolsMenuConfiguration on a previous CL but
> the
> > Header guards were not updated.
> Thanks for catching this! Yuke, you can use tools/git/move_source_file.py next
> time, which moves the file, updates GN and renames guards.

Thanks for catching it Sergio! Thanks Eugene, it's good to know there is such a
tool.

[liaoyuke, 2017-04-11 18:23:47]

Patchset #3 (id:80001) has been deleted

[liaoyuke, 2017-04-11 18:27:21]

Thanks for catching the header Sergio! Thanks Eugene, it's also good to know
there is such a tool :)

PTAL!

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.h (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.h:242: // Removes the web view and loads the
original url of the last non-redirect item
On 2017/04/11 01:09:41, kkhorimoto_ wrote:
> We aren't removing the web view anymore, are we?  Can't we just set the
> |customUserAgent| property of the WKWebView?

Set |customUserAgent| property requires re-factoring a few files to use
UserAgentType, so I wanted to save it to a separate CL.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.h:242: // Removes the web view and loads the
original url of the last non-redirect item
On 2017/04/11 02:11:56, Eugene But wrote:
> In any case Tab should not make any assumptions about implementation details
or
> even that WKWebView is used.

That is a very good point! Thanks!

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:1383: lastNonRedirectItem =
navigationManager->GetPendingItem();
On 2017/04/11 02:11:56, Eugene But wrote:
> Should this be GetVisibleItem instead? Pending url is not necessarily
displayed
> in the omnibox.

Good catch!

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4238: [[_model currentTab]
reloadWithUserAgentType:userAgentType];
On 2017/04/11 01:09:42, kkhorimoto_ wrote:
> Let's just call this in-line in the switch statement to avoid the clutter of
> adding another 1-line function.

Done.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:133: // Tests
requesting desktop site of a page works and the user agent sticks for
On 2017/04/11 02:11:57, Eugene But wrote:
> nit: |Tests that| ?

Done.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:133: // Tests
requesting desktop site of a page works and the user agent sticks for
On 2017/04/11 02:11:57, Eugene But wrote:
> This is really great set of tests, do you want to move all UA tests to a
> separate file?

sure, that sounds reasonable, and btw, thank you for the kind words!

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:134: // next
navigations in the same tab.
On 2017/04/11 02:11:56, Eugene But wrote:
> nit: |the next navigations|?

Done.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:142: [[EarlGrey
selectElementWithMatcher:chrome_test_util::WebViewContainingText(
On 2017/04/11 02:11:57, Eugene But wrote:
> Do you want to add |using chrome_test_util::WebViewContainingText| to improve
> the formatting?

Done.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:161: // Tests
requesting desktop site of a page works and desktop user agent doesn't
On 2017/04/11 02:11:57, Eugene But wrote:
> nit: |Tests the requesting|?

Done.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:162: // stick in new
tab.
On 2017/04/11 02:11:57, Eugene But wrote:
> nit: s/doesn't stick in new tab/does not propagate to the new tab ?

Done.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:190: // Tests
requesting desktop site of a page works and going back re-opens mobile
On 2017/04/11 02:11:57, Eugene But wrote:
> |Tests that requesting|

Done.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:307: [ChromeEarlGrey
loadURL:GURL("chrome://version")];
On 2017/04/11 02:11:57, Eugene But wrote:
> chrome://version is not native but WebUI. Do you want to rename the test?

Done.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:320: // Tests
requesting desktop site works correctly for script-based desktop/mobile
On 2017/04/11 02:11:57, Eugene But wrote:
> What is "script-based desktop/mobile differentation."? Also how is this test
> different from the previous tests?

Here is the script in the html file:
<script type="text/javascript">
if (navigator.appVersion.toLowerCase().indexOf("mobile") > -1)
  document.write('Mobile');
else
  document.write('Desktop');
</script>

It seems that this test does nothing but accessing user agent type using JS.
hmm, it's hard to say if it's necessary, but I prefer to delete it, WDYT?

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:339: // Tests
requesting mobile site works correctly for script-based desktop/mobile
On 2017/04/11 02:11:57, Eugene But wrote:
> Same questions here.

Acknowledged.

[Eugene But (OOO till 7-30), 2017-04-11 22:21:19]

lgtm!

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:320: // Tests
requesting desktop site works correctly for script-based desktop/mobile
On 2017/04/11 18:27:20, liaoyuke wrote:
> On 2017/04/11 02:11:57, Eugene But wrote:
> > What is "script-based desktop/mobile differentation."? Also how is this test
> > different from the previous tests?
> 
> Here is the script in the html file:
> <script type="text/javascript">
> if (navigator.appVersion.toLowerCase().indexOf("mobile") > -1)
>   document.write('Mobile');
> else
>   document.write('Desktop');
> </script>
> 
> It seems that this test does nothing but accessing user agent type using JS.
> hmm, it's hard to say if it's necessary, but I prefer to delete it, WDYT? 
Those are useful tests in general, so I think we should keep them.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:320: // Tests
requesting desktop site works correctly for script-based desktop/mobile
How about "Tests that navigator.appVersion JavaScript API returns correct string
for desktop User Agent."? Same question for |testToolsMenuRequestMobileScript|
comments.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:322: -
(void)testToolsMenuRequestDesktopScript {
How about |testAppVersionJSAPIWithDesktopUser| agent? Same question for
|testToolsMenuRequestMobileScript|.

https://codereview.chromium.org/2789433006/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm (right):

https://codereview.chromium.org/2789433006/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:8: #include
"base/strings/sys_string_conversions.h"
Is this include still necessary? Could you please check other includes which may
not be needed anymore.

[liaoyuke, 2017-04-12 00:26:18]

Thank you very much for reviewing!

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm (right):

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:320: // Tests
requesting desktop site works correctly for script-based desktop/mobile
On 2017/04/11 22:21:19, Eugene But wrote:
> How about "Tests that navigator.appVersion JavaScript API returns correct
string
> for desktop User Agent."? Same question for |testToolsMenuRequestMobileScript|
> comments.

Thanks! The new name and comments are much more descriptive.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:320: // Tests
requesting desktop site works correctly for script-based desktop/mobile
On 2017/04/11 22:21:19, Eugene But wrote:
> On 2017/04/11 18:27:20, liaoyuke wrote:
> > On 2017/04/11 02:11:57, Eugene But wrote:
> > > What is "script-based desktop/mobile differentation."? Also how is this
test
> > > different from the previous tests?
> > 
> > Here is the script in the html file:
> > <script type="text/javascript">
> > if (navigator.appVersion.toLowerCase().indexOf("mobile") > -1)
> >   document.write('Mobile');
> > else
> >   document.write('Desktop');
> > </script>
> > 
> > It seems that this test does nothing but accessing user agent type using JS.
> > hmm, it's hard to say if it's necessary, but I prefer to delete it, WDYT? 
> Those are useful tests in general, so I think we should keep them.

Acknowledged.

https://codereview.chromium.org/2789433006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:322: -
(void)testToolsMenuRequestDesktopScript {
On 2017/04/11 22:21:19, Eugene But wrote:
> How about |testAppVersionJSAPIWithDesktopUser| agent? Same question for
> |testToolsMenuRequestMobileScript|.

Done.

https://codereview.chromium.org/2789433006/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm (right):

https://codereview.chromium.org/2789433006/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:8: #include
"base/strings/sys_string_conversions.h"
On 2017/04/11 22:21:19, Eugene But wrote:
> Is this include still necessary? Could you please check other includes which
may
> not be needed anymore.

Done.

[liaoyuke, 2017-04-12 00:48:50]

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

[commit-bot: I haz the power, 2017-04-12 00:49:24]

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-04-12 01:51:36]

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

[commit-bot: I haz the power, 2017-04-12 01:51:37]

Dry run: 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...)

[liaoyuke, 2017-04-12 02:00:41]

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

[commit-bot: I haz the power, 2017-04-12 02:01:11]

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-04-12 02:59:51]

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

[commit-bot: I haz the power, 2017-04-12 02:59:52]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-04-12 06:11:17]

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

[commit-bot: I haz the power, 2017-04-12 06:11:36]

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-04-12 07:13:45]

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

[commit-bot: I haz the power, 2017-04-12 07:13:46]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-04-12 16:00:57]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-04-12 16:00:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org, sczs@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2789433006/#ps180001
(title: "Fix failing eg tests on iPad.")

[commit-bot: I haz the power, 2017-04-12 16:01:40]

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-04-12 16:10:11]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492012857416080,
"parent_rev": "828b2c40768e30aba718697edc4fe1f1c10e9b8a", "commit_rev":
"6ab36201c67153ba617a6a9817e22bc3d303f927"}

[commit-bot: I haz the power, 2017-04-12 16:11:01]

Description was changed from

==========
Implement request mobile site.

This CL implements the "Request Mobile Site" functionality to enable
switching from the desktop version of a page to mobile version.

BUG=678047
==========

to

==========
Implement request mobile site.

This CL implements the "Request Mobile Site" functionality to enable
switching from the desktop version of a page to mobile version.

BUG=678047

Review-Url: https://codereview.chromium.org/2789433006
Cr-Commit-Position: refs/heads/master@{#464046}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ab36201c67153ba617a6a9817e2...
==========

[commit-bot: I haz the power, 2017-04-12 16:11:03]

Committed patchset #7 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/6ab36201c67153ba617a6a9817e2...