Enable disabling WebCore::GraphicsContext in telemetry.

Enable disabling WebCore::GraphicsContext in telemetry.

Plumb a flag through from telemetry tests to the layer
painting code that disables the WebCore::GraphicsContext.
This allows us to isolate the various systems that paint
web content in Chromium.

R=nduca@chromium.org,skyostil@chromium.org
BUG=350684
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267913

[Stephen Chennney, 2014-04-28 20:09:04]

Can't land anything like this until Blink changes land. Also, don't know why
.gitmodules is in here.

[Sami, 2014-04-29 10:57:23]

Thanks Stephen. I think this looks okay functionally. Just a few stylistic nits
below.

https://codereview.chromium.org/253013003/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/253013003/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:24: bool disableContext = false) = 0;
hacker_style names please. Maybe the name could also be a little less opaque
like disable_graphics_context? Alternatively if that seems too verbose, let's
add a comment here saying what disable_context means.

https://codereview.chromium.org/253013003/diff/20001/cc/layers/picture_image_...
File cc/layers/picture_image_layer.cc (right):

https://codereview.chromium.org/253013003/diff/20001/cc/layers/picture_image_...
cc/layers/picture_image_layer.cc:47: if (!canvas || !bitmap_.width() ||
!bitmap_.height())
I think we can now also remove all the null checks against canvas since I only
added them for disabling painting.

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:264: bool disableGraphicsContext = false;
hacker_style here too.

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:274: // We pass a disable flag through the paint calls
nit: end comments with punctuation.

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture_uni...
File cc/resources/picture_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture_uni...
cc/resources/picture_unittest.cc:483:
EXPECT_TRUE(content_layer_client.last_canvas() != NULL);
Could we also check that content_layer_client was told to disable the graphics
context?

[Stephen Chennney, 2014-04-29 20:30:44]

I think I've addressed everything. To fix the tests build I needed a lot more
stuff.

https://codereview.chromium.org/253013003/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/253013003/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:24: bool disableContext = false) = 0;
On 2014/04/29 10:57:23, Sami wrote:
> hacker_style names please. Maybe the name could also be a little less opaque
> like disable_graphics_context? Alternatively if that seems too verbose, let's
> add a comment here saying what disable_context means.

Done.

https://codereview.chromium.org/253013003/diff/20001/cc/layers/picture_image_...
File cc/layers/picture_image_layer.cc (right):

https://codereview.chromium.org/253013003/diff/20001/cc/layers/picture_image_...
cc/layers/picture_image_layer.cc:47: if (!canvas || !bitmap_.width() ||
!bitmap_.height())
On 2014/04/29 10:57:23, Sami wrote:
> I think we can now also remove all the null checks against canvas since I only
> added them for disabling painting.

Done.

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:264: bool disableGraphicsContext = false;
On 2014/04/29 10:57:23, Sami wrote:
> hacker_style here too.

Done.

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:274: // We pass a disable flag through the paint calls
On 2014/04/29 10:57:23, Sami wrote:
> nit: end comments with punctuation.

Done.

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture_uni...
File cc/resources/picture_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture_uni...
cc/resources/picture_unittest.cc:483:
EXPECT_TRUE(content_layer_client.last_canvas() != NULL);
On 2014/04/29 10:57:23, Sami wrote:
> Could we also check that content_layer_client was told to disable the graphics
> context?

There is no way to get to the context to ask it if it is disables. We could
maybe check that nothing has been drawn, but that is dangerous as some other
code might draw into the canvas outside of GraphicsContext.

[Sami, 2014-04-29 20:47:47]

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture_uni...
File cc/resources/picture_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/20001/cc/resources/picture_uni...
cc/resources/picture_unittest.cc:483:
EXPECT_TRUE(content_layer_client.last_canvas() != NULL);
On 2014/04/29 20:30:45, Stephen Chenney wrote:
> There is no way to get to the context to ask it if it is disables. We could
> maybe check that nothing has been drawn, but that is dangerous as some other
> code might draw into the canvas outside of GraphicsContext.

I was more thinking of extending cc's FakeContentLayerClient to keep track of
whether it was asked to disable the context or not.

[Stephen Chennney, 2014-04-30 19:05:08]

OK. Ready to go, I think.

[Sami, 2014-05-01 16:32:52]

Thanks Stephen, 1gtm modulo a few comments. I'll let Nat (or danakj or enne) to
approve for realz.

https://codereview.chromium.org/253013003/diff/100001/cc/layers/content_layer...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/253013003/diff/100001/cc/layers/content_layer...
cc/layers/content_layer_client.h:22: GraphicsContextEnabled,
nit: Swap the order of these two so that enabled = 1?

https://codereview.chromium.org/253013003/diff/100001/cc/layers/content_layer...
cc/layers/content_layer_client.h:31: graphics_context_state =
GraphicsContextEnabled) = 0;
Chromium style prohibits default parameters, but it looks like this is always
passed in so we could just make it required?

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture_un...
File cc/resources/picture_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture_un...
cc/resources/picture_unittest.cc:489:
ContentLayerClient::GraphicsContextDisabled);
Thanks for improving the test.

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_masks.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_masks.cc:34: graphics_context_status =
ContentLayerClient::GraphicsContextEnabled)
Ditto about default parameters.

https://codereview.chromium.org/253013003/diff/100001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_content_layer_impl.h (right):

https://codereview.chromium.org/253013003/diff/100001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_content_layer_impl.h:44:
graphics_context_status = ContentLayerClient::GraphicsContextEnabled)
Ditto about default parameters.

[danakj, 2014-05-01 17:07:11]

https://codereview.chromium.org/253013003/diff/100001/cc/layers/content_layer...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/253013003/diff/100001/cc/layers/content_layer...
cc/layers/content_layer_client.h:22: GraphicsContextEnabled,
On 2014/05/01 16:32:53, Sami wrote:
> nit: Swap the order of these two so that enabled = 1?

+1

https://codereview.chromium.org/253013003/diff/100001/cc/layers/picture_image...
File cc/layers/picture_image_layer.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/layers/picture_image...
cc/layers/picture_image_layer.cc:48: if (!bitmap_.width() || !bitmap_.height())
Do you want to early out here based on the GraphicsContextStatus instead of the
null canvas? Or is removing the early out intentional?

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture_un...
File cc/resources/picture_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture_un...
cc/resources/picture_unittest.cc:466:
EXPECT_TRUE(content_layer_client.last_context_status() ==
nit: EXPECT_EQ(expected,actual) here and below?

https://codereview.chromium.org/253013003/diff/100001/cc/test/fake_content_la...
File cc/test/fake_content_layer_client.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/test/fake_content_la...
cc/test/fake_content_layer_client.cc:25: if (!canvas)
can get rid of this early out?

https://codereview.chromium.org/253013003/diff/100001/cc/test/solid_color_con...
File cc/test/solid_color_content_layer_client.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/test/solid_color_con...
cc/test/solid_color_content_layer_client.cc:19: if (!canvas)
can get rid of this early out now?

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:1244: SkCanvas*,
mind giving these variables names while you're here? looks like we missed these
in the blink->chromium style change

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2531:
ContentLayerClient::GraphicsContextStatus) OVERRIDE {
please give the variable a name always

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2774:
ContentLayerClient::GraphicsContextStatus) OVERRIDE {
and here, etc.

https://codereview.chromium.org/253013003/diff/100001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/253013003/diff/100001/ui/compositor/layer.h#n...
ui/compositor/layer.h:321: ContentLayerClient::GraphicsContextStatus) OVERRIDE;
needs a var name also

[danakj, 2014-05-01 17:07:50]

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture.cc...
cc/resources/picture.cc:299: if (canvas) {
This if() can go away too?

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture.cc...
cc/resources/picture.cc:315: if (canvas)
and this?

[Stephen Chennney, 2014-05-01 19:12:25]

Thanks for the thorough reviews. I'll get the next patch up once I verify it
builds.

https://codereview.chromium.org/253013003/diff/100001/cc/layers/content_layer...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/253013003/diff/100001/cc/layers/content_layer...
cc/layers/content_layer_client.h:22: GraphicsContextEnabled,
On 2014/05/01 17:07:11, danakj wrote:
> On 2014/05/01 16:32:53, Sami wrote:
> > nit: Swap the order of these two so that enabled = 1?
> 
> +1

Done.
And also switched to Chromium style for enums.

https://codereview.chromium.org/253013003/diff/100001/cc/layers/content_layer...
cc/layers/content_layer_client.h:31: graphics_context_state =
GraphicsContextEnabled) = 0;
On 2014/05/01 16:32:53, Sami wrote:
> Chromium style prohibits default parameters, but it looks like this is always
> passed in so we could just make it required?

Done.

Means changing more files, but that's life.

https://codereview.chromium.org/253013003/diff/100001/cc/layers/picture_image...
File cc/layers/picture_image_layer.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/layers/picture_image...
cc/layers/picture_image_layer.cc:48: if (!bitmap_.width() || !bitmap_.height())
On 2014/05/01 17:07:11, danakj wrote:
> Do you want to early out here based on the GraphicsContextStatus instead of
the
> null canvas? Or is removing the early out intentional?

Sami added the !canvas check during the previous attempt at solving this
problem. Originally, and again now, there is no way this is called with a null
canvas, and as compositor code we are not concerned with preventing drawing here
for telemetry.

Having said that, would you prefer to keep the canvas check?

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture.cc...
cc/resources/picture.cc:299: if (canvas) {
On 2014/05/01 17:07:50, danakj wrote:
> This if() can go away too?

Yep, done. I also think we should use the null canvas when drawing painting
disabled, to make the work done with PAINTING_DISABLED a strict subset of the
NULL_CANVAS situation.

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture.cc...
cc/resources/picture.cc:315: if (canvas)
On 2014/05/01 17:07:50, danakj wrote:
> and this?

Done.

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture_un...
File cc/resources/picture_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/resources/picture_un...
cc/resources/picture_unittest.cc:466:
EXPECT_TRUE(content_layer_client.last_context_status() ==
On 2014/05/01 17:07:11, danakj wrote:
> nit: EXPECT_EQ(expected,actual) here and below?

Done.

https://codereview.chromium.org/253013003/diff/100001/cc/test/fake_content_la...
File cc/test/fake_content_layer_client.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/test/fake_content_la...
cc/test/fake_content_layer_client.cc:25: if (!canvas)
On 2014/05/01 17:07:11, danakj wrote:
> can get rid of this early out?

Yes, done.

https://codereview.chromium.org/253013003/diff/100001/cc/test/solid_color_con...
File cc/test/solid_color_content_layer_client.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/test/solid_color_con...
cc/test/solid_color_content_layer_client.cc:19: if (!canvas)
On 2014/05/01 17:07:11, danakj wrote:
> can get rid of this early out now?

Yes, done.

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_masks.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_masks.cc:34: graphics_context_status =
ContentLayerClient::GraphicsContextEnabled)
On 2014/05/01 16:32:53, Sami wrote:
> Ditto about default parameters.

Done.

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:1244: SkCanvas*,
On 2014/05/01 17:07:11, danakj wrote:
> mind giving these variables names while you're here? looks like we missed
these
> in the blink->chromium style change

I finally found the reference to this in the Google style guide. I'll fix all my
violations and methods that I touch.

Done.

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2531:
ContentLayerClient::GraphicsContextStatus) OVERRIDE {
On 2014/05/01 17:07:11, danakj wrote:
> please give the variable a name always

Done.

https://codereview.chromium.org/253013003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2774:
ContentLayerClient::GraphicsContextStatus) OVERRIDE {
On 2014/05/01 17:07:11, danakj wrote:
> and here, etc.

Done.

https://codereview.chromium.org/253013003/diff/100001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/253013003/diff/100001/ui/compositor/layer.h#n...
ui/compositor/layer.h:321: ContentLayerClient::GraphicsContextStatus) OVERRIDE;
On 2014/05/01 17:07:11, danakj wrote:
> needs a var name also

Done.

https://codereview.chromium.org/253013003/diff/100001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_content_layer_impl.h (right):

https://codereview.chromium.org/253013003/diff/100001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_content_layer_impl.h:44:
graphics_context_status = ContentLayerClient::GraphicsContextEnabled)
On 2014/05/01 16:32:53, Sami wrote:
> Ditto about default parameters.

Done.

[danakj, 2014-05-01 19:15:14]

https://codereview.chromium.org/253013003/diff/100001/cc/layers/picture_image...
File cc/layers/picture_image_layer.cc (right):

https://codereview.chromium.org/253013003/diff/100001/cc/layers/picture_image...
cc/layers/picture_image_layer.cc:48: if (!bitmap_.width() || !bitmap_.height())
On 2014/05/01 19:12:26, Stephen Chenney wrote:
> On 2014/05/01 17:07:11, danakj wrote:
> > Do you want to early out here based on the GraphicsContextStatus instead of
> the
> > null canvas? Or is removing the early out intentional?
> 
> Sami added the !canvas check during the previous attempt at solving this
> problem. Originally, and again now, there is no way this is called with a null
> canvas, and as compositor code we are not concerned with preventing drawing
here
> for telemetry.
> 
> Having said that, would you prefer to keep the canvas check?

No, I don't want to early out on canvas if it's never null. I just want to make
sure you want this layer to be drawing its bitmap into the canvas when
GraphicsContextDisabled.

[Stephen Chennney, 2014-05-01 20:10:37]

On 2014/05/01 19:15:14, danakj wrote:
>
https://codereview.chromium.org/253013003/diff/100001/cc/layers/picture_image...
> File cc/layers/picture_image_layer.cc (right):
> 
>
https://codereview.chromium.org/253013003/diff/100001/cc/layers/picture_image...
> cc/layers/picture_image_layer.cc:48: if (!bitmap_.width() ||
!bitmap_.height())
> On 2014/05/01 19:12:26, Stephen Chenney wrote:
> > On 2014/05/01 17:07:11, danakj wrote:
> > > Do you want to early out here based on the GraphicsContextStatus instead
of
> > the
> > > null canvas? Or is removing the early out intentional?
> > 
> > Sami added the !canvas check during the previous attempt at solving this
> > problem. Originally, and again now, there is no way this is called with a
null
> > canvas, and as compositor code we are not concerned with preventing drawing
> here
> > for telemetry.
> > 
> > Having said that, would you prefer to keep the canvas check?
> 
> No, I don't want to early out on canvas if it's never null. I just want to
make
> sure you want this layer to be drawing its bitmap into the canvas when
> GraphicsContextDisabled.

OK. I changed it so that we use the null SKCanvas for the painting disabled case
too, so we will end up not doing any work.

I think this patch is good to go, but it can't land until Blink rolls past
r173096.

[danakj, 2014-05-01 20:31:56]

LGTM

https://codereview.chromium.org/253013003/diff/120001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/253013003/diff/120001/cc/resources/picture.cc...
cc/resources/picture.cc:287: // We pass a disable flag through the paint calls.
this comment says what, not why. can you either make it say why or remove it?

https://codereview.chromium.org/253013003/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2531:
ContentLayerClient::GraphicsContextStatus) OVERRIDE {
variable name missed here

https://codereview.chromium.org/253013003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2774:
ContentLayerClient::GraphicsContextStatus) OVERRIDE {
and here

[Stephen Chennney, 2014-05-02 18:40:44]

Thanks Dana.

https://codereview.chromium.org/253013003/diff/120001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/253013003/diff/120001/cc/resources/picture.cc...
cc/resources/picture.cc:287: // We pass a disable flag through the paint calls.
On 2014/05/01 20:31:57, danakj wrote:
> this comment says what, not why. can you either make it say why or remove it?

Done.

https://codereview.chromium.org/253013003/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/253013003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2531:
ContentLayerClient::GraphicsContextStatus) OVERRIDE {
On 2014/05/01 20:31:57, danakj wrote:
> variable name missed here

Done.

https://codereview.chromium.org/253013003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2774:
ContentLayerClient::GraphicsContextStatus) OVERRIDE {
On 2014/05/01 20:31:57, danakj wrote:
> and here

Done.

[Stephen Chennney, 2014-05-02 18:40:51]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-05-02 18:41:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/schenney@chromium.org/253013003/130001

[commit-bot: I haz the power, 2014-05-02 21:03:10]

Change committed as 267913