[Slimming Paint] Move PageWidgetDelegate scale to a transform display item.

[Slimming Paint] Move PageWidgetDelegate scale to a transform display item.

GraphicsContext::scale accesses the underlying canvas, which may not
exist.

BUG=450761
TEST=None yet (but this was discovered through unit tests in an upcoming patch)

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

[jbroman, 2015-01-27 22:02:27]

jbroman@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org, wangxianzhu@chromium.org

[jbroman, 2015-01-27 22:02:28]

This is one of the issues I ran into with my unit tests. Fix seems simple, but
please make sure I'm using the correct display item ID -- I'm not confident
about that.

https://codereview.chromium.org/879993006/diff/1/Source/web/PageWidgetDelegat...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/879993006/diff/1/Source/web/PageWidgetDelegat...
Source/web/PageWidgetDelegate.cpp:91: TransformRecorder
scaleRecorder(*graphicsContext, root.displayItemClient(), scale);
Is it fine to use the LocalFrame for this display item? I think so, and I think
invalidation is probably fine for the same reason other uses of it here are
(because the LocalFrame outlives its GraphicsLayers?).

TransformRecorder has a hardcoded DisplayItem::Type, but I can't find anywhere
else it would conflict with. Is this fine, or is there some policy describing
what DisplayItem::Type should be used here?

[chrishtr, 2015-01-28 01:46:41]

On 2015/01/27 at 22:02:28, jbroman wrote:
> This is one of the issues I ran into with my unit tests. Fix seems simple, but
please make sure I'm using the correct display item ID -- I'm not confident
about that.
> 
>
https://codereview.chromium.org/879993006/diff/1/Source/web/PageWidgetDelegat...
> File Source/web/PageWidgetDelegate.cpp (right):
> 
>
https://codereview.chromium.org/879993006/diff/1/Source/web/PageWidgetDelegat...
> Source/web/PageWidgetDelegate.cpp:91: TransformRecorder
scaleRecorder(*graphicsContext, root.displayItemClient(), scale);
> Is it fine to use the LocalFrame for this display item? I think so, and I
think invalidation is probably fine for the same reason other uses of it here
are (because the LocalFrame outlives its GraphicsLayers?).

It is ok, but as we discussed yesterday, we need likely invalidation logic when
the object is destroyed, so that stale pointers do not remain in the display
list cache. Did you look into that aspect? If not let's get a bug filed to track
the problem.

> 
> TransformRecorder has a hardcoded DisplayItem::Type, but I can't find anywhere
else it would conflict with. Is this fine, or is there some policy describing
what DisplayItem::Type should be used here?

[chrishtr, 2015-01-28 01:50:21]

https://codereview.chromium.org/879993006/diff/1/Source/web/PageWidgetDelegat...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/879993006/diff/1/Source/web/PageWidgetDelegat...
Source/web/PageWidgetDelegate.cpp:91: TransformRecorder
scaleRecorder(*graphicsContext, root.displayItemClient(), scale);
On 2015/01/27 at 22:02:28, jbroman wrote:
> Is it fine to use the LocalFrame for this display item? I think so, and I
think invalidation is probably fine for the same reason other uses of it here
are (because the LocalFrame outlives its GraphicsLayers?).
> 
> TransformRecorder has a hardcoded DisplayItem::Type, but I can't find anywhere
else it would conflict with. Is this fine, or is there some policy describing
what DisplayItem::Type should be used here?

It's fine. The only requirement is that the (DisplayItemClient,
DisplayItem::Type) pair be unique. But for readability the type should be chosen
to make sense, and not be one of the DrawwPaintPase* enum values unless it's
actually the drawing item for that phase of the official web painting algorithm.

(There are other cases where we actually, and incorrectly, violate that, in
particular cases with multiple types of clip on the same object.

[chrishtr, 2015-01-28 01:50:26]

lgtm

[jbroman, 2015-02-02 16:34:04]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2015-02-02 16:34:11]

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

[commit-bot: I haz the power, 2015-02-02 16:37:32]

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