Bug 106966

I'm not sure why the default base class implementation returns false. I would think that true is always the conservative response here.

Created attachment 184009 [details] Trivial patch

Comment on attachment 184009 [details] Trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=184009&action=review > Source/WebCore/platform/graphics/Image.h:102 > + virtual bool currentFrameHasAlpha() { return true; } Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community.

Comment on attachment 184009 [details] Trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=184009&action=review >> Source/WebCore/platform/graphics/Image.h:102 >> + virtual bool currentFrameHasAlpha() { return true; } > > Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community. Besides that issue (and I personally would be open to pure virtual for this), I think the function name is wrong. It should be named currentFrameMayHaveAlpha or some other name like that that makes it clear that it does not give a definitive answer for all types of images.

(In reply to comment #3) > Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community. I also think we need to understand the impact of this change. Many of the sites that call this method need to know whether it is safe to assume that the image is opaque in order to apply shortcuts (occlusion culling, skip blending, etc.) In that context "true" is an appropriate default value since it is safe to have false positives. Before submitting this, we should make sure that this change is in fact safe for all the call sites. Also, does this fix any bugs? If we used to be falsely assuming that SVGimages are always opaque and they're not, then there is a good chance that this patch fixes a bug. It would be nice to have a LayoutTest that proves it. If this was not causing a bug, do we understand why?

Comment on attachment 184009 [details] Trivial patch Thanks for the reviews! I'm clearing the r? flag while I prepare the updated patch.

Created attachment 185001 [details] First pass at addressing reviewer comments

(In reply to comment #3) > (From update of attachment 184009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184009&action=review > > > Source/WebCore/platform/graphics/Image.h:102 > > + virtual bool currentFrameHasAlpha() { return true; } > > Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community. I agree, and fixing this exposed a small bug: GeneratedImage also needs to implement this. I've updated the latest patch to address this. (In reply to comment #4) > (From update of attachment 184009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184009&action=review > > >> Source/WebCore/platform/graphics/Image.h:102 > >> + virtual bool currentFrameHasAlpha() { return true; } > > > > Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community. > > Besides that issue (and I personally would be open to pure virtual for this), I think the function name is wrong. It should be named currentFrameMayHaveAlpha or some other name like that that makes it clear that it does not give a definitive answer for all types of images. I tried a couple names such as currentFrameMayHaveAlpha and currentFrameMayhaveTransparency but I wasn't happy with how it looked from the caller's perspective. As an example, RenderImage::backgroundIsObscured() calls this function but is really looking to know whether the image is fully opaque or not. The latest patch uses "currentFrameIsKnownToBeOpaque". (In reply to comment #5) > (In reply to comment #3) > > > Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community. > > I also think we need to understand the impact of this change. Many of the sites that call this method need to know whether it is safe to assume that the image is opaque in order to apply shortcuts (occlusion culling, skip blending, etc.) In that context "true" is an appropriate default value since it is safe to have false positives. Before submitting this, we should make sure that this change is in fact safe for all the call sites. I hope the new function name "knownToBeOpaque" clarifies how we handle the unknown case. > > Also, does this fix any bugs? If we used to be falsely assuming that SVGimages are always opaque and they're not, then there is a good chance that this patch fixes a bug. It would be nice to have a LayoutTest that proves it. > > If this was not causing a bug, do we understand why? This was a tough one--this does not actually fix any bugs! When accessing SVG images we currently go through the SVGImageCache which returns a bitmap image for imageForRenderer. Therefore, in all of the callsites we actually weren't dealing with an SVG image but instead a rasterized bitmap image of the SVG image. In WK106159 I hope to change this. The latest patch modifies a test that will catch regressions in the future.

Comment on attachment 185001 [details] First pass at addressing reviewer comments Attachment 185001 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16160636

Thanks very much for taking this on! Justin and I were discussing how misleading the current situation is, and this is a great improvement. (In reply to comment #8) > I tried a couple names such as currentFrameMayHaveAlpha and currentFrameMayhaveTransparency but I wasn't happy with how it looked from the caller's perspective. As an example, RenderImage::backgroundIsObscured() calls this function but is really looking to know whether the image is fully opaque or not. The latest patch uses "currentFrameIsKnownToBeOpaque". I hate to bikeshed, but could it be simply currentFrameIsOpaque() (and isOpaque())? They're shorter, and I think it's clear that the conservative thing to do is return false if you're not certain. > (In reply to comment #5) > > (In reply to comment #3) > > > > > Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community. > > > > I also think we need to understand the impact of this change. Many of the sites that call this method need to know whether it is safe to assume that the image is opaque in order to apply shortcuts (occlusion culling, skip blending, etc.) In that context "true" is an appropriate default value since it is safe to have false positives. Before submitting this, we should make sure that this change is in fact safe for all the call sites. > > I hope the new function name "knownToBeOpaque" clarifies how we handle the unknown case. > > > > > Also, does this fix any bugs? If we used to be falsely assuming that SVGimages are always opaque and they're not, then there is a good chance that this patch fixes a bug. It would be nice to have a LayoutTest that proves it. > > > > If this was not causing a bug, do we understand why? > > This was a tough one--this does not actually fix any bugs! When accessing SVG images we currently go through the SVGImageCache which returns a bitmap image for imageForRenderer. Therefore, in all of the callsites we actually weren't dealing with an SVG image but instead a rasterized bitmap image of the SVG image. In WK106159 I hope to change this. The latest patch modifies a test that will catch regressions in the future.

Comment on attachment 185001 [details] First pass at addressing reviewer comments Attachment 185001 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16160637

Created attachment 185029 [details] Update for bot happiness

(In reply to comment #10) > I hate to bikeshed, but could it be simply currentFrameIsOpaque() (and isOpaque())? They're shorter, and I think it's clear that the conservative thing to do is return false if you're not certain. This is obvious at the moment but a year from now (when I've forgotten the details) I would be very surprised to get a non-opaque image that returns true for isOpaque. For that reason I'd like to lean towards keeping it in the function name.

Comment on attachment 185029 [details] Update for bot happiness Attachment 185029 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16154713

(In reply to comment #13) > (In reply to comment #10) > > I hate to bikeshed, but could it be simply currentFrameIsOpaque() (and isOpaque())? They're shorter, and I think it's clear that the conservative thing to do is return false if you're not certain. > > This is obvious at the moment but a year from now (when I've forgotten the details) I would be very surprised to get a non-opaque image that returns true for isOpaque. For that reason I'd like to lean towards keeping it in the function name. Wait.. if that were true, then your current patch would have isKnownToBeOpaque() returning true for a non-opaque image in the above case. That doesn't seem right either. I'm guessing the surprising case is isOpaque() returning false for an opaque image. isKnownToBeOpaque() returning false for opaque seems less surprising, so I retract my objection to the verbosity. :)

Created attachment 185309 [details] Update for bot happines (2) One last build file update. Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in needed an update for win bot's happiness.

Comment on attachment 185309 [details] Update for bot happines (2) View in context: https://bugs.webkit.org/attachment.cgi?id=185309&action=review Looks good, but Justin wants to take a look. > Source/WebCore/ChangeLog:13 > + This patch renames hasAlpha and currentFrameHasAlpha to isKnownOpaque and Nit: code says isKnownToBeOpaque, but ChangeLog says isKnownOpaque.

Comment on attachment 185309 [details] Update for bot happines (2) View in context: https://bugs.webkit.org/attachment.cgi?id=185309&action=review Me likes, but I'm no reviewer. > Source/WebCore/platform/graphics/skia/BitmapImageSingleFrameSkia.cpp:14 > + return m_nativeImage.bitmap().isOpaque(); Seeing this makes me think we should probably follow the example of this patch in skia and rename isOpaque to knownToBeOpaque, because it has a similar conservative best guess behavior. > Source/WebCore/svg/graphics/SVGImage.h:85 > + // FIXME: Implement this be less conservative. Unclear. Punctuation would help.

Comment on attachment 185309 [details] Update for bot happines (2) r=me. Nits are at your discretion.

Created attachment 185559 [details] Update per reviewer comments Two small updates: Fix the Hamletian ChangeLog typo and fix the grammar mistake in the FIXMEs. @Darin I think this patch is ready to commit but I'd like to double-check that your concerns were addressed. Do you mind taking one more look?

(In reply to comment #20) > Created an attachment (id=185559) [details] > Update per reviewer comments > > Two small updates: Fix the Hamletian ChangeLog typo and fix the grammar mistake in the FIXMEs. > > @Darin > I think this patch is ready to commit but I'd like to double-check that your concerns were addressed. Do you mind taking one more look? It has been a couple days so I'm going to send this to the queue before it rots. I am happy to re-open, roll-out, or revisit this at any time.

Comment on attachment 185559 [details] Update per reviewer comments Rejecting attachment 185559 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 185559, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: /PlatformContextSkiaTest.cpp patching file Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/box-shadow/image-box-shadow-expected.html patching file LayoutTests/fast/box-shadow/image-box-shadow.html patching file LayoutTests/fast/box-shadow/resources/green.svg Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/16306938

Created attachment 186098 [details] Rebased patch for landing Looks like the previous patch went stale after all. Attaching an updated, rebased patch that's ready for the queue.

Comment on attachment 186098 [details] Rebased patch for landing Clearing flags on attachment: 186098 Committed r141637: <http://trac.webkit.org/changeset/141637>

All reviewed patches have been landed. Closing bug.