Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(247)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 15925012: Merges two WK fixes for RenderLayer::backgroundIsKnownToBeOpaque. (Closed)

Created:
7 years, 6 months ago by alokp
Modified:
5 years, 7 months ago
Reviewers:
jamesr, Xianzhu, dsinclair
CC:
blink-reviews, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering
Visibility:
Public.

Description

Merges two WK fixes for RenderLayer::backgroundIsKnownToBeOpaque. 1. Opaqueness logic takes visibility:hidden property into account. If a render object is not visible, it is not marked as opaque. - http://trac.webkit.org/changeset/149084 - http://trac.webkit.org/changeset/149915 2. The logic was incorrect for layers where the given rect wasn't contained in the background rect, but where some child layer obscured the rect, even though clipping hid part of that child layer. So bail from RenderLayer::backgroundIsKnownToBeOpaqueInRect() if we have any overflow clipping. This could be enhanced in future to test whether child layers obscure the clipping rect, but that would be more expensive. - http://trac.webkit.org/changeset/149914 BUG=245229 R=jamesr@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151581

Patch Set 1 #

Total comments: 2

Patch Set 2 : added DOCTYPE #

Total comments: 5

Messages

Total messages: 13 (2 generated)
alokp
7 years, 6 months ago (2013-05-31 00:21:46 UTC) #1
jamesr
Code lgtm. Patch description does not: *) If these are merges, include links to the ...
7 years, 6 months ago (2013-05-31 01:17:17 UTC) #2
alokp
Updated description. https://codereview.chromium.org/15925012/diff/1/LayoutTests/compositing/contents-opaque/visibility-hidden.html File LayoutTests/compositing/contents-opaque/visibility-hidden.html (right): https://codereview.chromium.org/15925012/diff/1/LayoutTests/compositing/contents-opaque/visibility-hidden.html#newcode1 LayoutTests/compositing/contents-opaque/visibility-hidden.html:1: <html> On 2013/05/31 01:17:17, jamesr wrote: > ...
7 years, 6 months ago (2013-05-31 16:14:47 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alokp@chromium.org/15925012/5001
7 years, 6 months ago (2013-05-31 16:28:13 UTC) #4
commit-bot: I haz the power
Retried try job too often on win_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout_rel&number=8133
7 years, 6 months ago (2013-05-31 19:18:02 UTC) #5
alokp
Committed patchset #2 manually as r151581 (presubmit successful).
7 years, 6 months ago (2013-05-31 21:58:44 UTC) #6
Xianzhu
https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/contents-opaque/hidden-with-visible-child-expected.txt File LayoutTests/compositing/contents-opaque/hidden-with-visible-child-expected.txt (right): https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/contents-opaque/hidden-with-visible-child-expected.txt#newcode13 LayoutTests/compositing/contents-opaque/hidden-with-visible-child-expected.txt:13: (backgroundColor #008000) Same question as for visibility-hidden.html https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/contents-opaque/hidden-with-visible-text-expected.txt File ...
5 years, 7 months ago (2015-05-07 15:51:05 UTC) #9
alokp
https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt File LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt (right): https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt#newcode13 LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt:13: (backgroundColor #C0C0C0) On 2015/05/07 15:51:05, Xianzhu wrote: > alokp@ ...
5 years, 7 months ago (2015-05-07 17:00:59 UTC) #10
Xianzhu
https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt File LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt (right): https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt#newcode13 LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt:13: (backgroundColor #C0C0C0) On 2015/05/07 17:00:59, Alok Priyadarshi wrote: > ...
5 years, 7 months ago (2015-05-07 17:13:24 UTC) #11
alokp
On 2015/05/07 17:13:24, Xianzhu wrote: > https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt > File LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt > (right): > > https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt#newcode13 ...
5 years, 7 months ago (2015-05-07 17:17:12 UTC) #12
Xianzhu
5 years, 7 months ago (2015-05-07 17:26:08 UTC) #13
Message was sent while issue was closed. 
On 2015/05/07 17:17:12, Alok Priyadarshi wrote:
> On 2015/05/07 17:13:24, Xianzhu wrote:
> >
>
https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/co...
> > File LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt
> > (right):
> > 
> >
>
https://codereview.chromium.org/15925012/diff/5001/LayoutTests/compositing/co...
> > LayoutTests/compositing/contents-opaque/visibility-hidden-expected.txt:13:
> > (backgroundColor #C0C0C0)
> > On 2015/05/07 17:00:59, Alok Priyadarshi wrote:
> > > On 2015/05/07 15:51:05, Xianzhu wrote:
> > > > alokp@ this looks incorrect to me. The object has been set 'visibility:
> > > hidden',
> > > > why there is still a layer with background color?
> > > 
> > > May be assumptions have changed in 2 years since this patch landed.
> > > 
> > > What are you asking?
> > > - Why is there even a layer for invisible element, or
> > > - Why does this layer have a background color?
> > > 
> > > The important thing that this patch was trying to accomplish is to make
sure
> > we
> > > do not mark layers as opaque when they are NOT. We were trying to be
> > > conservative about it.
> > 
> > I'm asking mainly the first question. Will contentsOpaque affect whether cc
to
> > display the layer?
> 
> NO. contentsOpaque should not affect the visibility of layer. It was only used
> to enable/disable font subpixel antialiasing.

So I think the test expectations are incorrect. Cc will still display the layer
with the background while the layer should be hidden.

There seems some issue about compositing update.
https://codereview.chromium.org/1110233003/ seems to trigger the compositing
update for visibility-hidden.html causing it to produce correct result. Filed
crbug.com/485610.

Powered by Google App Engine
This is Rietveld 408576698