Add fixed raster scale use counter histograms.

Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  layer without will change has a fixed raster scale and the scale does
  not match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a non-will change layer that has a fixed
  raster scale and it has attempted to change its scale 5 times out of
  the last 10 frames.

R=danakj, chrishtr
BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/d6dce2606f1e2384484fc021cf926130bbd6840e
Cr-Commit-Position: refs/heads/master@{#394526}

[vmpstr, 2016-05-04 23:06:01]

Description was changed from

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  will change layer has a fixed raster scale and the scale does not
  match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a will change layer that has a fixed raster
  scale and it has attempted to change its scale 5 times out of the last
  10 frames.

BUG=609276
==========

to

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  will change layer has a fixed raster scale and the scale does not
  match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a will change layer that has a fixed raster
  scale and it has attempted to change its scale 5 times out of the last
  10 frames.

BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2016-05-04 23:40:27]

Description was changed from

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  will change layer has a fixed raster scale and the scale does not
  match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a will change layer that has a fixed raster
  scale and it has attempted to change its scale 5 times out of the last
  10 frames.

BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  will change layer has a fixed raster scale and the scale does not
  match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a will change layer that has a fixed raster
  scale and it has attempted to change its scale 5 times out of the last
  10 frames.

R=danakj, chrishtr
BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2016-05-04 23:40:27]

vmpstr@chromium.org changed reviewers:
+ chrishtr@chromium.org, danakj@chromium.org

[vmpstr, 2016-05-04 23:40:40]

Please take a look.

[danakj, 2016-05-04 23:42:50]

> FixedRasterScale_BlurryContent: this histogram records any time a
>  will change layer has a fixed raster scale and the scale does not
>  match the ideal scale (ie, blurry content)

I thought we want the opposite, a non-will-change layer?

[chrishtr, 2016-05-04 23:43:32]

On 2016/05/04 at 23:42:50, danakj wrote:
> > FixedRasterScale_BlurryContent: this histogram records any time a
> >  will change layer has a fixed raster scale and the scale does not
> >  match the ideal scale (ie, blurry content)
> 
> I thought we want the opposite, a non-will-change layer?

Right, the opposite.

[danakj, 2016-05-04 23:44:30]

> FixedRasterScale_PotentialPerformanceRegression: this histogram
>  records any time we have a will change layer that has a fixed raster
>  scale and it has attempted to change its scale 5 times out of the last
>  10 frames.

Ditto, non-will-change?

[vmpstr, 2016-05-04 23:53:24]

On 2016/05/04 23:44:30, danakj wrote:
> > FixedRasterScale_PotentialPerformanceRegression: this histogram
> >  records any time we have a will change layer that has a fixed raster
> >  scale and it has attempted to change its scale 5 times out of the last
> >  10 frames.
> 
> Ditto, non-will-change?

Right. I'll change it. I got confused by the bug.

[vmpstr, 2016-05-04 23:54:50]

Description was changed from

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  will change layer has a fixed raster scale and the scale does not
  match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a will change layer that has a fixed raster
  scale and it has attempted to change its scale 5 times out of the last
  10 frames.

R=danakj, chrishtr
BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  layer without will change has a fixed raster scale and the scale does
  not match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a non-will change layer that has a fixed
  raster scale and it has attempted to change its scale 5 times out of
  the last 10 frames.

R=danakj, chrishtr
BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2016-05-04 23:56:28]

ptal

[danakj, 2016-05-05 00:09:49]

https://codereview.chromium.org/1946403003/diff/20001/cc/blink/web_layer_impl.h
File cc/blink/web_layer_impl.h (right):

https://codereview.chromium.org/1946403003/diff/20001/cc/blink/web_layer_impl...
cc/blink/web_layer_impl.h:137: void setHasWillChangeTransformHint(bool
has_will_change) override;
group this with the rest, no need for whitespace. these should all be part of
the WebLayer implementation block.

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:511: has_will_change_transform_hint_ = has_will_change;
Don't you need to SetNeedsCommit? (which will SetNeedsPushProperties)

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:690: bool has_will_change_transform_hint_;
group this with the other bools

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:535: has_will_change_transform_hint_ = has_will_change;
You need to SetNeedsPushProperties?

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:679: 
no whitespace

https://codereview.chromium.org/1946403003/diff/40001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_client.h (right):

https://codereview.chromium.org/1946403003/diff/40001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_client.h:57: void
ReportFixedRasterScaleUseCounters(
this should be part of the LayerTreeHostClient impl block

https://codereview.chromium.org/1946403003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1946403003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:553:
host_impl->has_fixed_raster_scale_blurry_content();
So the pending tree won't have updated yet.. so we won't report pages that never
commit after the frame that causes this. Problem? Solution?

https://codereview.chromium.org/1946403003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1946403003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:656:
kFixedRasterScaleAttemptedScaleChangeThreshold = 5,
1 value per if you're doing this with enums.

enum {a=1}
enum {b=2}

[vmpstr, 2016-05-05 01:33:51]

PTAL

https://codereview.chromium.org/1946403003/diff/20001/cc/blink/web_layer_impl.h
File cc/blink/web_layer_impl.h (right):

https://codereview.chromium.org/1946403003/diff/20001/cc/blink/web_layer_impl...
cc/blink/web_layer_impl.h:137: void setHasWillChangeTransformHint(bool
has_will_change) override;
On 2016/05/05 00:09:49, danakj wrote:
> group this with the rest, no need for whitespace. these should all be part of
> the WebLayer implementation block.

Done.

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:511: has_will_change_transform_hint_ = has_will_change;
On 2016/05/05 00:09:49, danakj wrote:
> Don't you need to SetNeedsCommit? (which will SetNeedsPushProperties)

Done.

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:690: bool has_will_change_transform_hint_;
On 2016/05/05 00:09:49, danakj wrote:
> group this with the other bools

Done.

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:535: has_will_change_transform_hint_ = has_will_change;
On 2016/05/05 00:09:49, danakj wrote:
> You need to SetNeedsPushProperties?

Done.

https://codereview.chromium.org/1946403003/diff/20001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:679: 
On 2016/05/05 00:09:49, danakj wrote:
> no whitespace

Done.

https://codereview.chromium.org/1946403003/diff/40001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_client.h (right):

https://codereview.chromium.org/1946403003/diff/40001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_client.h:57: void
ReportFixedRasterScaleUseCounters(
On 2016/05/05 00:09:49, danakj wrote:
> this should be part of the LayerTreeHostClient impl block

Done.

https://codereview.chromium.org/1946403003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1946403003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:553:
host_impl->has_fixed_raster_scale_blurry_content();
On 2016/05/05 00:09:49, danakj wrote:
> So the pending tree won't have updated yet.. so we won't report pages that
never
> commit after the frame that causes this. Problem? Solution?

Fixed it by using BeginMainFrameState and assuming we'll have another commit.

https://codereview.chromium.org/1946403003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1946403003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:656:
kFixedRasterScaleAttemptedScaleChangeThreshold = 5,
On 2016/05/05 00:09:49, danakj wrote:
> 1 value per if you're doing this with enums.
> 
> enum {a=1}
> enum {b=2}

Done.

[chrishtr, 2016-05-05 20:33:07]

It should be easy to add a quick layout test for this that:

1. makes a translateZ layer
2. changes its transform
3. Checks for the use counters.

Example:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1946403003/diff/60001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1946403003/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.h:152: void
ReportFixedRasterScaleUseCounters(
I guess you already implemented it, but maybe overkill to support Blimp for
this?

https://codereview.chromium.org/1946403003/diff/60001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/1946403003/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.h:133: float last_ideal_source_scale_;
Brief comment about what "last" means?

https://codereview.chromium.org/1946403003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1946403003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1152:
FixedRasterScale_BlurryContent = 1334,
Skip the underscores. The recent entries that have them are not following the
typical Blink style.

https://codereview.chromium.org/1946403003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1946403003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:560:
m_graphicsLayer->setHasWillChangeTransformHint(m_owningLayer.layoutObject()->styleRef().hasWillChangeTransformHint());
Can you do a quick test of a translateZ layer that adds and removes will-change,
and verify that it executes this line of code?
There are some dirty bits on the compositing code that I want to make sure this
works correctly with.

[vmpstr, 2016-05-06 19:53:48]

PTAL, working on a layout test now.

https://codereview.chromium.org/1946403003/diff/60001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1946403003/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.h:152: void
ReportFixedRasterScaleUseCounters(
On 2016/05/05 20:33:07, chrishtr wrote:
> I guess you already implemented it, but maybe overkill to support Blimp for
> this?

It's just an empty implementation since the interface not requires it.

https://codereview.chromium.org/1946403003/diff/60001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/1946403003/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.h:133: float last_ideal_source_scale_;
On 2016/05/05 20:33:07, chrishtr wrote:
> Brief comment about what "last" means?

Done.

https://codereview.chromium.org/1946403003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1946403003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1152:
FixedRasterScale_BlurryContent = 1334,
On 2016/05/05 20:33:07, chrishtr wrote:
> Skip the underscores. The recent entries that have them are not following the
> typical Blink style.

Done.

https://codereview.chromium.org/1946403003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1946403003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:560:
m_graphicsLayer->setHasWillChangeTransformHint(m_owningLayer.layoutObject()->styleRef().hasWillChangeTransformHint());
On 2016/05/05 20:33:07, chrishtr wrote:
> Can you do a quick test of a translateZ layer that adds and removes
will-change,
> and verify that it executes this line of code?
> There are some dirty bits on the compositing code that I want to make sure
this
> works correctly with.

Yep, every will change update (with nothing else changing) hits this.

[vmpstr, 2016-05-06 22:31:15]

Added a layout test, please take a look.

[chrishtr, 2016-05-06 23:01:41]

lgtm

[danakj, 2016-05-10 22:05:30]

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1418:
base->set_has_will_change_transform_hint(has_will_change_transform_hint_);
move this up with the other calls to base->setfoo. line 1385?

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1512: has_will_change_transform_hint_ =
base.has_will_change_transform_hint();
ditto move this up with other like things. line 1465?

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:504:
layer->SetHasWillChangeTransformHint(has_will_change_transform_hint());
move this up with other simple setters. line 477?

https://codereview.chromium.org/1946403003/diff/140001/cc/proto/layer.proto
File cc/proto/layer.proto (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/proto/layer.proto#n...
cc/proto/layer.proto:126: optional bool has_will_change_transform_hint = 48;
move up with other simple properties. line 112?

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:552: has_fixed_raster_scale_blurry_content_ =
What happened to sending them in begin main frame?

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:972: has_fixed_raster_scale_blurry_content_,
whats going on here? receive vars as args, but use members of LTH instead?

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:358: // Advance the attempted scale change
history before updaring draw properties.
updating

[vmpstr, 2016-05-13 00:13:35]

PTAL

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1418:
base->set_has_will_change_transform_hint(has_will_change_transform_hint_);
On 2016/05/10 22:05:30, danakj wrote:
> move this up with the other calls to base->setfoo. line 1385?

Done.

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1512: has_will_change_transform_hint_ =
base.has_will_change_transform_hint();
On 2016/05/10 22:05:30, danakj wrote:
> ditto move this up with other like things. line 1465?

Done.

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:504:
layer->SetHasWillChangeTransformHint(has_will_change_transform_hint());
On 2016/05/10 22:05:30, danakj wrote:
> move this up with other simple setters. line 477?

Done.

https://codereview.chromium.org/1946403003/diff/140001/cc/proto/layer.proto
File cc/proto/layer.proto (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/proto/layer.proto#n...
cc/proto/layer.proto:126: optional bool has_will_change_transform_hint = 48;
On 2016/05/10 22:05:30, danakj wrote:
> move up with other simple properties. line 112?

I kind of prefer if this file's fields remained in numerical order, but I can
see that it's not that already... Done and filed a bug to fix all of this.

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:552: has_fixed_raster_scale_blurry_content_ =
On 2016/05/10 22:05:30, danakj wrote:
> What happened to sending them in begin main frame?

Leftover code from the previous patch. Removed.

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:972: has_fixed_raster_scale_blurry_content_,
On 2016/05/10 22:05:30, danakj wrote:
> whats going on here? receive vars as args, but use members of LTH instead?

Fixed.

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1946403003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:358: // Advance the attempted scale change
history before updaring draw properties.
On 2016/05/10 22:05:30, danakj wrote:
> updating

Done.

[danakj, 2016-05-13 20:41:48]

LGTM

https://codereview.chromium.org/1946403003/diff/180001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1946403003/diff/180001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:443: std::numeric_limits<float>::epsilon()) {
Why?

https://codereview.chromium.org/1946403003/diff/180001/cc/trees/proxy_common.cc
File cc/trees/proxy_common.cc (right):

https://codereview.chromium.org/1946403003/diff/180001/cc/trees/proxy_common....
cc/trees/proxy_common.cc:13: : memory_allocation_limit_bytes(0),
while you're here... mind moving all this init sadness to the definitions to be
less sadness?

[vmpstr, 2016-05-13 21:17:10]

https://codereview.chromium.org/1946403003/diff/180001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1946403003/diff/180001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:443: std::numeric_limits<float>::epsilon()) {
On 2016/05/13 20:41:47, danakj wrote:
> Why?

Acknowledged.

https://codereview.chromium.org/1946403003/diff/180001/cc/trees/proxy_common.cc
File cc/trees/proxy_common.cc (right):

https://codereview.chromium.org/1946403003/diff/180001/cc/trees/proxy_common....
cc/trees/proxy_common.cc:13: : memory_allocation_limit_bytes(0),
On 2016/05/13 20:41:47, danakj wrote:
> while you're here... mind moving all this init sadness to the definitions to
be
> less sadness?

Done.

[vmpstr, 2016-05-13 21:19:13]

vmpstr@chromium.org changed reviewers:
+ esprehn@chromium.org, isherman@chromium.org, piman@chromium.org

[vmpstr, 2016-05-13 21:19:14]

Adding owners:
+piman for content
+esprehn for webkit
+isherman for histograms

Please take a look.

[chrishtr, 2016-05-14 08:14:40]

I'm a WebKit owner, no additional approvals needed.

[esprehn, 2016-05-16 04:59:24]

lgtm, but please note that this is not really what we're trying to do with Onion
Soup. cc shouldn't need to be involved for this, we can track all the knowledge
inside blink and report the metrics. The web/ layer in general will be going
away, and blink will need to be built on top of primitives and lego blocks
exposed by cc/.

https://codereview.chromium.org/1946403003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1946403003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:2814: UseCounter::count(document,
UseCounter::FixedRasterScalePotentialPerformanceRegression);
Why can't we just handle this inside blink? Having the embedder call into blink
for a UseCounter is not really what we want with Onion Soup.

[piman, 2016-05-16 17:43:13]

lgtm

[danakj, 2016-05-16 18:27:11]

On Sun, May 15, 2016 at 9:59 PM, <esprehn@chromium.org> wrote:

> lgtm, but please note that this is not really what we're trying to do with
> Onion
> Soup. cc shouldn't need to be involved for this, we can track all the
> knowledge
> inside blink and report the metrics. The web/ layer in general will be
> going
> away, and blink will need to be built on top of primitives and lego blocks
> exposed by cc/.
>

In this case we're use-counting decisions made by cc, so blink can't tell.
In a perfect onion soup world, I think this still comes out of cc via
LayerTreeHostClient. Maybe there's less onion layers between that and
WebViewImpl or whatever lives in blink though, which would be nice.


>
>
>
>
https://codereview.chromium.org/1946403003/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
>
>
>
https://codereview.chromium.org/1946403003/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebViewImpl.cpp:2814:
> UseCounter::count(document,
> UseCounter::FixedRasterScalePotentialPerformanceRegression);
> Why can't we just handle this inside blink? Having the embedder call
> into blink for a UseCounter is not really what we want with Onion Soup.
>
> https://codereview.chromium.org/1946403003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2016-05-16 18:27:11]

On Sun, May 15, 2016 at 9:59 PM, <esprehn@chromium.org> wrote:

> lgtm, but please note that this is not really what we're trying to do with
> Onion
> Soup. cc shouldn't need to be involved for this, we can track all the
> knowledge
> inside blink and report the metrics. The web/ layer in general will be
> going
> away, and blink will need to be built on top of primitives and lego blocks
> exposed by cc/.
>

In this case we're use-counting decisions made by cc, so blink can't tell.
In a perfect onion soup world, I think this still comes out of cc via
LayerTreeHostClient. Maybe there's less onion layers between that and
WebViewImpl or whatever lives in blink though, which would be nice.


>
>
>
>
https://codereview.chromium.org/1946403003/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
>
>
>
https://codereview.chromium.org/1946403003/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebViewImpl.cpp:2814:
> UseCounter::count(document,
> UseCounter::FixedRasterScalePotentialPerformanceRegression);
> Why can't we just handle this inside blink? Having the embedder call
> into blink for a UseCounter is not really what we want with Onion Soup.
>
> https://codereview.chromium.org/1946403003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2016-05-16 18:37:05]

vmpstr@chromium.org changed reviewers:
+ asvitkine@chromium.org, nyquist@chromium.org

[vmpstr, 2016-05-16 18:37:06]

+asvitkine@ for histograms
+nyquist@ for blimp

[Alexei Svitkine (slow), 2016-05-16 18:48:01]

histograms lgtm

[nyquist, 2016-05-16 21:16:55]

blimp lgtm

[Ilya Sherman, 2016-05-17 18:19:49]

isherman@chromium.org changed reviewers:
- isherman@chromium.org

[vmpstr, 2016-05-18 18:27:08]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-05-18 18:27:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, danakj@chromium.org, esprehn@chromium.org,
piman@chromium.org, nyquist@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1946403003/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-18 18:27:49]

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

[commit-bot: I haz the power, 2016-05-18 18:42:40]

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

[commit-bot: I haz the power, 2016-05-18 18:42:42]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[vmpstr, 2016-05-18 18:54:18]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-05-18 18:54:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, esprehn@chromium.org, piman@chromium.org,
danakj@chromium.org, nyquist@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1946403003/#ps240001
(title: "rebase+fix")

[commit-bot: I haz the power, 2016-05-18 18:55:12]

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

[commit-bot: I haz the power, 2016-05-18 20:19:17]

Description was changed from

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  layer without will change has a fixed raster scale and the scale does
  not match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a non-will change layer that has a fixed
  raster scale and it has attempted to change its scale 5 times out of
  the last 10 frames.

R=danakj, chrishtr
BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  layer without will change has a fixed raster scale and the scale does
  not match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a non-will change layer that has a fixed
  raster scale and it has attempted to change its scale 5 times out of
  the last 10 frames.

R=danakj, chrishtr
BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-18 20:19:19]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-18 20:21:29]

Description was changed from

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  layer without will change has a fixed raster scale and the scale does
  not match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a non-will change layer that has a fixed
  raster scale and it has attempted to change its scale 5 times out of
  the last 10 frames.

R=danakj, chrishtr
BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add fixed raster scale use counter histograms.

This patch adds two new values for WebCore.FeatureObserver histogram:
- FixedRasterScale_BlurryContent: this histogram records any time a
  layer without will change has a fixed raster scale and the scale does
  not match the ideal scale (ie, blurry content)
- FixedRasterScale_PotentialPerformanceRegression: this histogram
  records any time we have a non-will change layer that has a fixed
  raster scale and it has attempted to change its scale 5 times out of
  the last 10 frames.

R=danakj, chrishtr
BUG=609276
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/d6dce2606f1e2384484fc021cf926130bbd6840e
Cr-Commit-Position: refs/heads/master@{#394526}
==========

[commit-bot: I haz the power, 2016-05-18 20:21:32]

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