Bug 95040 – [chromium] Add CompositorSupport interface for constructing compositor classes

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

[James Robinson]

[chromium] Add CompositorSupport interface for constructing compositor classes

[James Robinson]

Created attachment 160627 [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.

[Adam Barth]

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.

[James Robinson]

I'll try using the fancy new WebPassOwnPtr<> type here and see how it works out.

[Adam Barth]

Woah, we have WebPassOwnPtr now!  That's exciting.

[James Robinson]

(In reply to comment #5)
> Woah, we have WebPassOwnPtr now!  That's exciting.

Not yet - we will soon though :)

[James Robinson]

Created attachment 160797 [details]
Patch

[James Robinson]

Check it out - I don't have to document ownership transfers any more since it's in the type system. Woohoo!

[WebKit Review Bot]

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.

[James Robinson]

Created attachment 160810 [details]
Remove default WebPassOwnPtr constructor, it's not useful without a copy ctor

[WebKit Review Bot]

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.

[James Robinson]

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.

[James Robinson]

Created attachment 160861 [details]
Patch

[James Robinson]

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.

[WebKit Review Bot]

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.

[James Robinson]

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.

[Darin Fisher (:fishd, Google)]

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.

[James Robinson]

Created attachment 161350 [details]
consistent fallback behavior

[James Robinson]

Created attachment 161351 [details]
add example of implementing in WebKit and using on chromium side

[James Robinson]

This assumes that if compositorSupport is not null, it'll return the types we want.

[WebKit Review Bot]

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.

[James Robinson]

Committed r127172: <http://trac.webkit.org/changeset/127172>