Bug 70822 – [chromium] Add testing for --enable-accelerated-drawing

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

[Alok Priyadarshi]

The accelerated drawing path in not ready for prime time. It has quite a few correctness and performance issues. So we cannot enable it for webkit_gpu_tests yet. While I am working on resolving the known issues, we need to add at least a single test for accelerated drawing to prevent it from breaking.

I am planning on following the pattern for --force-compositing-mode and add setAcceleratedDrawing() to windows.internal.

[Alok Priyadarshi]

Created attachment 112365 [details]
proposed patch

[James Robinson]

Comment on attachment 112365 [details]
proposed patch

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

> LayoutTests/platform/chromium/compositing/accelerated-drawing/alpha-expected.txt:1
> +layer at (0,0) size 800x600

why would you want a render tree dump for this test?

[Alok Priyadarshi]

Comment on attachment 112365 [details]
proposed patch

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

>> LayoutTests/platform/chromium/compositing/accelerated-drawing/alpha-expected.txt:1
>> +layer at (0,0) size 800x600
> 
> why would you want a render tree dump for this test?

It is not required. I thought all tests have render-tree. How can I disable the test from checking the render-tree?

[James Robinson]

layoutTestController.dumpAsText(true). Read http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree please

[Alok Priyadarshi]

Created attachment 112377 [details]
proposed patch

Removed render-tree.

[Adrienne Walker]

Bikeshed: Is there a point where we can rename accelerated drawing => accelerated painting to match the WebKit paint vs. compositor draw language used elsewhere?

[Alok Priyadarshi]

(In reply to comment #6)
> Bikeshed: Is there a point where we can rename accelerated drawing => accelerated painting to match the WebKit paint vs. compositor draw language used elsewhere?

We have had this discussion a few times before. WebKit has had WebCore::Settings::setAcceleratedDrawingEnabled() for a long time. When I started working on it, I named everything "drawing" to avoid confusion. I agree that "painting" is a better word. I could rename all instances in a different patch?

[James Robinson]

The mac port uses the accelerated drawing terminology so some cross-platform logic will use it. We shouldn't use it in our internal code, however.

[Adrienne Walker]

(In reply to comment #8)
> The mac port uses the accelerated drawing terminology so some cross-platform logic will use it. We shouldn't use it in our internal code, however.

Thanks for rehashing this argument for me.  I missed the point where the mac port was using that terminology.

[Gustavo Noronha (kov)]

Comment on attachment 112377 [details]
proposed patch

Attachment 112377 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10212519

[James Robinson]

Comment on attachment 112377 [details]
proposed patch

Looks like the trybots have cycled, so please add the appropriate exports to make this build everywhere. I'm fairly surprised this compiled unmodified on mac.

[Alok Priyadarshi]

Created attachment 112755 [details]
proposed patch

Moved the implementation of WebCore::Settings::setAcceleratedDrawingEnabled() to the header file to fix linking issues.

[Alok Priyadarshi]

(In reply to comment #11)
> (From update of attachment 112377 [details])
> Looks like the trybots have cycled, so please add the appropriate exports to make this build everywhere. I'm fairly surprised this compiled unmodified on mac.

It compiled on mac because __ZN7WebCore8Settings28setAcceleratedDrawingEnabledEb is defined in Source/WebCore/WebCore.exp.in.

[Alok Priyadarshi]

(In reply to comment #12)
> Created an attachment (id=112755) [details]
> proposed patch
> 
> Moved the implementation of WebCore::Settings::setAcceleratedDrawingEnabled() to the header file to fix linking issues.

Moving the implementation to header file now breaks the mac port because of the exported symbol in WebCore.exp.in. I see two options to fix the build on all bots:
1. Delete the exported symbol in WebCore.exp.in, or
2. Revert my changes in Settings, i.e. move the implementation back to Settings.cpp. Add the symbol in export files for gtk and win port.

Which option is better?

[Alok Priyadarshi]

Created attachment 112913 [details]
proposed patch

Removed __ZN7WebCore8Settings28setAcceleratedDrawingEnabledEb from WebCore.exp.in. Review not needed at this time. Just checking if it makes the mac bot green.

[James Robinson]

Comment on attachment 112913 [details]
proposed patch

Seems to pass EWS, so r=me

[Matthew Delaney]

Alok et al, could you make sure to CC me on any current and future patches that involve accelerated painting/drawing?

I'm not sure what others feel, but I think it'd be nice to have consistency here with respect to the exported symbols and locations of implementations. As in, I'd rather just have all the simple getter/setters in one place or the other. 

If there's a desire to change the name of the flag and has good reason, then let's decide what to rename it to and change it everywhere to have it be consistent.

Why is this test platform specific?

[Alok Priyadarshi]

(In reply to comment #17)
> Alok et al, could you make sure to CC me on any current and future patches that involve accelerated painting/drawing?

Will do. Although most of my patches are chromium specific.

> I'm not sure what others feel, but I think it'd be nice to have consistency here with respect to the exported symbols and locations of implementations. As in, I'd rather just have all the simple getter/setters in one place or the other. 

I agree. I dont know why all the simple setters are implemented in the source file. Moving them to the header file will make it easier to access them from DRT.

> If there's a desire to change the name of the flag and has good reason, then let's decide what to rename it to and change it everywhere to have it be consistent.

We feel that "painting" is better than "drawing".

> Why is this test platform specific?

This is a temporary test while we fix obvious issues with accelerated-drawing path in chromium. We should not be testing every graphics port as part of layout tests. As soon as the accelerated drawing path is ready, I plan to delete this test and update all image baselines.

[WebKit Review Bot]

Comment on attachment 112913 [details]
proposed patch

Clearing flags on attachment: 112913

Committed r98990: <http://trac.webkit.org/changeset/98990>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.