Bug 102557

Created attachment 174758 [details] Patch

Comment on attachment 174758 [details] Patch Attachment 174758 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14861375 New failing tests: fast/dom/shadow/shadow-dom-event-dispatching.html

Comment on attachment 174758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174758&action=review Looks likea great change, but I was curious about the static cast switch rather than a simple virtual method. They all appear to be children of a common superclass. > Source/WebCore/css/CSSImageGeneratorValue.cpp:200 > +{ In the ChangeLog could you indicate why this switch statement is preferable to a virtual method on CSSImageGeneratorValue? Seems like a lot of casting and specialized knowledge about which subclasses need this behavior. Was this found to be a bottleneck?

(In reply to comment #3) > (From update of attachment 174758 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174758&action=review > > Looks likea great change, but I was curious about the static cast switch rather than a simple virtual method. They all appear to be children of a common superclass. Actually, I was curious about that too when I wrote it. I just followed the pattern that was already there. I plead guilty to cargo cult programming. I assume there is some sort of requirement for CSS*Value classes to be POD? I should go satisfy my own curiosity, and come back... FWIW, before landing this I will perform some experiments just to make sure we are getting full use case coverage (all the hasAlpha methods returning true and false). I will add additional tests to the patch if needed.

(In reply to comment #3) > > In the ChangeLog could you indicate why this switch statement is preferable to a virtual method on CSSImageGeneratorValue? Seems like a lot of casting and specialized knowledge about which subclasses need this behavior. Was this found to be a bottleneck? The reason is already documented in CSSValue.h: // NOTE: This class is non-virtual for memory and performance reasons. // Don't go making it virtual again unless you know exactly what you're doing!

Created attachment 175503 [details] Patch

Latest patch fixes issues I found while performing more in-depth testing. I found that hasAlpha was returning true for opaque bitmap images that had not yet been pulled into cache. To fix this, I had to add a RanderObject* argument to hasAlpha so that if the style contains a BitmapImage, the bitmap can be resolved for the current renderer. The new patch also adds a layout test that covers a series of use cases that are optimization candidates. The current patch successfully optimizes 6 of the 10 cases. A regular test run does not verify that occlusion culling kicked in. To determine whether an optimization is successful, the test has to be run manually with a debugger attached.

Created attachment 175520 [details] Patch

New patch adds test expectations for missing baselines

Comment on attachment 175520 [details] Patch Attachment 175520 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14961191 New failing tests: fast/backgrounds/background-opaque-images-over-color.html

Created attachment 175684 [details] Patch

New patch adds text baseline and fixes bug where "space" repeat mode was falsely considered opaque (bug caught by existing test).

Comment on attachment 175520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175520&action=review > Source/WebCore/loader/cache/CachedImage.cpp:478 > +bool CachedImage::hasAlpha(const RenderObject* renderer) Maybe we should rename this to currentFrameHasAlpha() as well, just to be clear to callers.

Comment on attachment 175520 [details] Patch Fix the above on landing, and I'm good with it. r=me

Comment on attachment 175684 [details] Patch r+'ing the right patch this time.

Created attachment 175695 [details] Patch for landing

Comment on attachment 175695 [details] Patch for landing According to Noel Gordon, the change to BitmapImage will break deferred image decoding. See http://trac.webkit.org/changeset/125154 I'm going to cq- this patch for now until we can sort this out.

Created attachment 175805 [details] Patch

Triggering the decode for the purpose of occlusion culling while rendering the RenderBox is perfectly fine because the decoded image will be required imminently anyways in order to draw it. New patch reverts change to BimapImage, and moves the decode triggering into CachedImage. Since this is a new method, it won't affect previously existing code paths that check for the presence of alpha.

Comment on attachment 175805 [details] Patch Great. This is the solution I was thinking of as well. r=me

Comment on attachment 175805 [details] Patch Attachment 175805 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14966761 New failing tests: fast/backgrounds/background-opaque-images-over-color.html

Created attachment 175821 [details] Patch for landing

Comment on attachment 175821 [details] Patch for landing Clearing flags on attachment: 175821 Committed r135629: <http://trac.webkit.org/changeset/135629>

All reviewed patches have been landed. Closing bug.

Comment on attachment 175821 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=175821&action=review I wish I'd had a chance to review this before it was landed. > Source/WebCore/loader/cache/CachedImage.cpp:482 > + if (image->isBitmapImage()) > + image->nativeImageForCurrentFrame(); // force decode Seem unfortunate to force a decode here. Would that decode have always happened anyway, or would it be better to just say that it has alpha? > Source/WebCore/loader/cache/CachedImage.h:55 > + bool currentFrameHasAlpha(const RenderObject*); // Side effect: ensures decoded image is in cache, therefore should only be called when about to draw the image. Why is this not const? > Source/WebCore/rendering/RenderBox.cpp:1007 > + // RenderBoxModelOBject::paintFillLayerExtended. A more efficient solution might be to move Typo: OBject > Source/WebCore/rendering/RenderBox.cpp:1012 > + if (fillLayer->next() && (!fillLayer->hasOpaqueImage(this) || !fillLayer->image()->canRender(this, style()->effectiveZoom()) || !fillLayer->hasRepeatXY())) > + paintFillLayers(paintInfo, c, fillLayer->next(), rect, bleedAvoidance, op, backgroundObject); > paintFillLayer(paintInfo, c, fillLayer, rect, bleedAvoidance, op, backgroundObject); I don't see how this correctly handles background-clip (which means the image might not fill the entire border box). > Source/WebCore/rendering/RenderBoxModelObject.cpp:913 > + if (!bgLayer->next() && !(shouldPaintBackgroundImage && bgLayer->hasOpaqueImage(this) && bgLayer->hasRepeatXY())) { Same here. > LayoutTests/ChangeLog:13 > + * fast/backgrounds/background-opaque-images-over-color-expected.txt: Added. > + * fast/backgrounds/background-opaque-images-over-color.html: Added. > + * platform/chromium/TestExpectations: Why was this not a ref tests? Without pixel results, the test is useless.

(In reply to comment #25) > (From update of attachment 175821 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175821&action=review > > I wish I'd had a chance to review this before it was landed. Noted, and don't worry I'll post a follow-up patch to address your concerns early next week. > > > Source/WebCore/loader/cache/CachedImage.cpp:482 > > + if (image->isBitmapImage()) > > + image->nativeImageForCurrentFrame(); // force decode > > Seem unfortunate to force a decode here. Would that decode have always happened anyway, or would it be better to just say that it has alpha? Yes, a decode will happen anyways because this method is only ever called immediately before drawing the image. I documented this side-effect in the header in case anyone else is tempted to use this method for other purposes. > > > > Source/WebCore/rendering/RenderBox.cpp:1012 > > + if (fillLayer->next() && (!fillLayer->hasOpaqueImage(this) || !fillLayer->image()->canRender(this, style()->effectiveZoom()) || !fillLayer->hasRepeatXY())) > > + paintFillLayers(paintInfo, c, fillLayer->next(), rect, bleedAvoidance, op, backgroundObject); > > paintFillLayer(paintInfo, c, fillLayer, rect, bleedAvoidance, op, backgroundObject); > > I don't see how this correctly handles background-clip (which means the image might not fill the entire border box). OK, I'm surprised no existing test picked-up on that I'll come-up with a layout test and a fix for that ASAP. > > > LayoutTests/ChangeLog:13 > > + * fast/backgrounds/background-opaque-images-over-color-expected.txt: Added. > > + * fast/backgrounds/background-opaque-images-over-color.html: Added. > > + * platform/chromium/TestExpectations: > > Why was this not a ref tests? Without pixel results, the test is useless. It is, I just submitted it without the pixel baselines. I'll pick-up and submit the baselines from the bots with Tools/Script/webkit-patch garden-o-matic

(In reply to comment #26) > > > LayoutTests/ChangeLog:13 > > > + * fast/backgrounds/background-opaque-images-over-color-expected.txt: Added. > > > + * fast/backgrounds/background-opaque-images-over-color.html: Added. > > > + * platform/chromium/TestExpectations: > > > > Why was this not a ref tests? Without pixel results, the test is useless. > > It is, I just submitted it without the pixel baselines. I'll pick-up and submit the baselines from the bots with Tools/Script/webkit-patch garden-o-matic A ref tests would have a -expected.html, and no .txt file.

(In reply to comment #27) > (In reply to comment #26) > > > > LayoutTests/ChangeLog:13 > > > > + * fast/backgrounds/background-opaque-images-over-color-expected.txt: Added. > > > > + * fast/backgrounds/background-opaque-images-over-color.html: Added. > > > > + * platform/chromium/TestExpectations: > > > > > > Why was this not a ref tests? Without pixel results, the test is useless. > > > > It is, I just submitted it without the pixel baselines. I'll pick-up and submit the baselines from the bots with Tools/Script/webkit-patch garden-o-matic > > A ref tests would have a -expected.html, and no .txt file. Please make this a reftest unless it's absolutely necessary to test the generated render tree. It's easier for everyone to maintain a reference output of a 200x100px green rectangle than to potentially have many ports generate unneeded pixel baselines. OTOH, the test is producing a text failure on every port (except mac where the test is skipped). http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fbackgrounds%2Fbackground-opaque-images-over-color.html

Unfortunately, new fast/backgrounds/background-opaque-images-over-color.html started to fail on Qt. I have created a bug for the issue at https://bugs.webkit.org/show_bug.cgi?id=103227 I will skip this test on Qt until it's fixed, if you don't mind.

The test has been skipped on Qt in http://trac.webkit.org/changeset/135698 Please unskip it with the proper fix (see the related bug at https://bugs.webkit.org/show_bug.cgi?id=103227 )

I think we should revert this change if we don't get a fix soon.

Follow-ups: Ref test: https://bugs.webkit.org/show_bug.cgi?id=103275 Clip issue: https://bugs.webkit.org/show_bug.cgi?id=103276