Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1787733003:
Consolidate background fill layer opaqueness into one method (Closed)
|
Created:
4 years, 9 months ago by Stephen Chennney Modified:
4 years, 9 months ago Reviewers:
trchen CC:
chromium-reviews, blink-reviews, dshwang, blink-reviews-style_chromium.org, slimming-paint-reviews_chromium.org, blink-reviews-paint_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConsolidate background fill layer opaqueness into one method
Move all the logic for determining if a background layer is
opaque into FillLayer. Also add repeat round as a opaque
tiling mode.
R=trchen@chromium.org
BUG=593437
Committed: https://crrev.com/e3ab046fcbe969bd1b38a0361f7d583998365c2c
Cr-Commit-Position: refs/heads/master@{#382301}
Patch Set 1 #
Total comments: 2
Patch Set 2 : A new way to refactor #
Total comments: 5
Patch Set 3 : Remove redundant checks #Patch Set 4 : Better solution fixes previous failure #Patch Set 5 : Fix windows build #
Messages
Total messages: 39 (16 generated)
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1787733003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1787733003/1
I renamed the FillLayer method to backgroundImageIsOpaque, working on the principle that the CSS property is called background-image. I would also be OK with backgroundImageRendersOpaque.
On 2016/03/11 21:23:09, Stephen Chenney wrote: > I renamed the FillLayer method to backgroundImageIsOpaque, working on the > principle that the CSS property is called background-image. I would also be OK > with backgroundImageRendersOpaque. FillLayer is also used by CSS mask. It is not necessarily a "background" image. :p
https://codereview.chromium.org/1787733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/FillLayer.cpp (right): https://codereview.chromium.org/1787733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/FillLayer.cpp:373: bool FillLayer::backgroundImageIsOpaque(const LayoutObject& imageClient, const float effectiveZoom) const I think the complexity of this function is getting unmanageable. Usually a boolean function checks a few positive conditions to return true, and return false if none of the conditions are met. (i.e. each functions being CNF.) Mixing positive and negative conditions make it really difficult to comprehend. Is there a reason why we can't we split these criteria into logical groups as I recommended on crbug?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
And thanks for the heads-up about CSS masking. https://codereview.chromium.org/1787733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/FillLayer.cpp (right): https://codereview.chromium.org/1787733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/FillLayer.cpp:373: bool FillLayer::backgroundImageIsOpaque(const LayoutObject& imageClient, const float effectiveZoom) const On 2016/03/11 22:13:33, trchen wrote: > I think the complexity of this function is getting unmanageable. > > Usually a boolean function checks a few positive conditions to return true, and > return false if none of the conditions are met. (i.e. each functions being CNF.) > > Mixing positive and negative conditions make it really difficult to comprehend. > Is there a reason why we can't we split these criteria into logical groups as I > recommended on crbug? I'm wary of exposing multiple methods that a caller then has to combine. I agree on the "all if's return true, return false at end" or vice versa in this case, so I'll aim to get things into that form.
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1787733003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1787733003/20001
The only alternative I can think of to this is to switch on the composite mode. Let me know if you think that's better. https://codereview.chromium.org/1787733003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/FillLayer.cpp (right): https://codereview.chromium.org/1787733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/FillLayer.cpp:374: bool FillLayer::imageIsOpaqueAndTilesLayer(const LayoutObject& layoutObject) const Put almost all the conditions surround images here. https://codereview.chromium.org/1787733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/FillLayer.cpp:383: && m_image->canRender() Forgot to remove these. Will follow up. https://codereview.chromium.org/1787733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/FillLayer.cpp:391: bool FillLayer::imageOccludesNextLayers(const LayoutObject& layoutObject) const Due to the compositing modes behavior, it's not really possible to get a nice breakdown of single conditions. https://codereview.chromium.org/1787733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/FillLayer.cpp:407: return (m_composite == CompositeClear || m_composite == CompositeCopy); This is really the else clause, but we do not put the else clause before an immediate method return. https://codereview.chromium.org/1787733003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/FillLayer.h (right): https://codereview.chromium.org/1787733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/FillLayer.h:178: bool imageOccludesNextLayers(const LayoutObject&) const; Renamed to match the clipOccludesNextLayers method.
The CQ bit was unchecked by schenney@chromium.org
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1787733003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1787733003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1787733003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1787733003/60001
Moving to a switch statement on the compositing state. This is looking more and more like what trchen@ proposed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1787733003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1787733003/80001
Patchset #5 (id:80001) has been deleted
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1787733003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1787733003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. Thank you for the cleanup!
The CQ bit was checked by schenney@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1787733003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1787733003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by schenney@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1787733003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1787733003/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Consolidate background fill layer opaqueness into one method Move all the logic for determining if a background layer is opaque into FillLayer. Also add repeat round as a opaque tiling mode. R=trchen@chromium.org BUG=593437 ========== to ========== Consolidate background fill layer opaqueness into one method Move all the logic for determining if a background layer is opaque into FillLayer. Also add repeat round as a opaque tiling mode. R=trchen@chromium.org BUG=593437 Committed: https://crrev.com/e3ab046fcbe969bd1b38a0361f7d583998365c2c Cr-Commit-Position: refs/heads/master@{#382301} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e3ab046fcbe969bd1b38a0361f7d583998365c2c Cr-Commit-Position: refs/heads/master@{#382301} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
