Bug 109686 - [chromium] move pixel generation logic to TestRunner library
Summary: [chromium] move pixel generation logic to TestRunner library
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks: 108469
  Show dependency treegraph
 
Reported: 2013-02-13 06:13 PST by jochen
Modified: 2013-02-14 15:52 PST (History)
7 users (show)

See Also:


Attachments
Patch (34.73 KB, patch)
2013-02-13 06:13 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (34.76 KB, patch)
2013-02-13 11:39 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (34.13 KB, patch)
2013-02-14 01:36 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (34.54 KB, patch)
2013-02-14 13:38 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (34.45 KB, patch)
2013-02-14 14:18 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2013-02-13 06:13:11 PST
[chromium] move pixel generation logic to TestRunner library
Comment 1 jochen 2013-02-13 06:13:34 PST
Created attachment 188067 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-13 06:16:08 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Tony Chang 2013-02-13 10:21:52 PST
Comment on attachment 188067 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:60
> +#include "skia/ext/platform_canvas.h"

This is a layering violation, but I guess you're just moving it.

It looks like this is only needed for skia::CreateBitmapCanvas and skia::TryCreateBitmapCanvas.  Maybe we can move this into WebTestDelegate?
Comment 4 jochen 2013-02-13 10:24:24 PST
Comment on attachment 188067 [details]
Patch

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

>> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:60
>> +#include "skia/ext/platform_canvas.h"
> 
> This is a layering violation, but I guess you're just moving it.
> 
> It looks like this is only needed for skia::CreateBitmapCanvas and skia::TryCreateBitmapCanvas.  Maybe we can move this into WebTestDelegate?

It's not really a layering violation: the testrunner library is a WebKit embedder, i.e. it's above the WebKit API just like TestShell or content.

I also don't see the point in moving stuff to the WebTestDelegate which is then implemented exactly the same for TestShell and content_shell.
Comment 5 Tony Chang 2013-02-13 10:52:59 PST
(In reply to comment #4)
> (From update of attachment 188067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188067&action=review
> 
> >> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:60
> >> +#include "skia/ext/platform_canvas.h"
> > 
> > This is a layering violation, but I guess you're just moving it.
> > 
> > It looks like this is only needed for skia::CreateBitmapCanvas and skia::TryCreateBitmapCanvas.  Maybe we can move this into WebTestDelegate?
> 
> It's not really a layering violation: the testrunner library is a WebKit embedder, i.e. it's above the WebKit API just like TestShell or content.

It's a problem in the same way that depending on base is a problem: If someone wants to refactor skia/ext/platform_canvas.h, they have to worry about not breaking code in WebKit and do a 3-way patch dance.  This is why we made webkit/support.

> I also don't see the point in moving stuff to the WebTestDelegate which is then implemented exactly the same for TestShell and content_shell.

Ah, right, WebTestDelegate doesn't help since DRT's WebTestDelegate is in the webkit repo.  Maybe we could add this to Platform/chromium/public/WebUnitTestSupport.h?

I think it would also be fine to add a FIXME above the #include line and punt.
Comment 6 jochen 2013-02-13 11:39:06 PST
Created attachment 188131 [details]
Patch
Comment 7 jochen 2013-02-13 11:41:39 PST
(In reply to comment #5)
> I think it would also be fine to add a FIXME above the #include line and punt.

ok
Comment 8 Tony Chang 2013-02-13 12:28:00 PST
LGTM
Comment 9 James Robinson 2013-02-13 12:50:11 PST
#including skia/ext/ is definitely busterated, but it's been busted for a long time and we haven't figured out how to solve it yet.
Comment 10 James Robinson 2013-02-13 13:27:09 PST
Comment on attachment 188131 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:188
> +    std::auto_ptr<SkCanvas> m_canvas;

This isn't right - SkCanvas is SkRefCnt'd, so you need to use the skia refcounting APIs to control ownership.
Comment 11 jochen 2013-02-13 23:46:51 PST
(In reply to comment #10)
> (From update of attachment 188131 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188131&action=review
> 
> > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:188
> > +    std::auto_ptr<SkCanvas> m_canvas;
> 
> This isn't right - SkCanvas is SkRefCnt'd, so you need to use the skia refcounting APIs to control ownership.

while it's true that it is SkRefCnt'd, the pattern seems to be to use a scoped_ptr to hold it:

cs.chromium.org/SkRefPtr<SkCanvas> - zero hits
cs.chromium.org/scoped_ptr<SkCanvas> -  12 hits
cs.chromium.org/OwnPtr<SkCanvas> - 6 hits (including this ptr which I moved from WebViewHost)
Comment 12 jochen 2013-02-14 01:36:46 PST
Created attachment 188287 [details]
Patch
Comment 13 James Robinson 2013-02-14 10:37:13 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 188131 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=188131&action=review
> > 
> > > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:188
> > > +    std::auto_ptr<SkCanvas> m_canvas;
> > 
> > This isn't right - SkCanvas is SkRefCnt'd, so you need to use the skia refcounting APIs to control ownership.
> 
> while it's true that it is SkRefCnt'd, the pattern seems to be to use a scoped_ptr to hold it:
> 
> cs.chromium.org/SkRefPtr<SkCanvas> - zero hits
> cs.chromium.org/scoped_ptr<SkCanvas> -  12 hits
> cs.chromium.org/OwnPtr<SkCanvas> - 6 hits (including this ptr which I moved from WebViewHost)

Those are all bugs waiting to happen.  That's the same as deleting a RefCounted object in chromium.
Comment 14 jochen 2013-02-14 11:28:31 PST
(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 188131 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=188131&action=review
> > > 
> > > > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:188
> > > > +    std::auto_ptr<SkCanvas> m_canvas;
> > > 
> > > This isn't right - SkCanvas is SkRefCnt'd, so you need to use the skia refcounting APIs to control ownership.
> > 
> > while it's true that it is SkRefCnt'd, the pattern seems to be to use a scoped_ptr to hold it:
> > 
> > cs.chromium.org/SkRefPtr<SkCanvas> - zero hits
> > cs.chromium.org/scoped_ptr<SkCanvas> -  12 hits
> > cs.chromium.org/OwnPtr<SkCanvas> - 6 hits (including this ptr which I moved from WebViewHost)
> 
> Those are all bugs waiting to happen.  That's the same as deleting a RefCounted object in chromium.

No, it's not. SkCanvas has a public destructor, RefCounted objects don't. Also, the destructor DCHECKs that the refcount is one when it is invoked, so at least in debug builds, you'd get a backtrace when someone still holds a reference to the SkCanvas when it's deleted.

Anyway, I'm a bit wary of becoming the first person ever to hold a SkRefPtr to a SkCanvas, so what about consulting some skia folks?
Comment 15 jochen 2013-02-14 13:38:28 PST
Created attachment 188415 [details]
Patch
Comment 16 WebKit Review Bot 2013-02-14 13:49:12 PST
Attachment 188415 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp', u'Tools/DumpRenderTree/chromium/TestRunner/public/WebTestDelegate.h', u'Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h', u'Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h', u'Tools/DumpRenderTree/chromium/TestRunner/public/WebTestRunner.h', u'Tools/DumpRenderTree/chromium/TestRunner/src/TestInterfaces.cpp', u'Tools/DumpRenderTree/chromium/TestRunner/src/TestInterfaces.h', u'Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp', u'Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.h', u'Tools/DumpRenderTree/chromium/TestRunner/src/WebTestInterfaces.cpp', u'Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp', u'Tools/DumpRenderTree/chromium/TestShell.cpp', u'Tools/DumpRenderTree/chromium/WebViewHost.cpp', u'Tools/DumpRenderTree/chromium/WebViewHost.h']" exit_code: 1
Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:426:  auto_unref is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:638:  auto_unref is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Stephen White 2013-02-14 14:11:28 PST
Comment on attachment 188287 [details]
Patch

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

I don't think auto_ptr is any different than OwnPtr in this case.  You're essentially ignoring the refcount and calling the (public) destructor.  This is ok, since SkCanvas is designed to operate either with refcounts or on the stack (or so Mike tells me), so this should be safe.

I'll take a look at the rest of the code shortly.

> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:587
> +        m_canvas = std::auto_ptr<SkCanvas>(testCanvas);

I'm no auto_ptr expert, but could this be m_canvas.reset(testCanvas) instead?

> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:607
> +    m_canvas = std::auto_ptr<SkCanvas>(skia::CreateBitmapCanvas(scaledWidth, scaledHeight, true));

Same here.
Comment 18 jochen 2013-02-14 14:18:27 PST
Created attachment 188424 [details]
Patch
Comment 19 jochen 2013-02-14 14:18:57 PST
(In reply to comment #17)
> (From update of attachment 188287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188287&action=review
> 
> I don't think auto_ptr is any different than OwnPtr in this case.  You're essentially ignoring the refcount and calling the (public) destructor.  This is ok, since SkCanvas is designed to operate either with refcounts or on the stack (or so Mike tells me), so this should be safe.
> 
> I'll take a look at the rest of the code shortly.
> 
> > Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:587
> > +        m_canvas = std::auto_ptr<SkCanvas>(testCanvas);
> 
> I'm no auto_ptr expert, but could this be m_canvas.reset(testCanvas) instead?
> 
> > Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:607
> > +    m_canvas = std::auto_ptr<SkCanvas>(skia::CreateBitmapCanvas(scaledWidth, scaledHeight, true));
> 
> Same here.

yes, reset(ptr) also works, updated the patch
Comment 20 Stephen White 2013-02-14 14:45:07 PST
Comment on attachment 188424 [details]
Patch

I'm ok with the skia bits, so combine that with Tony's LGTM from comment 8 for the rest and r=me.
Comment 21 WebKit Review Bot 2013-02-14 15:52:36 PST
Comment on attachment 188424 [details]
Patch

Clearing flags on attachment: 188424

Committed r142929: <http://trac.webkit.org/changeset/142929>
Comment 22 WebKit Review Bot 2013-02-14 15:52:41 PST
All reviewed patches have been landed.  Closing bug.