Bug 62593 – Optimization: do a single fillRect when painting the background in RenderBoxModelObject::paintFillLayerExtended

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Una Sabovic]

RenderBoxModelObject::paintFillLayerExtended could do a single fillRect when painting the background instead of two.
Blend the baseColor with bgColor and perform a single fillRect.

[Una Sabovic]

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.

[WebKit Review Bot]

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.

[Darin Adler]

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.

[Una Sabovic]

Created attachment 97002 [details]
proposed patch

fix style

[Darin Adler]

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.

[Una Sabovic]

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?

[Una Sabovic]

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

[Una Sabovic]

Created attachment 97043 [details]
screenshot trunk vs branch - rendering the attached test html page

[Una Sabovic]

Created attachment 97044 [details]
test page with transparent bgcolor

[Simon Fraser (smfr)]

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.

[Una Sabovic]

Created attachment 97469 [details]
proposed patch

[Una Sabovic]

Comment on attachment 97469 [details]
proposed patch

Should Darin review as well? I don't see him on the reviewers list.

[WebKit Review Bot]

Comment on attachment 97469 [details]
proposed patch

Clearing flags on attachment: 97469

Committed r89057: <http://trac.webkit.org/changeset/89057>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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

[Una Sabovic]

>> 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?

[Darin Adler]

(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.