blink: Enable the GPU trigger when viewport meta tag is not read

blink: Enable the GPU trigger when viewport meta tag is not read

When we're on a device that isn't using the viewport meta tag 
(e.g. desktop), we turn on the GPU trigger.

If we're emulating a device on desktop, the trigger does respect the meta
tag.

One gotcha for this change, if you use --single-process, we don't read
the white/black list before figuring out if we should enable GPU, so
it gets enabled by default -- I'll log an issue for this.

Please see https://codereview.chromium.org/813773002/ for the other half
of this change.

BUG=440518
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194901

[hendrikw, 2014-12-17 18:31:34]

PTAL

Also, @ajuma, can you suggest a good owner reviewer for this?

Thanks!

[ajuma, 2014-12-17 19:38:38]

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

[ajuma, 2014-12-17 19:38:38]

> Also, @ajuma, can you suggest a good owner reviewer for this?

aelias@ has done OWNER reviews for previous changes to the GPU trigger.

https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3260: void
WebViewImpl::updatePageDefinedViewportConstraints(const ViewportDescription&
description)
Does this always get called on pages that don't have any viewport tag?

https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3264: if (!settings()->viewportMetaEnabled()) {
Use settingsImpl instead of settings, and then you don't need to add anything to
WebSettings.h, only to WebSettingsImpl.h.

[hendrikw, 2014-12-17 20:20:01]

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

[hendrikw, 2014-12-17 20:20:01]

PTAL, thanks!

https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3260: void
WebViewImpl::updatePageDefinedViewportConstraints(const ViewportDescription&
description)
On 2014/12/17 19:38:38, ajuma wrote:
> Does this always get called on pages that don't have any viewport tag?

AFAICT, yes.  the callers WebViewImpl::performResize and
WebViewImpl::refreshPageScaleFactorAfterLayout don't skip calling this if the
viewport doesn't exist, but I could be misunderstanding the code.

https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3264: if (!settings()->viewportMetaEnabled()) {
On 2014/12/17 19:38:38, ajuma wrote:
> Use settingsImpl instead of settings, and then you don't need to add anything
to
> WebSettings.h, only to WebSettingsImpl.h.

Awesome, wasn't aware of that!

[ajuma, 2014-12-17 20:36:16]

https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3260: void
WebViewImpl::updatePageDefinedViewportConstraints(const ViewportDescription&
description)
On 2014/12/17 20:20:01, hendrikw wrote:
> On 2014/12/17 19:38:38, ajuma wrote:
> > Does this always get called on pages that don't have any viewport tag?
> 
> AFAICT, yes.  the callers WebViewImpl::performResize and
> WebViewImpl::refreshPageScaleFactorAfterLayout don't skip calling this if the
> viewport doesn't exist, but I could be misunderstanding the code.

Ok. Note that when we do have a viewport, this gets called by
ChromeClientImpl::dispatchViewportPropertiesDidChange, which traces back to
HTMLMetaElement::processViewportContentAttribute. So in that case we're not
relying on the resize-related callers.

https://codereview.chromium.org/792233007/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/792233007/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3264: if (!settingsImpl()->viewportMetaEnabled()) {
WebSettingsImpl will still need this method added to it.

[hendrikw, 2014-12-17 21:32:22]

On 2014/12/17 20:36:16, ajuma wrote:
>
https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/792233007/diff/20001/Source/web/WebViewImpl.c...
> Source/web/WebViewImpl.cpp:3260: void
> WebViewImpl::updatePageDefinedViewportConstraints(const ViewportDescription&
> description)
> On 2014/12/17 20:20:01, hendrikw wrote:
> > On 2014/12/17 19:38:38, ajuma wrote:
> > > Does this always get called on pages that don't have any viewport tag?
> > 
> > AFAICT, yes.  the callers WebViewImpl::performResize and
> > WebViewImpl::refreshPageScaleFactorAfterLayout don't skip calling this if
the
> > viewport doesn't exist, but I could be misunderstanding the code.
> 
> Ok. Note that when we do have a viewport, this gets called by
> ChromeClientImpl::dispatchViewportPropertiesDidChange, which traces back to
> HTMLMetaElement::processViewportContentAttribute. So in that case we're not
> relying on the resize-related callers.

Ok, do you think there's a case which we might miss setting this then?

On the few pages I've tested this with, it seemed to have always worked
correctly.

> 
>
https://codereview.chromium.org/792233007/diff/40001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/792233007/diff/40001/Source/web/WebViewImpl.c...
> Source/web/WebViewImpl.cpp:3264: if (!settingsImpl()->viewportMetaEnabled()) {
> WebSettingsImpl will still need this method added to it.

Done

[ajuma, 2014-12-17 21:44:32]

On 2014/12/17 21:32:22, hendrikw wrote:
> Ok, do you think there's a case which we might miss setting this then?
> 
> On the few pages I've tested this with, it seemed to have always worked
> correctly.

It looks like this always gets called because LocalDOMWindow::installNewDocument
calls updateViewportDescription. So we're not relying on the resize-related
callers after all. In that case, this lgtm.

[aelias_OOO_until_Jul13, 2014-12-18 00:16:50]

lgtm

[hendrikw, 2014-12-18 00:17:10]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-12-18 00:18:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/792233007/60001

[commit-bot: I haz the power, 2014-12-18 02:59:53]

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

[commit-bot: I haz the power, 2014-12-18 02:59:54]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/42464)

[hendrikw, 2014-12-30 01:10:32]

I finally had a chance to debug this.

content_shell (at least on desktop) is calling
content::IsGpuRasterizationEnabled() without first calling
content::GpuDataManagerImplPrivate::Initialize().  Initialize() creates the
blacklist.

Since the blacklist isn't loaded, gpu rasterization is enabled by default, and
my change causes it to remain on.

content::RenderWidgetCompositor::compositeAndReadbackAsync() via
content::WebTestProxyBase::CapturePixelsAsync() is used to output the layout
results, it tries to use the single_thread_synchronous_task_graph_runner_, which
only exists in software, and crashes!

[hendrikw, 2015-02-28 00:15:10]

On 2014/12/30 01:10:32, hendrikw wrote:
> I finally had a chance to debug this.
> 
> content_shell (at least on desktop) is calling
> content::IsGpuRasterizationEnabled() without first calling
> content::GpuDataManagerImplPrivate::Initialize().  Initialize() creates the
> blacklist.
> 
> Since the blacklist isn't loaded, gpu rasterization is enabled by default, and
> my change causes it to remain on.
> 
> content::RenderWidgetCompositor::compositeAndReadbackAsync() via
> content::WebTestProxyBase::CapturePixelsAsync() is used to output the layout
> results, it tries to use the single_thread_synchronous_task_graph_runner_,
which
> only exists in software, and crashes!

I'm about to attempt to land this again (after
https://codereview.chromium.org/963963002/ lands)

Any objections?

[aelias_OOO_until_Jul13, 2015-02-28 00:20:55]

Hmm, just so I follow, this makes "enable" behave similarly to "force" on
desktops, modulo:
1. GPU-based blacklist
2. devtools emulation
3. path-based veto (?)

Is that correct?

[hendrikw, 2015-02-28 00:23:30]

On 2015/02/28 00:20:55, aelias wrote:
> Hmm, just so I follow, this makes "enable" behave similarly to "force" on
> desktops, modulo:
> 1. GPU-based blacklist
> 2. devtools emulation
> 3. path-based veto (?)
> 
> Is that correct?

Currently, desktop is blacklisted, so the default will be disabled, but the user
can choose enable in flags, at which point it will start using the veto on
desktop.

[aelias_OOO_until_Jul13, 2015-02-28 00:47:25]

Won't desktop be disabled by entry 99 in
gpu/config/software_rendering_list_json.cc even if enabled in chrome://flags? 
Because Android devices default to "enable" flag, but are still disabled by that
blacklist.  So it seems like this patch on its own won't have any effect?

[hendrikw, 2015-02-28 00:51:01]

On 2015/02/28 00:47:25, aelias wrote:
> Won't desktop be disabled by entry 99 in
> gpu/config/software_rendering_list_json.cc even if enabled in chrome://flags? 
> Because Android devices default to "enable" flag, but are still disabled by
that
> blacklist.  So it seems like this patch on its own won't have any effect?

bool IsGpuRasterizationEnabled() checks for the command line
switches::kEnableGpuRasterization before checking the blacklist entry
(IsGpuRasterizationBlacklisted())

[aelias_OOO_until_Jul13, 2015-02-28 01:05:05]

So, my point is that we need two modes:

- "force" should just unconditionally enable GPU raster, for development
purposes
- "enable" should just enable when it makes sense, so for example it shouldn't
enable it on bad GPUs.

I assume the point of this change is to start preparing for a real launch of GPU
raster on desktop.  In that case, we need the ability to blacklist particular
GPU models, when they prove to be crappy.  That won't work if the setting
"enable" totally ignores the GPU blacklist.  Only "force" should do that.

[aelias_OOO_until_Jul13, 2015-02-28 01:13:33]

OK, in any case, this Blink patch still looks mostly good to go, but I just
wanted to make the point that one of your patch series should be removing entry
99 from software_rendering_list_json.cc or there's something wrong here.  Just
one nit:

https://codereview.chromium.org/792233007/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/792233007/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:3368: // If we're not reading the viewport meta tag,
allow GPU rasterization.
Could you move the new code to the top of this function, leaving the
early-return block as it was?  It's a bit confusing here because it has nothing
to do with some of the conditions like "!page()".

[hendrikw, 2015-02-28 04:32:37]

On 2015/02/28 01:05:05, aelias wrote:
> So, my point is that we need two modes:
> 
> - "force" should just unconditionally enable GPU raster, for development
> purposes
> - "enable" should just enable when it makes sense, so for example it shouldn't
> enable it on bad GPUs.
> 
> I assume the point of this change is to start preparing for a real launch of
GPU
> raster on desktop.  In that case, we need the ability to blacklist particular
> GPU models, when they prove to be crappy.  That won't work if the setting
> "enable" totally ignores the GPU blacklist.  Only "force" should do that.

Yeah, that's right, we'll use whatever mechanism we already use on android.  The
only difference between android and desktop is that we don't look at the
viewport meta tag on desktop.

[hendrikw, 2015-02-28 04:43:08]

On 2015/02/28 01:13:33, aelias wrote:
> OK, in any case, this Blink patch still looks mostly good to go, but I just
> wanted to make the point that one of your patch series should be removing
entry
> 99 from software_rendering_list_json.cc or there's something wrong here. 

Not exactly, this change will allow developers to test/debug the GPU
rasterization veto on desktop.

The entry to enable GPU rasterization is already in about:flags on desktop, but
it won't work correctly until this change goes in.

That said, it sounds like piman wants all of the webkit layout tests to run with
both GPU and software rasterization before the other change that this depends on
can go in, so I don't think I'll be landed the other change any time soon (and
as a result, this one won't be landing either) 

> Just one nit:
> 
>
https://codereview.chromium.org/792233007/diff/100001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/792233007/diff/100001/Source/web/WebViewImpl....
> Source/web/WebViewImpl.cpp:3368: // If we're not reading the viewport meta
tag,
> allow GPU rasterization.
> Could you move the new code to the top of this function, leaving the
> early-return block as it was?  It's a bit confusing here because it has
nothing
> to do with some of the conditions like "!page()".

Ok, I'll make the change.

[hendrikw, 2015-05-04 21:59:59]

On 2015/02/28 04:43:08, hendrikw wrote:
> On 2015/02/28 01:13:33, aelias wrote:
> > OK, in any case, this Blink patch still looks mostly good to go, but I just
> > wanted to make the point that one of your patch series should be removing
> entry
> > 99 from software_rendering_list_json.cc or there's something wrong here. 
> 
> Not exactly, this change will allow developers to test/debug the GPU
> rasterization veto on desktop.
> 
> The entry to enable GPU rasterization is already in about:flags on desktop,
but
> it won't work correctly until this change goes in.
> 
> That said, it sounds like piman wants all of the webkit layout tests to run
with
> both GPU and software rasterization before the other change that this depends
on
> can go in, so I don't think I'll be landed the other change any time soon (and
> as a result, this one won't be landing either) 
> 
> > Just one nit:
> > 
> >
>
https://codereview.chromium.org/792233007/diff/100001/Source/web/WebViewImpl.cpp
> > File Source/web/WebViewImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/792233007/diff/100001/Source/web/WebViewImpl....
> > Source/web/WebViewImpl.cpp:3368: // If we're not reading the viewport meta
> tag,
> > allow GPU rasterization.
> > Could you move the new code to the top of this function, leaving the
> > early-return block as it was?  It's a bit confusing here because it has
> nothing
> > to do with some of the conditions like "!page()".
> 
> Ok, I'll make the change.

Ok, precursor change is now in, and this is ready to land (and entry 99 is
gone), and I have other devs asking for this change in order to test stuff, are
we still good, can I commit?

[aelias_OOO_until_Jul13, 2015-05-04 22:58:55]

lgtm

[hendrikw, 2015-05-05 01:29:04]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-05-05 01:29:04]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/792233007/#ps160001
(title: "merge once again last time I swear")

[commit-bot: I haz the power, 2015-05-05 01:29:26]

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

[commit-bot: I haz the power, 2015-05-05 03:19:44]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194901