Count presence of viewport tag as mobile-friendly

Count presence of viewport tag as mobile-friendly

BUG=658416

[wychen, 2016-10-24 21:18:18]

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

[wychen, 2016-10-24 21:18:19]

PTAL

[aelias_OOO_until_Jul13, 2016-10-24 21:48:59]

https://codereview.chromium.org/2443753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2443753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:785: return
mainFrame()->view()->layoutSize().width() <= m_size.width() ||
Hmm, I think we should decouple this from the details of resolved viewport width
and screen size entirely.

Could you delete this logic entirely and merge this detection with
matchesHeuristicsForGpuRasterization?  That checks if the
ViewportDescription.isSpecifiedByAuthor.  Might make sense to rename some of the
GPU-raster-related names to m_viewportSpecifiedByAuthor as well and generalize
it to this use.

(The other uses of shouldDisableDesktopWorkarounds are for tap disambiguation
stuff and double-tap zoom stuff, both of which should also be OK with switching
to *any* viewport tag.)

[wychen, 2016-10-24 23:15:28]

Patchset #2 (id:20001) has been deleted

[wychen, 2016-10-24 23:18:52]

https://codereview.chromium.org/2443753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2443753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:785: return
mainFrame()->view()->layoutSize().width() <= m_size.width() ||
On 2016/10/24 21:48:59, aelias wrote:
> Hmm, I think we should decouple this from the details of resolved viewport
width
> and screen size entirely.
> 
> Could you delete this logic entirely and merge this detection with
> matchesHeuristicsForGpuRasterization?  That checks if the
> ViewportDescription.isSpecifiedByAuthor.  Might make sense to rename some of
the
> GPU-raster-related names to m_viewportSpecifiedByAuthor as well and generalize
> it to this use.
> 
> (The other uses of shouldDisableDesktopWorkarounds are for tap disambiguation
> stuff and double-tap zoom stuff, both of which should also be OK with
switching
> to *any* viewport tag.)

Done.

[wychen, 2016-10-24 23:37:37]

Description was changed from

==========
Treat small hardcoded viewport width as mobile-friendly

BUG=658416
==========

to

==========
Count presence of viewport as mobile-friendly

BUG=658416
==========

[wychen, 2016-10-24 23:38:15]

Description was changed from

==========
Count presence of viewport as mobile-friendly

BUG=658416
==========

to

==========
Count presence of viewport tag as mobile-friendly

BUG=658416
==========

[wychen, 2016-10-28 01:02:35]

aelias@, does this look better?

[aelias_OOO_until_Jul13, 2016-10-28 01:11:51]

https://codereview.chromium.org/2443753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2443753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:3359: m_viewportSpecifiedByAuthor
= true;
Hmm, I realized this line here makes the variable naming a misnomer, sorry for
the bad suggestion.  Please rename to m_isDesktopSiteOnMobile and invert the
true/false state of the boolean to match that name.

[wychen, 2016-10-28 21:21:33]

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

[wychen, 2016-10-28 21:21:38]

https://codereview.chromium.org/2443753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2443753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:3359: m_viewportSpecifiedByAuthor
= true;
On 2016/10/28 01:11:51, aelias wrote:
> Hmm, I realized this line here makes the variable naming a misnomer, sorry for
> the bad suggestion.  Please rename to m_isDesktopSiteOnMobile and invert the
> true/false state of the boolean to match that name.

Good catch! Done.

[commit-bot: I haz the power, 2016-10-28 21:21:51]

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

[aelias_OOO_until_Jul13, 2016-10-28 21:43:46]

lgtm modulo comment

https://codereview.chromium.org/2443753002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2443753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:420:
m_isDesktopSiteOnMobile(true),
Hmm, this has a slight risk of activating intended-to-be-Clank-specific code on
desktop platforms in a brief time window early in loading.  I don't think that
could make any difference with the current featureset tied to this boolean, but
I'd be more comfortable with a default of "false".

[wychen, 2016-10-28 23:07:36]

There are a few tests about disambiguation and zoom in failing when switching
from shouldDisableDesktopWorkarounds() to isSpecifiedByAuthor(). I think the
testing environment setup in configureAndroid() conflicts with my change.

Before I figure out how to fix these, it might be faster to revert the changes
in disambiguation and zoom in, focus on isMobileFriendly signal in this CL, and
remove shouldDisableDesktopWorkarounds() in another CL if that's desirable.

https://codereview.chromium.org/2443753002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2443753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:420:
m_isDesktopSiteOnMobile(true),
On 2016/10/28 21:43:46, aelias wrote:
> Hmm, this has a slight risk of activating intended-to-be-Clank-specific code
on
> desktop platforms in a brief time window early in loading.  I don't think that
> could make any difference with the current featureset tied to this boolean,
but
> I'd be more comfortable with a default of "false".

Done.

[aelias_OOO_until_Jul13, 2016-10-29 00:23:53]

On 2016/10/28 at 23:07:36, wychen wrote:
> There are a few tests about disambiguation and zoom in failing when switching
from shouldDisableDesktopWorkarounds() to isSpecifiedByAuthor(). I think the
testing environment setup in configureAndroid() conflicts with my change.
> 
> Before I figure out how to fix these, it might be faster to revert the changes
in disambiguation and zoom in, focus on isMobileFriendly signal in this CL, and
remove shouldDisableDesktopWorkarounds() in another CL if that's desirable.

The bug doesn't have any labels saying this needs to be done fast, so I don't
see any reason to split up the work that way.  In the absence of urgency I
usually prefer to spend a bit more time doing it right the first time.  This
test problem doesn't sound like it should be particularly difficult to fix, let
me know if you have any questions about the test setup.

[wychen, 2016-11-02 04:49:09]

On 2016/10/29 00:23:53, aelias wrote:
> On 2016/10/28 at 23:07:36, wychen wrote:
> > There are a few tests about disambiguation and zoom in failing when
switching
> from shouldDisableDesktopWorkarounds() to isSpecifiedByAuthor(). I think the
> testing environment setup in configureAndroid() conflicts with my change.
> > 
> > Before I figure out how to fix these, it might be faster to revert the
changes
> in disambiguation and zoom in, focus on isMobileFriendly signal in this CL,
and
> remove shouldDisableDesktopWorkarounds() in another CL if that's desirable.
> 
> The bug doesn't have any labels saying this needs to be done fast, so I don't
> see any reason to split up the work that way.  In the absence of urgency I
> usually prefer to spend a bit more time doing it right the first time.  This
> test problem doesn't sound like it should be particularly difficult to fix,
let
> me know if you have any questions about the test setup.

Good point.

This CL was intended to be the first one for issue 658416, and some more CLs
to follow.

According to #c6, using presence of meta viewport wouldn't work.
https://bugs.chromium.org/p/chromium/issues/detail?id=658416#c6
I think we have to use more fine-grained rules, more or less like ps1.

Disambiguation and pinch-zoom should follow the same rule as Reader Mode
mobile-friendliness signal. I'm not so sure about GPU rasterization though.

[aelias_OOO_until_Jul13, 2016-11-02 06:32:33]

On 2016/11/02 at 04:49:09, wychen wrote:
> According to #c6, using presence of meta viewport wouldn't work.
> https://bugs.chromium.org/p/chromium/issues/detail?id=658416#c6
> I think we have to use more fine-grained rules, more or less like ps1.

OK, you can try a more fine-grained rule, but let's keep it simple.  Maybe you
could use the existing criteria, but with an exclusion if an explicit fixed
width is specified in the viewport tag which is >= 980 (which is the magic
number we layout desktop sites to in non-viewport-tag mode).

> 
> Disambiguation and pinch-zoom should follow the same rule as Reader Mode
> mobile-friendliness signal. I'm not so sure about GPU rasterization though.

The exclusion for GPU rasterization is mainly because GPU rasterization
performance is currently pretty bad with pinch zoom, and you need to pinch zoom
a lot on desktop sites.  So I think it's OK to use the same rule since in all
cases we're detecting wide desktop sites more accurately.