Bug 74017 – [Chromium] Add per-tile painting flag to DumpRenderTree and rename AcceleratedDrawing to AcceleratedPainting in chromium specific code.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Reveman]

Add support for command line flag enable-per-tile-drawing in DumpRenderTree.

[David Reveman]

Created attachment 118250 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 118250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118250&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:2906
> +        ccSettings.perTilePainting = page()->settings()->perTileDrawingEnabled();

is it per-tile drawing or per-tile painting?

i thought we made a careful distinction between those terms.  seeing
them equated here makes me confused.

[David Reveman]

(In reply to comment #3)
> (From update of attachment 118250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118250&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:2906
> > +        ccSettings.perTilePainting = page()->settings()->perTileDrawingEnabled();
> 
> is it per-tile drawing or per-tile painting?
> 
> i thought we made a careful distinction between those terms.  seeing
> them equated here makes me confused.

It's currently "accelerated painting" in the compositor but "accelerated drawing" outside. This change makes the "per-tile" option consistent with the "accelerated" option. If we want "painting" to be used everywhere, both of these options should change.

[WebKit Review Bot]

Comment on attachment 118250 [details]
Patch

Attachment 118250 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10752277

New failing tests:
compositing/plugins/invalidate_rect.html

[James Robinson]

Comment on attachment 118250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118250&action=review

Can you look into the test failure?

> Source/WebCore/page/Settings.h:504
> +        void setPerTileDrawingEnabled(bool enabled) { m_perTileDrawingEnabled = enabled; }
> +        bool perTileDrawingEnabled() const { return m_perTileDrawingEnabled; }

this should be painting, methinks

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:2906
>>> +        ccSettings.perTilePainting = page()->settings()->perTileDrawingEnabled();
>> 
>> is it per-tile drawing or per-tile painting?
>> 
>> i thought we made a careful distinction between those terms.  seeing
>> them equated here makes me confused.
> 
> It's currently "accelerated painting" in the compositor but "accelerated drawing" outside. This change makes the "per-tile" option consistent with the "accelerated" option. If we want "painting" to be used everywhere, both of these options should change.

"accelerated drawing" exists only in cross-platform code because it's the term that apple likes to use for what we call accelerated painting. We should continue to use that term in cross-platform code where it's necessary, but stick to painting everywhere else

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:223
> +        optparse.make_option("--per-tile-drawing",

this also should be painting

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:225
> +            help="Use per-tile drawing of composited pages"),

painting

[David Reveman]

(In reply to comment #6)
> (From update of attachment 118250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118250&action=review
> 
> Can you look into the test failure?
> 
> > Source/WebCore/page/Settings.h:504
> > +        void setPerTileDrawingEnabled(bool enabled) { m_perTileDrawingEnabled = enabled; }
> > +        bool perTileDrawingEnabled() const { return m_perTileDrawingEnabled; }
> 
> this should be painting, methinks
> 
> >>> Source/WebKit/chromium/src/WebViewImpl.cpp:2906
> >>> +        ccSettings.perTilePainting = page()->settings()->perTileDrawingEnabled();
> >> 
> >> is it per-tile drawing or per-tile painting?
> >> 
> >> i thought we made a careful distinction between those terms.  seeing
> >> them equated here makes me confused.
> > 
> > It's currently "accelerated painting" in the compositor but "accelerated drawing" outside. This change makes the "per-tile" option consistent with the "accelerated" option. If we want "painting" to be used everywhere, both of these options should change.
> 
> "accelerated drawing" exists only in cross-platform code because it's the term that apple likes to use for what we call accelerated painting. We should continue to use that term in cross-platform code where it's necessary, but stick to painting everywhere else
> 
> > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:223
> > +        optparse.make_option("--per-tile-drawing",
> 
> this also should be painting
> 
> > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:225
> > +            help="Use per-tile drawing of composited pages"),
> 
> painting


OK, no problem. I'll change this patch so that every chromium specific use of "accelerated/per-tile drawing" becomes "accelerated/per-tile painting" instead. This also means changing the chromium command line option "--enable-accelerated-drawing" to "--enable-accelerated-painting", which I'll take care of while adding the "--enable-per-tile-painting" option.

[Alok Priyadarshi]

DRT uses accelerated-drawing, which I think is appropriate because it is changing the cross-platform accelerated-drawing setting. By the same argument, --enable-accelerated-drawing flag is appropriate for chrome?

I prefer using one name everywhere. I do not care which one it is. If we hate "drawing" so much, can we persuade Apple engineers to use "painting"?

[David Reveman]

Started changing to "painting" where possible but think this makes it more confusing than the current patch unless cross-platform code is also changed to "painting". The current patch makes it clear where "drawing" vs "painting" is used so I think we should stick with it for now until we can convince apple to use "painting" instead.

[James Robinson]

I disagree.  We should keep all chromium-specific code consistent, at the very least.

[David Reveman]

Created attachment 118464 [details]
Patch

[James Robinson]

Comment on attachment 118464 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 118464 [details]
Patch

Clearing flags on attachment: 118464

Committed r102400: <http://trac.webkit.org/changeset/102400>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.