Refactor LayoutView paint offset updates out of FrameView update code

Refactor LayoutView paint offset updates out of FrameView update code

Root layer scrolls lets us treat LayoutView like other LayoutObjects
and lets us move property updates into the regular LayoutObject update
code. This patch removes a special-case for LayoutView in the FrameView
property update. Comments have been added describing the LayoutView
context updates for fixed and absolute cases.

Additionally, LayoutView tests have been unskipped to ensure this
does not regress.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[pdr., 2016-10-31 22:48:16]

Description was changed from

==========
Refactor LayoutView paint offset updates out of FrameView update code

BUG=645667
==========

to

==========
Refactor LayoutView paint offset updates out of FrameView update code

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-11-01 21:32:44]

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

[pdr., 2016-11-01 21:32:50]

pdr@chromium.org changed reviewers:
+ trchen@chromium.org

[commit-bot: I haz the power, 2016-11-01 21:33:48]

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

[trchen, 2016-11-01 22:14:00]

https://codereview.chromium.org/2464823003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2464823003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:113:
context.current.renderingContextID = 0;
Alternative idea:
Can we add
context.current.paintOffset.moveBy(frameView.location());

Then force usesPaintOffsetTranslation = true in updatePaintOffsetTranslation()
below? This way we won't need to special case for LayoutView. Don't worry about
the snapping differences. paintOffset are expected to be integer at this point.

https://codereview.chromium.org/2464823003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:187:
PaintPropertyTreeBuilderContext& context) {
DCHECK(RuntimeEnabledFeatures::rootLayerScrollingEnabled());

https://codereview.chromium.org/2464823003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:192:
frameView->y() + view.location().y() + context.current.paintOffset.y());
deriveBorderBoxFromContainerContext() should have already accumulated
view.location() to context.current.paintOffset. I think we only need

translate.translate(
    frameView->x() + context.current.paintOffset.x(),
    frameView->y() + context.current.paintOffset.y());

https://codereview.chromium.org/2464823003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:215: bool
usesPaintOffsetTranslation = false;
Alternative idea:
if (RuntimeEnabledFeatures::rootLayerScrollingEnabled() &&
object.isLayoutView())
  usesPaintOffsetTranslation = true;
else if (object.isBoxModelObject() &&

[commit-bot: I haz the power, 2016-11-01 23:15:38]

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

[commit-bot: I haz the power, 2016-11-01 23:15:39]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-11-01 23:28:23]

Both of the ideas were quite clever. Thanks for the detailed review.

Implemented both and all tests still pass.

https://codereview.chromium.org/2464823003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2464823003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:113:
context.current.renderingContextID = 0;
On 2016/11/01 at 22:14:00, trchen wrote:
> Alternative idea:
> Can we add
> context.current.paintOffset.moveBy(frameView.location());
> 
> Then force usesPaintOffsetTranslation = true in updatePaintOffsetTranslation()
below? This way we won't need to special case for LayoutView. Don't worry about
the snapping differences. paintOffset are expected to be integer at this point.

Done.

https://codereview.chromium.org/2464823003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:187:
PaintPropertyTreeBuilderContext& context) {
On 2016/11/01 at 22:14:00, trchen wrote:
> DCHECK(RuntimeEnabledFeatures::rootLayerScrollingEnabled());

Done

https://codereview.chromium.org/2464823003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:192:
frameView->y() + view.location().y() + context.current.paintOffset.y());
On 2016/11/01 at 22:14:00, trchen wrote:
> deriveBorderBoxFromContainerContext() should have already accumulated
view.location() to context.current.paintOffset. I think we only need
> 
> translate.translate(
>     frameView->x() + context.current.paintOffset.x(),
>     frameView->y() + context.current.paintOffset.y());

Done.

[pdr., 2016-11-01 23:28:54]

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

[commit-bot: I haz the power, 2016-11-01 23:29:27]

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

[trchen, 2016-11-01 23:36:25]

lgtm

https://codereview.chromium.org/2464823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2464823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:210:
object.isLayoutView()) {
Is the special case still needed?

It looks like there are subtle differences in flattening and fixed-pos context
bookkeeping. I'm okay with it if not special casing makes it too awkward.

[commit-bot: I haz the power, 2016-11-02 01:10:01]

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

[commit-bot: I haz the power, 2016-11-02 01:10:01]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-11-02 02:24:17]

Description was changed from

==========
Refactor LayoutView paint offset updates out of FrameView update code

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Refactor LayoutView paint offset updates out of FrameView update code

Root layer scrolls lets us treat LayoutView like other LayoutObjects
and lets us move property updates into the regular LayoutObject update
code. This patch removes a special-case for LayoutView in the FrameView
property update. Comments have been added describing the LayoutView
context updates for fixed and absolute cases.

Additionally, LayoutView tests have been unskipped to ensure this
does not regress.

BUG=645667

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-11-02 02:26:09]

On 2016/11/01 at 23:36:25, trchen wrote:
> lgtm
> 
>
https://codereview.chromium.org/2464823003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2464823003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:210:
object.isLayoutView()) {
> Is the special case still needed?
> 
> It looks like there are subtle differences in flattening and fixed-pos context
bookkeeping. I'm okay with it if not special casing makes it too awkward.

Updated!

Could you take one more look? Please CQ if it looks good.

[pdr., 2016-11-02 02:26:15]

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

[commit-bot: I haz the power, 2016-11-02 02:26:34]

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

[trchen, 2016-11-02 02:29:47]

On 2016/11/02 02:26:09, pdr. wrote:
> On 2016/11/01 at 23:36:25, trchen wrote:
https://codereview.chromium.org/2464823003/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:210:
> object.isLayoutView()) {
> > Is the special case still needed?
> > 
> > It looks like there are subtle differences in flattening and fixed-pos
context
> bookkeeping. I'm okay with it if not special casing makes it too awkward.
> 
> Updated!
> 
> Could you take one more look? Please CQ if it looks good.

Yes it is exactly what I meant. (Clicking CQ)

[trchen, 2016-11-02 02:29:51]

The CQ bit was unchecked by trchen@chromium.org

[trchen, 2016-11-02 02:29:56]

The CQ bit was checked by trchen@chromium.org

[trchen, 2016-11-02 02:29:56]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2464823003/#ps120001
(title: "Remove unnecessary forward decl")

[commit-bot: I haz the power, 2016-11-02 02:30:22]

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

[commit-bot: I haz the power, 2016-11-02 05:24:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-02 05:28:15]

Description was changed from

==========
Refactor LayoutView paint offset updates out of FrameView update code

Root layer scrolls lets us treat LayoutView like other LayoutObjects
and lets us move property updates into the regular LayoutObject update
code. This patch removes a special-case for LayoutView in the FrameView
property update. Comments have been added describing the LayoutView
context updates for fixed and absolute cases.

Additionally, LayoutView tests have been unskipped to ensure this
does not regress.

BUG=645667

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Refactor LayoutView paint offset updates out of FrameView update code

Root layer scrolls lets us treat LayoutView like other LayoutObjects
and lets us move property updates into the regular LayoutObject update
code. This patch removes a special-case for LayoutView in the FrameView
property update. Comments have been added describing the LayoutView
context updates for fixed and absolute cases.

Additionally, LayoutView tests have been unskipped to ensure this
does not regress.

BUG=645667

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2016-11-02 05:28:16]

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