Ignore viewport meta tags when Request Desktop Site is enabled.

Ignore viewport meta tags when Request Desktop Site is enabled.

This matches desktop Chrome, and it applies the default
"min-width: 980px;" in viewportAndroid.css -- putting responsive
pages into their desktop/tablet layout.

The enableViewport/enableMetaViewport settings distinction had
bit-rotted somewhat since they've always been the same value
(in fact disabling the latter setting didn't actually drop support
for it) so I needed to do some cleanup there as well.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5252baa9fbff3f1ffda51a4390cdf43070af22d7
Cr-Commit-Position: refs/heads/master@{#386313}

[aelias_OOO_until_Jul13, 2016-03-11 04:48:45]

Description was changed from

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

The best practice for responsive design nowadays is to serve a single
page with a meta viewport width="device-width" to all devices,
irrespective of user agent, and to adjust the layout to desktop or
mobile according to the window width.  Unfortunately, this has the side
effect of making "Request Desktop Site" feature have no effect.

This patch makes us ignore the author-supplied meta viewport tag when
Request Desktop Site is enabled.  This matches desktop Chrome, and it
has the effect of applying the default "min-width: 980px;" in
viewportAndroid.css -- putting such responsive pages into their
desktop/tablet layout.

BUG=586286
==========

to

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

The best practice for responsive design nowadays is to serve a single
page with a meta viewport width="device-width" to all devices,
irrespective of user agent, and to adjust the layout to desktop or
mobile according to the window width.  Unfortunately, this has the side
effect of making "Request Desktop Site" feature have no effect.

This patch makes us ignore the author-supplied meta viewport tag when
Request Desktop Site is enabled.  This matches desktop Chrome, and it
has the effect of applying the default "min-width: 980px;" in
viewportAndroid.css -- putting such responsive pages into their
desktop/tablet layout.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[aelias_OOO_until_Jul13, 2016-03-11 04:49:18]

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

[commit-bot: I haz the power, 2016-03-11 04:50:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785953002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785953002/1

[aelias_OOO_until_Jul13, 2016-03-11 04:53:53]

Description was changed from

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

The best practice for responsive design nowadays is to serve a single
page with a meta viewport width="device-width" to all devices,
irrespective of user agent, and to adjust the layout to desktop or
mobile according to the window width.  Unfortunately, this has the side
effect of making "Request Desktop Site" feature have no effect.

This patch makes us ignore the author-supplied meta viewport tag when
Request Desktop Site is enabled.  This matches desktop Chrome, and it
has the effect of applying the default "min-width: 980px;" in
viewportAndroid.css -- putting such responsive pages into their
desktop/tablet layout.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

This matches desktop Chrome, and it applies the default 
"min-width: 980px;" in viewportAndroid.css -- putting responsive
pages into their desktop/tablet layout.

The enableViewport/enableMetaViewport settings distinction had
bit-rotted somewhat since they've always been the same value 
(in fact disabling the latter setting didn't actually drop support
for it) so I needed to do some cleanup there as well.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[aelias_OOO_until_Jul13, 2016-03-11 04:55:09]

aelias@chromium.org changed reviewers:
+ bokan@chromium.org

[aelias_OOO_until_Jul13, 2016-03-11 04:56:01]

Hi bokan@, PTAL for initial review.

[commit-bot: I haz the power, 2016-03-11 05:54:16]

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

[commit-bot: I haz the power, 2016-03-11 05:54:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-03-11 20:34:36]

lgtm.

Media queries will still get the device's screen width in:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'm ok landing this and seeing how far it gets us though.

https://codereview.chromium.org/1785953002/diff/1/content/browser/android/con...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/1785953002/diff/1/content/browser/android/con...
content/browser/android/content_startup_flags.cc:43:
parsed_command_line->AppendSwitch(switches::kEnableViewport);
Why is this needed now?

[aelias_OOO_until_Jul13, 2016-03-12 01:57:24]

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

[commit-bot: I haz the power, 2016-03-12 01:57:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785953002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785953002/40001

[commit-bot: I haz the power, 2016-03-12 04:39:24]

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

[commit-bot: I haz the power, 2016-03-12 04:39:25]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-03-14 20:14:04]

aelias@chromium.org changed reviewers:
+ boliu@chromium.org, rbyers@chromium.org, sievers@chromium.org

[aelias_OOO_until_Jul13, 2016-03-14 20:14:30]

Adding OWNERS:

sievers@ for content/
rbyers@ for Source/core/
boliu@ for android_webview/

https://codereview.chromium.org/1785953002/diff/1/content/browser/android/con...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/1785953002/diff/1/content/browser/android/con...
content/browser/android/content_startup_flags.cc:43:
parsed_command_line->AppendSwitch(switches::kEnableViewport);
On 2016/03/11 at 20:34:36, bokan wrote:
> Why is this needed now?

It's because I removed the setting of viewport_enabled from
viewport_meta_enabled in render_view_host_impl.cc; some other way is then needed
to set viewport_enabled on Android.

[Rick Byers, 2016-03-14 20:18:58]

core LGTM

[boliu, 2016-03-14 20:22:12]

a_w rs lgtm

[johnme, 2016-03-15 11:38:09]

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

[johnme, 2016-03-15 11:38:10]

Drive-by: this seems a good change. We didn't do this when first implementing
RDS because responsive websites were rare, and the hope was that they would show
the same information on mobile as on desktop, thus making RDS unnecessary. But
in practice responsive websites do often hide parts of the page on mobile.

Note that this may break some responsive websites. Unless I've missed something,
the following will continue to evaluate to their true mobile values:

CSS @media device-width
JS screen.width
JS screen.availWidth
JS window.outerWidth
CSS @media device-height
JS screen.height
JS screen.availHeight
JS window.outerHeight

Of these, @media device-width/device-height are the most concerning. Whilst
websites should never use them (you should always use @media width and @media
height instead, in fact device-width/device-height have been deprecated in Media
Queries Level 4 because they're a footgun), I remember them being very common in
practice.

That said, since users have to opt-in to RDS, it's ok to break some sites.

https://codereview.chromium.org/1785953002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1785953002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3137: if (settings() &&
!settings()->viewportMetaEnabled() && viewportDescription.isMetaViewportType())
Why do you only disable meta viewport in Request Desktop Mode? Why not also
ignore HandheldFriendlyMeta and MobileOptimizedMeta?

As for @viewport, are you treating that differently because the spec says it
should be implemented the same on mobile and desktop?

[aelias_OOO_until_Jul13, 2016-03-15 18:39:27]

On 2016/03/15 at 11:38:10, johnme wrote:
> Drive-by: this seems a good change. We didn't do this when first implementing
RDS because responsive websites were rare, and the hope was that they would show
the same information on mobile as on desktop, thus making RDS unnecessary. But
in practice responsive websites do often hide parts of the page on mobile.

Yeah, from the user point of view it's just very frustrating when you click
"Request Desktop Site" and the page reloads to exactly the same thing.

> 
> Note that this may break some responsive websites. Unless I've missed
something, the following will continue to evaluate to their true mobile values:

Yes, bokan@ made the same point.  But it would complexify the implementation
significantly to start affecting other stuff, and I didn't want to do it without
a reason.  We can revisit if/when examples come up of breakage, thanks for the
list.  This patch worked for many popular sites.

[aelias_OOO_until_Jul13, 2016-03-15 19:14:15]

https://codereview.chromium.org/1785953002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1785953002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3137: if (settings() &&
!settings()->viewportMetaEnabled() && viewportDescription.isMetaViewportType())
On 2016/03/15 at 11:38:10, johnme wrote:
> Why do you only disable meta viewport in Request Desktop Mode? Why not also
ignore HandheldFriendlyMeta and MobileOptimizedMeta?

Those are rarely seen, and I think it's especially unlikely that an old site
would send a HandheldFriendlyMeta in response to a desktop UA.  I think there's
an argument to be made that from the point of view of the Blink setting,
!viewportMetaEnabled should disable *all* legacy viewport tags though.

> 
> As for @viewport, are you treating that differently because the spec says it
should be implemented the same on mobile and desktop?

There's already a separate setting "viewportEnabled" for @viewport yes.  Note
that we need to always support it in some form on Android since the magic "980"
value for desktop sites is in an @viewport tag for viewportAndroid.css.

[aelias_OOO_until_Jul13, 2016-03-15 19:29:26]

https://codereview.chromium.org/1785953002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1785953002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3137: if (settings() &&
!settings()->viewportMetaEnabled() && viewportDescription.isMetaViewportType())
On 2016/03/15 at 19:14:15, aelias wrote:
> On 2016/03/15 at 11:38:10, johnme wrote:
> > Why do you only disable meta viewport in Request Desktop Mode? Why not also
ignore HandheldFriendlyMeta and MobileOptimizedMeta?
> 
> Those are rarely seen, and I think it's especially unlikely that an old site
would send a HandheldFriendlyMeta in response to a desktop UA.  I think there's
an argument to be made that from the point of view of the Blink setting,
!viewportMetaEnabled should disable *all* legacy viewport tags though.

OK, I made this change.

[aelias_OOO_until_Jul13, 2016-03-15 19:29:34]

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

[commit-bot: I haz the power, 2016-03-15 19:29:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785953002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785953002/60001

[johnme, 2016-03-15 19:36:22]

lgtm :)

[no sievers, 2016-03-15 19:56:02]

Description was changed from

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

This matches desktop Chrome, and it applies the default 
"min-width: 980px;" in viewportAndroid.css -- putting responsive
pages into their desktop/tablet layout.

The enableViewport/enableMetaViewport settings distinction had
bit-rotted somewhat since they've always been the same value 
(in fact disabling the latter setting didn't actually drop support
for it) so I needed to do some cleanup there as well.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

This matches desktop Chrome, and it applies the default 
"min-width: 980px;" in viewportAndroid.css -- putting responsive
pages into their desktop/tablet layout.

The enableViewport/enableMetaViewport settings distinction had
bit-rotted somewhat since they've always been the same value 
(in fact disabling the latter setting didn't actually drop support
for it) so I needed to do some cleanup there as well.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-03-15 19:56:02]

sievers@chromium.org changed reviewers:
+ nasko@chromium.org

[no sievers, 2016-03-15 20:00:05]

+nasko

https://codereview.chromium.org/1785953002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1571:
delegate_->UpdateOverridingUserAgent();
Does passing this to the RWH here actually work for every navigation (incl. back
for example)?

+nasko@ also since the NavigationControllerDelegate has comments that it's
temporary.
It seems like overriding WebPreferences would be better through
ContentBrowserClient::OverrideWebkitPrefs(), but it still doesn't seem
straightforward since we associate this override with the navigation entry.

[commit-bot: I haz the power, 2016-03-15 21:49:07]

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

[commit-bot: I haz the power, 2016-03-15 21:49:09]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-03-18 21:05:26]

Sorry for the delay. I think the delegate way might be avoidable, which you
already hinted at.

Also, as it is, this code won't work for out-of-process iframes, so we either
need a bug to track that and put in a comment, or make it compatible in this CL.
I'm ok with the former, assuming this is acceptable behavior, then feel free to
assign the bug to me.

https://codereview.chromium.org/1785953002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1571:
delegate_->UpdateOverridingUserAgent();
On 2016/03/15 20:00:05, sievers wrote:
> Does passing this to the RWH here actually work for every navigation (incl.
back
> for example)?

Did you meant RenderViewHost? RenderWidgetHost shouldn't be dealing with user
agent information.
How would you know which RenderViewHost to send it to?
 
> +nasko@ also since the NavigationControllerDelegate has comments that it's
> temporary.

Yeah, that might be wrong by now :).

> It seems like overriding WebPreferences would be better through
> ContentBrowserClient::OverrideWebkitPrefs(), but it still doesn't seem
> straightforward since we associate this override with the navigation entry.

The current code in this CL doesn't seem to be associated at all with
NavigationEntry either. Why not take the
ContentBrowserClient::OverrideWebkitPrefs approach and just call
RenderViewHost::OnWebkitPreferencesChanged from the Android code?

https://codereview.chromium.org/1785953002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:240: // Whether the
user agent is overridden using the Android "Request Desktop
nit: Technically the feature is of Chrome-on-Android, not Android itself, which
this text reads as.

https://codereview.chromium.org/1785953002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4856:
GetRenderViewHost()->OnWebkitPreferencesChanged();
This doesn't seem compatible with out-of-process iframes. The main frame will
use the proper user agent, but other processes rendering subframes will not.

You will need to call this method on each RenderViewHost that is in the list
found in the FrameTree object.

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
content/public/browser/navigation_controller.h:494: virtual void
UpdateOverridingUserAgent() = 0;
There are no callers of this method outside of content/, right? We don't add
methods to the public interface unless they are needed by other modules.

[aelias_OOO_until_Jul13, 2016-03-18 22:49:18]

https://codereview.chromium.org/1785953002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1571:
delegate_->UpdateOverridingUserAgent();
On 2016/03/18 at 21:05:25, nasko (slow) wrote:
> The current code in this CL doesn't seem to be associated at all with
NavigationEntry either. Why not take the
ContentBrowserClient::OverrideWebkitPrefs approach and just call
RenderViewHost::OnWebkitPreferencesChanged from the Android code?

I took that approach initially but it failed DEPS verification when I tried to
upload the patch.  This plumbing is the simplest way I found so far that
appeared valid.

https://codereview.chromium.org/1785953002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:240: // Whether the
user agent is overridden using the Android "Request Desktop
On 2016/03/18 at 21:05:25, nasko (slow) wrote:
> nit: Technically the feature is of Chrome-on-Android, not Android itself,
which this text reads as.

Fixed.

https://codereview.chromium.org/1785953002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4856:
GetRenderViewHost()->OnWebkitPreferencesChanged();
On 2016/03/18 at 21:05:25, nasko (slow) wrote:
> This doesn't seem compatible with out-of-process iframes. The main frame will
use the proper user agent, but other processes rendering subframes will not.
> 
> You will need to call this method on each RenderViewHost that is in the list
found in the FrameTree object.

The enableViewportMeta setting only ever has an effect on the main frame. 
Subframes ignore viewport meta tags by de-facto specification.  So I think
adding additional calls would be unnecessary complexity.

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
content/public/browser/navigation_controller.h:494: virtual void
UpdateOverridingUserAgent() = 0;
On 2016/03/18 at 21:05:26, nasko (slow) wrote:
> There are no callers of this method outside of content/, right? We don't add
methods to the public interface unless they are needed by other modules.

On Android the content/ directory boundary doesn't map properly to the DEPS
boundary, so this seemed to be needed.  This is something Daniel and I discussed
earlier this week, we intend to start a project fix this confusion in the longer
term.

[nasko, 2016-03-25 23:29:05]

Apologies for the delay. This has slipped through the cracks with all the perf
work in the last couple of weeks.

https://codereview.chromium.org/1785953002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4856:
GetRenderViewHost()->OnWebkitPreferencesChanged();
On 2016/03/18 22:49:18, aelias wrote:
> On 2016/03/18 at 21:05:25, nasko (slow) wrote:
> > This doesn't seem compatible with out-of-process iframes. The main frame
will
> use the proper user agent, but other processes rendering subframes will not.
> > 
> > You will need to call this method on each RenderViewHost that is in the list
> found in the FrameTree object.
> 
> The enableViewportMeta setting only ever has an effect on the main frame. 
> Subframes ignore viewport meta tags by de-facto specification.  So I think
> adding additional calls would be unnecessary complexity.

But this is method that calls for overriding user agent. It seems weird to use
different user agent strings on frames of the same page.

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
content/public/browser/navigation_controller.h:494: virtual void
UpdateOverridingUserAgent() = 0;
On 2016/03/18 22:49:18, aelias wrote:
> On 2016/03/18 at 21:05:26, nasko (slow) wrote:
> > There are no callers of this method outside of content/, right? We don't add
> methods to the public interface unless they are needed by other modules.
> 
> On Android the content/ directory boundary doesn't map properly to the DEPS
> boundary, so this seemed to be needed.  This is something Daniel and I
discussed
> earlier this week, we intend to start a project fix this confusion in the
longer
> term.

This should not go here if it is not called from outside of content/. Can you
give me a more detailed explanation of what is the problem with DEPS. Also, if
there is a DEPS issue, can't it be resolved there?

[no sievers, 2016-03-28 19:53:59]

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
content/public/browser/navigation_controller.h:494: virtual void
UpdateOverridingUserAgent() = 0;
On 2016/03/25 23:29:05, nasko (slow) wrote:
> On 2016/03/18 22:49:18, aelias wrote:
> > On 2016/03/18 at 21:05:26, nasko (slow) wrote:
> > > There are no callers of this method outside of content/, right? We don't
add
> > methods to the public interface unless they are needed by other modules.
> > 
> > On Android the content/ directory boundary doesn't map properly to the DEPS
> > boundary, so this seemed to be needed.  This is something Daniel and I
> discussed
> > earlier this week, we intend to start a project fix this confusion in the
> longer
> > term.
> 
> This should not go here if it is not called from outside of content/. Can you
> give me a more detailed explanation of what is the problem with DEPS. Also, if
> there is a DEPS issue, can't it be resolved there?

So these call sites are impl-to-impl, right? And unnecessarily rerouted through
the public interface.

I suggest changing NavigationControllerAndroid so it has a pointer to the
NavigationControllerImpl, which you can pass in from the construction call site
in WebContentsAndroid (you can static_cast the WebContents to the
WebContentsImpl and then call WebContentsImpl::GetController()).

[aelias_OOO_until_Jul13, 2016-04-07 03:06:14]

PTAL.  nasko@, I addressed all your concerns.

https://codereview.chromium.org/1785953002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1571:
delegate_->UpdateOverridingUserAgent();
On 2016/03/18 at 22:49:18, aelias wrote:
> On 2016/03/18 at 21:05:25, nasko (slow) wrote:
> > The current code in this CL doesn't seem to be associated at all with
NavigationEntry either. Why not take the
ContentBrowserClient::OverrideWebkitPrefs approach and just call
RenderViewHost::OnWebkitPreferencesChanged from the Android code?
> 
> I took that approach initially but it failed DEPS verification when I tried to
upload the patch.  This plumbing is the simplest way I found so far that
appeared valid.

I was able to remove the unnecessary new NavigationControllerImpl method by
making use of the existing NavigationControllerImpl::delegate().  (The DEPS rule
that I had been violating is that I couldn't call into WebContents from
frame_host/.)

https://codereview.chromium.org/1785953002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4856:
GetRenderViewHost()->OnWebkitPreferencesChanged();
OK, I changed to loop through the FrameTreeNodes (although the actual user agent
string doesn't need this to propagate, this just Update call only affects the
meta tag currently, but I suppose that might not always be the case).

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1785953002/diff/60001/content/public/browser/...
content/public/browser/navigation_controller.h:494: virtual void
UpdateOverridingUserAgent() = 0;
On 2016/03/28 at 19:53:59, sievers wrote:
> So these call sites are impl-to-impl, right? And unnecessarily rerouted
through the public interface.
> 
> I suggest changing NavigationControllerAndroid so it has a pointer to the
NavigationControllerImpl, which you can pass in from the construction call site
in WebContentsAndroid (you can static_cast the WebContents to the
WebContentsImpl and then call WebContentsImpl::GetController()).

OK, I removed the public method.  (Actually nothing was routed through the
public interface after all, just the "Impl" typing was unnecessarily dropped in
WebContentsAndroid constructor, so by adding it back there I was able to get to
the NavigationControllerImpl without resorting to a static_cast.)

[nasko, 2016-04-08 22:32:35]

Looks much better! Thanks for making these changes. Just a couple minor things.

https://codereview.chromium.org/1785953002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/1785953002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_android.cc:15: #include
"content/public/browser/render_view_host.h"
Why is this header needed? I don't see any new code referencing RenderViewHost.

https://codereview.chromium.org/1785953002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4864:
render_view_host->OnWebkitPreferencesChanged();
It will be more efficient to add the RenderViewHost pointers you get to a
std::set, as it is likely that more than one FrameTreeNode will share
RenderViewHost. This will avoid sending multiple updates to the same RenderView.

[aelias_OOO_until_Jul13, 2016-04-08 23:31:36]

https://codereview.chromium.org/1785953002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/1785953002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_android.cc:15: #include
"content/public/browser/render_view_host.h"
On 2016/04/08 at 22:32:35, nasko (very slow Apr 8-18) wrote:
> Why is this header needed? I don't see any new code referencing
RenderViewHost.

Removed.

https://codereview.chromium.org/1785953002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4864:
render_view_host->OnWebkitPreferencesChanged();
On 2016/04/08 at 22:32:35, nasko (very slow Apr 8-18) wrote:
> It will be more efficient to add the RenderViewHost pointers you get to a
std::set, as it is likely that more than one FrameTreeNode will share
RenderViewHost. This will avoid sending multiple updates to the same RenderView.

Done.

[nasko, 2016-04-09 00:02:27]

LGTM with a couple of style comments.

https://codereview.chromium.org/1785953002/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1785953002/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4900: for (FrameTreeNode* node
: frame_tree_.Nodes())
Multiline for loop should have {}.

https://codereview.chromium.org/1785953002/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4901: if (RenderWidgetHost*
render_widget_host =
Style in this file is to have variable declaration on a line separate from the
check. It will also allow you to continue; easily if the check fails.

[aelias_OOO_until_Jul13, 2016-04-09 23:39:51]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-04-09 23:39:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
boliu@chromium.org, rbyers@chromium.org, johnme@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1785953002/#ps160001
(title: "Change loop style")

[commit-bot: I haz the power, 2016-04-09 23:39:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785953002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785953002/160001

[commit-bot: I haz the power, 2016-04-10 01:18:11]

Description was changed from

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

This matches desktop Chrome, and it applies the default 
"min-width: 980px;" in viewportAndroid.css -- putting responsive
pages into their desktop/tablet layout.

The enableViewport/enableMetaViewport settings distinction had
bit-rotted somewhat since they've always been the same value 
(in fact disabling the latter setting didn't actually drop support
for it) so I needed to do some cleanup there as well.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

This matches desktop Chrome, and it applies the default 
"min-width: 980px;" in viewportAndroid.css -- putting responsive
pages into their desktop/tablet layout.

The enableViewport/enableMetaViewport settings distinction had
bit-rotted somewhat since they've always been the same value 
(in fact disabling the latter setting didn't actually drop support
for it) so I needed to do some cleanup there as well.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-04-10 01:18:12]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-10 01:19:07]

Description was changed from

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

This matches desktop Chrome, and it applies the default 
"min-width: 980px;" in viewportAndroid.css -- putting responsive
pages into their desktop/tablet layout.

The enableViewport/enableMetaViewport settings distinction had
bit-rotted somewhat since they've always been the same value 
(in fact disabling the latter setting didn't actually drop support
for it) so I needed to do some cleanup there as well.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore viewport meta tags when Request Desktop Site is enabled.

This matches desktop Chrome, and it applies the default
"min-width: 980px;" in viewportAndroid.css -- putting responsive
pages into their desktop/tablet layout.

The enableViewport/enableMetaViewport settings distinction had
bit-rotted somewhat since they've always been the same value
(in fact disabling the latter setting didn't actually drop support
for it) so I needed to do some cleanup there as well.

BUG=586286
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5252baa9fbff3f1ffda51a4390cdf43070af22d7
Cr-Commit-Position: refs/heads/master@{#386313}
==========

[commit-bot: I haz the power, 2016-04-10 01:19:09]

Patchset 9 (id:??) landed as
https://crrev.com/5252baa9fbff3f1ffda51a4390cdf43070af22d7
Cr-Commit-Position: refs/heads/master@{#386313}