Bug 70822 - [chromium] Add testing for --enable-accelerated-drawing
Summary: [chromium] Add testing for --enable-accelerated-drawing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on: 71057
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-25 10:16 PDT by Alok Priyadarshi
Modified: 2011-11-01 12:19 PDT (History)
11 users (show)

See Also:


Attachments
proposed patch (10.07 KB, patch)
2011-10-25 11:27 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (9.85 KB, patch)
2011-10-25 12:42 PDT, Alok Priyadarshi
jamesr: review-
gustavo: commit-queue-
Details | Formatted Diff | Diff
proposed patch (11.34 KB, patch)
2011-10-27 14:36 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (12.10 KB, patch)
2011-10-28 14:44 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2011-10-25 10:16:28 PDT
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.
Comment 1 Alok Priyadarshi 2011-10-25 11:27:02 PDT
Created attachment 112365 [details]
proposed patch
Comment 2 James Robinson 2011-10-25 12:18:26 PDT
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?
Comment 3 Alok Priyadarshi 2011-10-25 12:23:00 PDT
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?
Comment 4 James Robinson 2011-10-25 12:26:27 PDT
layoutTestController.dumpAsText(true). Read http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree please
Comment 5 Alok Priyadarshi 2011-10-25 12:42:43 PDT
Created attachment 112377 [details]
proposed patch

Removed render-tree.
Comment 6 Adrienne Walker 2011-10-25 13:11:43 PDT
Bikeshed: Is there a point where we can rename accelerated drawing => accelerated painting to match the WebKit paint vs. compositor draw language used elsewhere?
Comment 7 Alok Priyadarshi 2011-10-25 13:17:47 PDT
(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?
Comment 8 James Robinson 2011-10-25 13:20:01 PDT
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.
Comment 9 Adrienne Walker 2011-10-25 13:21:04 PDT
(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.
Comment 10 Gustavo Noronha (kov) 2011-10-26 01:18:22 PDT
Comment on attachment 112377 [details]
proposed patch

Attachment 112377 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10212519
Comment 11 James Robinson 2011-10-26 11:12:06 PDT
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.
Comment 12 Alok Priyadarshi 2011-10-27 14:36:37 PDT
Created attachment 112755 [details]
proposed patch

Moved the implementation of WebCore::Settings::setAcceleratedDrawingEnabled() to the header file to fix linking issues.
Comment 13 Alok Priyadarshi 2011-10-28 10:53:54 PDT
(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.
Comment 14 Alok Priyadarshi 2011-10-28 10:57:03 PDT
(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?
Comment 15 Alok Priyadarshi 2011-10-28 14:44:29 PDT
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.
Comment 16 James Robinson 2011-10-29 14:25:03 PDT
Comment on attachment 112913 [details]
proposed patch

Seems to pass EWS, so r=me
Comment 17 Matthew Delaney 2011-10-29 19:41:16 PDT
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?
Comment 18 Alok Priyadarshi 2011-10-31 12:25:33 PDT
(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.
Comment 19 WebKit Review Bot 2011-11-01 12:19:15 PDT
Comment on attachment 112913 [details]
proposed patch

Clearing flags on attachment: 112913

Committed r98990: <http://trac.webkit.org/changeset/98990>
Comment 20 WebKit Review Bot 2011-11-01 12:19:21 PDT
All reviewed patches have been landed.  Closing bug.