RenderBoxModelObject::paintFillLayerExtended could do a single fillRect when painting the background instead of two. Blend the baseColor with bgColor and perform a single fillRect.
Created attachment 96999 [details] proposed patch No new tests added since this change is just an optimization. It doesn't change or add any functionality.
Attachment 96999 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/RenderBoxModelObject.cpp:747: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/rendering/RenderBoxModelObject.cpp:760: An else should appear on the same line as the preceding } [whitespace/newline] [4] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 96999 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=96999&action=review There is no need here to make any of the calls to isValid. Any color that is not valid will also have an alpha of zero. Instead all the code can just check for non-zero alpha values. We’d like to eliminate the concept of “valid” entirely from Color, so it’s best not to add more checks for validity. > Source/WebCore/rendering/RenderBoxModelObject.cpp:748 > + baseColor = view()->frameView()->baseBackgroundColor(); > + if (baseColor.alpha() == 0) > + baseColor.setRGB(0, 0, 0); Will this do the same thing as the old code if the base color has an alpha of 50%? I don’t think it will.
Created attachment 97002 [details] proposed patch fix style
Comment on attachment 97002 [details] proposed patch My comments still apply and are not addressed. The review- is because I believe this has incorrect handling of the 50% alpha base color case.
Why is alpha of 50% special? I did run all the pixel tests and didn't run into failures. Is there a test or a group of tests I can run/modify to make sure it works in all cases?
Created attachment 97042 [details] proposed patch Qt seems to be hard coding the base color for the frame view to brush(QPalette::Base) which is RGBA (255,255,255,255). Since I couldn't change this value with html/css I hard coded it to baseColor = Color(0,0,255,128) for testing purposes in both trunk and my branch. (If you know how I can affect this value with html/css pls let me know) I ran QtLauncher trunk vs QtLauncher with the patch - result came out the same. See attached screenshot. Here is the log from RenderBoxModelObject baseColor RGBA 0,0,255,128 backgroundRect (0,0, 800,533) bgColor RGBA 255,255,0,204 baseColor blended with bgColor RGBA 226,226,28,229
Created attachment 97043 [details] screenshot trunk vs branch - rendering the attached test html page
Created attachment 97044 [details] test page with transparent bgcolor
Comment on attachment 97042 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97042&action=review This seems ok to me but I'll let Darin review. > Source/WebCore/ChangeLog:6 > + Optimization: do a single fillRect when painting the background in RenderBoxModelObject::paintFillLayerExtended > + https://bugs.webkit.org/show_bug.cgi?id=62593 The bug title should say that this is just for the root background. > Source/WebCore/ChangeLog:8 > + Intead of doing two fillRects blend the base with background color and do a single fillRect. This should also explain that it's just for the root background.
Created attachment 97469 [details] proposed patch
Comment on attachment 97469 [details] proposed patch Should Darin review as well? I don't see him on the reviewers list.
Comment on attachment 97469 [details] proposed patch Clearing flags on attachment: 97469 Committed r89057: <http://trac.webkit.org/changeset/89057>
All reviewed patches have been landed. Closing bug.
Comment on attachment 97469 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97469&action=review My apologies for not reviewing this before landing it. I still have a bit of concern. Do we have test cases to cover all the combinations? I’d like to see those. I still worry that base colors with alpha that are neither 0 nor 1 combined with bg colors with alpha other than 0 are not handled correctly. Test cases would make that clear. Given that the drawImage functions take a composite operator, I’m surprised we don’t have a fillRect that takes a composite operator. Beyond the correctness issue, I also think we could optimize the case of a base color with alpha of 0 and a bg color with a non-zero alpha so we did not have first the clearRect and then the fillRect. > Source/WebCore/rendering/RenderBoxModelObject.cpp:751 > + if (baseColor.alpha() > 0) { I think we should drop the "> 0" from these to make it closer to our normal coding style
>> Do we have test cases to cover all the combinations? See Comment #7. Base bg is hard coded and I couldn't change it with html/css. If you know how to do it with html/css pls let me know and I'll provide the test cases. I tested the 50% alpha base bg color by hard coding it - see Comment #7 and attached screenshot and test html. I have tested many combinations of base & bg colors with alphas adding to less than 1 - just didn't attach all the screenshots. >> we could optimize the case of a base color with alpha of 0 and a bg color with a non-zero alpha so we did not have first the clearRect and then the fillRect. I thought to add this as well. Should I just add another patch to this bug or create a new one?
(In reply to comment #16) > Should I just add another patch to this bug or create a new one? New bug is better. There’s little upside to doing multiple patches in one bug.