[chromium] move pixel generation logic to TestRunner library
Created attachment 188067 [details] Patch
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 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.
(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
LGTM
#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 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
(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
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 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
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 on attachment 188424 [details] Patch Clearing flags on attachment: 188424 Committed r142929: <http://trac.webkit.org/changeset/142929>
All reviewed patches have been landed. Closing bug.