Call setUseMobileViewportStyle before setViewportEnabled

Call setUseMobileViewportStyle before setViewportEnabled

In order to avoid initialization of the preloadScanner with the wrong viewport dimensions, this CL moves the initialization of mobile viewport dimensions further up, so that they would happen before the viewport is initialized.

BUG=531820
Committed: https://crrev.com/a1a508b42224a697a0a621c36bfb55fe2f8bd760
Cr-Commit-Position: refs/heads/master@{#355041}

[bokan, 2015-10-08 15:03:46]

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

[bokan, 2015-10-08 15:03:47]

This makes sense to me (actually a simpler fix than I expected!).

As for test, layout tests wont work since we're testing renderer initialization
and layout tests usually reuse renderers. Browser test should work but I don't
know much about them and haven't written one myself so I'm afraid I won't be
much help there.

https://codereview.chromium.org/1392213002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1392213002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1063:
settings->setViewportEnabled(prefs.viewport_enabled);
How about moving this block (viewportEnabled to MainFrameResizes...) of settings
down below the OS_ANDROID ifdef? A quick look at the side-effects seems like
that should work too and would avoid splitting the Android specific calls.

[Yoav Weiss, 2015-10-09 09:16:28]

On 2015/10/08 15:03:47, bokan wrote:
> This makes sense to me (actually a simpler fix than I expected!).
> 
> As for test, layout tests wont work since we're testing renderer
initialization
> and layout tests usually reuse renderers. Browser test should work but I don't
> know much about them and haven't written one myself so I'm afraid I won't be
> much help there.

On second thought, I don't think browser tests run on the target platform (at
least not as part of the CQ).
I could observe the implications of bad initialization using layout test, but
they don't run on Android/DevTools either.

Any ideas on how to test it would be appreciated...

> 
>
https://codereview.chromium.org/1392213002/diff/1/content/renderer/render_vie...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/1392213002/diff/1/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:1063:
> settings->setViewportEnabled(prefs.viewport_enabled);
> How about moving this block (viewportEnabled to MainFrameResizes...) of
settings
> down below the OS_ANDROID ifdef? A quick look at the side-effects seems like
> that should work too and would avoid splitting the Android specific calls.

Sure. Moved that block down.

[bokan, 2015-10-09 13:39:46]

On 2015/10/09 09:16:28, Yoav Weiss wrote:
> On 2015/10/08 15:03:47, bokan wrote:
> > This makes sense to me (actually a simpler fix than I expected!).
> > 
> > As for test, layout tests wont work since we're testing renderer
> initialization
> > and layout tests usually reuse renderers. Browser test should work but I
don't
> > know much about them and haven't written one myself so I'm afraid I won't be
> > much help there.
> 
> On second thought, I don't think browser tests run on the target platform (at
> least not as part of the CQ).
> I could observe the implications of bad initialization using layout test, but
> they don't run on Android/DevTools either.
> 
> Any ideas on how to test it would be appreciated...

Hmm...if you can observe the problem in layout tests you can add it to the
SmokeTests file which runs a small subset of layout tests on Android. I suspect
though you can see if when you run your test in isolation. I think if you ran it
as part of a larger suite and it wasn't first you probably wouldn't hit the
issue.

I think if you can get a browser test written, even if it doesn't run in the CQ
it would probably be enough. Unfortunately, Android coverage in the CQ is
lacking AFAIK.
> 
> > 
> >
>
https://codereview.chromium.org/1392213002/diff/1/content/renderer/render_vie...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1392213002/diff/1/content/renderer/render_vie...
> > content/renderer/render_view_impl.cc:1063:
> > settings->setViewportEnabled(prefs.viewport_enabled);
> > How about moving this block (viewportEnabled to MainFrameResizes...) of
> settings
> > down below the OS_ANDROID ifdef? A quick look at the side-effects seems like
> > that should work too and would avoid splitting the Android specific calls.
> 
> Sure. Moved that block down.

[Yoav Weiss, 2015-10-16 10:32:59]

Added a browser test. PTAL.

[bokan, 2015-10-16 16:19:55]

Change itself lgtm (not an OWNER there though).

Test looks plausible to me but I know nothing about browser tests. Have you
verified the test fails without your change applied?

https://codereview.chromium.org/1392213002/diff/40001/content/browser/resourc...
File content/browser/resource_loading_browsertest.cc (right):

https://codereview.chromium.org/1392213002/diff/40001/content/browser/resourc...
content/browser/resource_loading_browsertest.cc:25:
content::TestNavigationObserver observer(shell()->web_contents());
Why is this needed? It seems unused.

[Yoav Weiss, 2015-10-17 00:45:51]

On 2015/10/16 16:19:55, bokan wrote:
> Change itself lgtm (not an OWNER there though).
> 
> Test looks plausible to me but I know nothing about browser tests. Have you
> verified the test fails without your change applied?

Verified it manually by going to said page hosted locally. I'm not sure how to
run the test in either device emulation mode or on android.
re
> 
>
https://codereview.chromium.org/1392213002/diff/40001/content/browser/resourc...
> File content/browser/resource_loading_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1392213002/diff/40001/content/browser/resourc...
> content/browser/resource_loading_browsertest.cc:25:
> content::TestNavigationObserver observer(shell()->web_contents());
> Why is this needed? It seems unused.

It is not. Removed

[Yoav Weiss, 2015-10-17 00:49:03]

On 2015/10/17 00:45:51, Yoav Weiss wrote:
> On 2015/10/16 16:19:55, bokan wrote:
> > Change itself lgtm (not an OWNER there though).
> > 

Can you add related OWNERs?

[Yoav Weiss, 2015-10-17 10:12:41]

yoav@yoav.ws changed reviewers:
+ jochen@chromium.org

[Yoav Weiss, 2015-10-17 10:12:41]

Jochen, care to take a look?

[jochen (gone - plz use gerrit), 2015-10-20 09:48:04]

lgtm

https://codereview.chromium.org/1392213002/diff/60001/content/test/data/resou...
File content/test/data/resource_loading/resource_loading_non_mobile.html
(right):

https://codereview.chromium.org/1392213002/diff/60001/content/test/data/resou...
content/test/data/resource_loading/resource_loading_non_mobile.html:1: <html>
please add <!DOCTYPE html>

[Yoav Weiss, 2015-10-20 10:16:01]

On 2015/10/20 09:48:04, jochen wrote:
> lgtm

Thanks for reviewing!

> 
>
https://codereview.chromium.org/1392213002/diff/60001/content/test/data/resou...
> File content/test/data/resource_loading/resource_loading_non_mobile.html
> (right):
> 
>
https://codereview.chromium.org/1392213002/diff/60001/content/test/data/resou...
> content/test/data/resource_loading/resource_loading_non_mobile.html:1: <html>
> please add <!DOCTYPE html>

Added.

[Yoav Weiss, 2015-10-20 10:16:28]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2015-10-20 10:16:41]

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

[commit-bot: I haz the power, 2015-10-20 11:58:23]

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

[commit-bot: I haz the power, 2015-10-20 11:58:24]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2015-10-20 11:58:49]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-10-20 11:58:49]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1392213002/#ps80001
(title: "Added doctype")

[commit-bot: I haz the power, 2015-10-20 11:58:57]

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

[commit-bot: I haz the power, 2015-10-20 12:03:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-20 12:04:15]

Patchset 5 (id:??) landed as
https://crrev.com/a1a508b42224a697a0a621c36bfb55fe2f8bd760
Cr-Commit-Position: refs/heads/master@{#355041}