Bug 109686 – [chromium] move pixel generation logic to TestRunner library

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[chromium] move pixel generation logic to TestRunner library

Created attachment 188067 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Tony Chang]

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 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.

[Tony Chang]

(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.

Created attachment 188131 [details]
Patch

(In reply to comment #5)
> I think it would also be fine to add a FIXME above the #include line and punt.

ok

[Tony Chang]

LGTM

[James Robinson]

#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.

[James Robinson]

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.

(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)

Created attachment 188287 [details]
Patch

[James Robinson]

(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.

(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?

Created attachment 188415 [details]
Patch

[WebKit Review Bot]

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.

[Stephen White]

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.

Created attachment 188424 [details]
Patch

(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

[Stephen White]

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.

[WebKit Review Bot]

Comment on attachment 188424 [details]
Patch

Clearing flags on attachment: 188424

Committed r142929: <http://trac.webkit.org/changeset/142929>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.