Bug 74017 - [Chromium] Add per-tile painting flag to DumpRenderTree and rename AcceleratedDrawing to AcceleratedPainting in chromium specific code.
Summary: [Chromium] Add per-tile painting flag to DumpRenderTree and rename Accelerate...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on:
Blocks: 74125
  Show dependency treegraph
 
Reported: 2011-12-07 11:55 PST by David Reveman
Modified: 2011-12-08 17:05 PST (History)
8 users (show)

See Also:


Attachments
Patch (15.41 KB, patch)
2011-12-07 12:08 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (19.56 KB, patch)
2011-12-08 14:03 PST, David Reveman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2011-12-07 11:55:43 PST
Add support for command line flag enable-per-tile-drawing in DumpRenderTree.
Comment 1 David Reveman 2011-12-07 12:08:19 PST
Created attachment 118250 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-07 12:11:01 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-12-07 14:51:00 PST
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.
Comment 4 David Reveman 2011-12-07 14:58:14 PST
(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.
Comment 5 WebKit Review Bot 2011-12-07 15:06:30 PST
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
Comment 6 James Robinson 2011-12-07 17:36:19 PST
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
Comment 7 David Reveman 2011-12-07 17:45:30 PST
(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.
Comment 8 Alok Priyadarshi 2011-12-07 21:05:14 PST
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"?
Comment 9 David Reveman 2011-12-08 09:02:49 PST
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.
Comment 10 James Robinson 2011-12-08 11:59:41 PST
I disagree.  We should keep all chromium-specific code consistent, at the very least.
Comment 11 David Reveman 2011-12-08 14:03:09 PST
Created attachment 118464 [details]
Patch
Comment 12 James Robinson 2011-12-08 14:05:08 PST
Comment on attachment 118464 [details]
Patch

R=me
Comment 13 WebKit Review Bot 2011-12-08 17:05:06 PST
Comment on attachment 118464 [details]
Patch

Clearing flags on attachment: 118464

Committed r102400: <http://trac.webkit.org/changeset/102400>
Comment 14 WebKit Review Bot 2011-12-08 17:05:11 PST
All reviewed patches have been landed.  Closing bug.