Some simple cleanup in PaintPropertyTreeBuilder.

Some simple cleanup in PaintPropertyTreeBuilder.

Removed one unnecessary function (NeedsCssClip is always true when this one is
true).

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2893893003
Cr-Commit-Position: refs/heads/master@{#473070}
Committed: https://chromium.googlesource.com/chromium/src/+/338aeaad66d93ce040f5757e2d29502810031d67

[chrishtr, 2017-05-18 23:03:19]

Description was changed from

==========
none

BUG=
==========

to

==========
none

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

[chrishtr, 2017-05-18 23:04:40]

Description was changed from

==========
none

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

to

==========
Some simple cleanup in PaintPropertyTreeBuilder.

Removed one unnecessary function (NeedsCssClip is always true when this one is
true).

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

[chrishtr, 2017-05-18 23:04:53]

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

[chrishtr, 2017-05-18 23:05:16]

chrishtr@chromium.org changed reviewers:
+ wkorman@chromium.org

[commit-bot: I haz the power, 2017-05-18 23:06:17]

Dry run: 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, 2017-05-18 23:11:41]

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

[commit-bot: I haz the power, 2017-05-18 23:11:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-05-18 23:17:03]

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

[commit-bot: I haz the power, 2017-05-18 23:18:15]

Dry run: CQ is trying da patch.

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

[wkorman, 2017-05-18 23:47:22]

https://codereview.chromium.org/2893893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (left):

https://codereview.chromium.org/2893893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:981: return
!object.IsLayoutView() && !object.CanContainFixedPositionObjects() &&
NeedsCssClip is obviously redundant with the one callsite of this method, but
how are you sure re: the other two?

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

https://codereview.chromium.org/2893893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:923: return
!scroll_offset.IsZero() || scrollable_area->ScrollsOverflow();
Can scrollable area ever be null? Should we null- or dcheck-it?

[chrishtr, 2017-05-19 00:21:35]

https://codereview.chromium.org/2893893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (left):

https://codereview.chromium.org/2893893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:981: return
!object.IsLayoutView() && !object.CanContainFixedPositionObjects() &&
On 2017/05/18 at 23:47:21, wkorman wrote:
> NeedsCssClip is obviously redundant with the one callsite of this method, but
how are you sure re: the other two?

The logic in UpdatePaintProperties looks like:

bool needs_paint_properties = ... || NeedsCssClip() || ... ||
NeedsCssClipFixedPosition() || ...;

which is expanded as:

bool needs_paint_properties = ... || NeedsCssClip() || ... ||
(!object.IsLayoutView() && !object.CanContainFixedPositionObjects() &&
NeedsCssClip()) || ...;

Then

NeedsCssClip() || (!object.IsLayoutView() &&
!object.CanContainFixedPositionObjects() && NeedsCssClip())

can be reduced to

NeedsCssClip().

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

https://codereview.chromium.org/2893893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:923: return
!scroll_offset.IsZero() || scrollable_area->ScrollsOverflow();
On 2017/05/18 at 23:47:22, wkorman wrote:
> Can scrollable area ever be null? Should we null- or dcheck-it?

No, because HasOverflowClip ensures it. I don't know about DCHECK,
other callsites do the same thing.

[wkorman, 2017-05-19 00:32:30]

lgtm

https://codereview.chromium.org/2893893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (left):

https://codereview.chromium.org/2893893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:981: return
!object.IsLayoutView() && !object.CanContainFixedPositionObjects() &&
On 2017/05/19 00:21:35, chrishtr wrote:
> On 2017/05/18 at 23:47:21, wkorman wrote:
> > NeedsCssClip is obviously redundant with the one callsite of this method,
but
> how are you sure re: the other two?
> 
> The logic in UpdatePaintProperties looks like:
> 
> bool needs_paint_properties = ... || NeedsCssClip() || ... ||
> NeedsCssClipFixedPosition() || ...;
> 
> which is expanded as:
> 
> bool needs_paint_properties = ... || NeedsCssClip() || ... ||
> (!object.IsLayoutView() && !object.CanContainFixedPositionObjects() &&
> NeedsCssClip()) || ...;
> 
> Then
> 
> NeedsCssClip() || (!object.IsLayoutView() &&
> !object.CanContainFixedPositionObjects() && NeedsCssClip())
> 
> can be reduced to
> 
> NeedsCssClip().

Ah. LOL.

[commit-bot: I haz the power, 2017-05-19 01:55:22]

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

[commit-bot: I haz the power, 2017-05-19 01:55:23]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[chrishtr, 2017-05-19 02:43:38]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-19 02:44:36]

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, 2017-05-19 03:50:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495161818872980,
"parent_rev": "5c2987aea858fe28a76f6d41ca86ed2771b39e4b", "commit_rev":
"338aeaad66d93ce040f5757e2d29502810031d67"}

[commit-bot: I haz the power, 2017-05-19 03:51:11]

Description was changed from

==========
Some simple cleanup in PaintPropertyTreeBuilder.

Removed one unnecessary function (NeedsCssClip is always true when this one is
true).

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

to

==========
Some simple cleanup in PaintPropertyTreeBuilder.

Removed one unnecessary function (NeedsCssClip is always true when this one is
true).

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2893893003
Cr-Commit-Position: refs/heads/master@{#473070}
Committed:
https://chromium.googlesource.com/chromium/src/+/338aeaad66d93ce040f5757e2d29...
==========

[commit-bot: I haz the power, 2017-05-19 03:51:12]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/338aeaad66d93ce040f5757e2d29...