[chromium] Add CompositorSupport interface for constructing compositor classes
Created attachment 160627 [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 160627 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160627&action=review I was hoping I could review this, but I just have silly comments about whitespace. :) > Source/Platform/chromium/public/WebCompositorSupport.h:55 > + // May return nil if initialization fails. Does not take ownership of the provided client or root layer. nil -> 0 > Source/Platform/chromium/public/WebCompositorSupport.h:59 > + // Layers ------------------------------------------------------- Please add a blank line below this line and one more blank line above this line. > Source/Platform/chromium/public/WebCompositorSupport.h:80 > + // Animation ---------------------------------------------------- Please add a blank line below this line. > Source/WebKit/chromium/src/LinkHighlight.cpp:44 > + This blank line isn't needed.
I'll try using the fancy new WebPassOwnPtr<> type here and see how it works out.
Woah, we have WebPassOwnPtr now! That's exciting.
(In reply to comment #5) > Woah, we have WebPassOwnPtr now! That's exciting. Not yet - we will soon though :)
Created attachment 160797 [details] Patch
Check it out - I don't have to document ownership transfers any more since it's in the type system. Woohoo!
Attachment 160797 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/Platform/chromium/public/WebPassOwnPtr.h:85: scoped_ptr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 160810 [details] Remove default WebPassOwnPtr constructor, it's not useful without a copy ctor
Attachment 160810 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/Platform/chromium/public/WebPassOwnPtr.h:80: scoped_ptr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin pointed out it would be super handy to have default implementations of the create..() functions so it's easier to modify the interface without having to land implementations of the new functions chromium-side first. However, I don't know how to write a default implementation of these since WebPassOwnPtr<T> is not copyable (by design) since users should only be using it to put into a OwnPtr<T> or scoped_ptr<T> depending on which side of the interface they are on.
Created attachment 160861 [details] Patch
This provides default implementations of WebCompositorSupport, which Darin points out will make modifying the interface a hell of a lot easier. To do this we've (mostly accidentally) implemented move emulation for WebPassOwnPtr<> so that we can return a temporary null value.
Attachment 160861 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/Platform/chromium/public/WebPassOwnPtr.h:85: scoped_ptr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
One consequence of this is that WebPassOwnPtr<T> is a really weird type. I think we should just make sure it's never declared as a local or member variable, or really used at all outside of declaring + implementing WebKit APIs.
Comment on attachment 160861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160861&action=review > Source/Platform/chromium/public/WebPassOwnPtr.h:35 > +#include "base/memory/scoped_ptr.h" This adds a new circular dependency between WebKit and Chromium, but it is probably fairly unlikely that this path will ever change. > Source/Platform/chromium/public/WebPassOwnPtr.h:52 > +// OwnPtr<Foo> foo = ...->createFoo(); It would be good to complete this by showing how to implement on the WebKit side and call on the Chromium side.
Created attachment 161350 [details] consistent fallback behavior
Created attachment 161351 [details] add example of implementing in WebKit and using on chromium side
This assumes that if compositorSupport is not null, it'll return the types we want.
Attachment 161351 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/Platform/chromium/public/WebPassOwnPtr.h:98: scoped_ptr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r127172: <http://trac.webkit.org/changeset/127172>