[webkit-reviews] review granted: [Bug 69181] [chromium] Add WebWidget API for accessing the current WebCompositor : [Attachment 109389] add basic test for compositorForIdentifier

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted James Robinson
<jamesr at chromium.org>'s request for review:
Bug 69181: [chromium] Add WebWidget API for accessing the current WebCompositor
https://bugs.webkit.org/show_bug.cgi?id=69181

Attachment 109389: add basic test for compositorForIdentifier
https://bugs.webkit.org/attachment.cgi?id=109389&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109389&action=review


R=me w/ these suggested changes.

> Source/WebKit/chromium/public/WebCompositor.h:43
> +    WEBKIT_EXPORT static WebCompositor* compositorForIdentifier(int);

nit: at callsites, this will look like
WebCompositor::compositorForIdentifier(x), but
since that says "compositor" twice, how about WebCompositor::fromIdentifier(x)
instead?
kinda like String::fromUTF8, etc.

> Source/WebKit/chromium/public/WebWidget.h:44
> +class WebCompositor;

nit: can delete this now

> Source/WebKit/chromium/public/WebWidgetClient.h:61
> +    virtual void didEnableAcceleratedCompositing(int compositorIdentifier) {
}

nit: I originally wanted to use the word "enable" here, but vangelis talked
me out of it, since we already use the term "enable" in the WebSettings API:
WebSettings::setAcceleratedCompositingEnabled(bool).  That's why we settled
on "activate", and then this term is used pretty consistently on the Chrome
side too IIRC.	it is OK to temporarily override a method name.

  didActivateAcceleratedCompositing(int compositorIdentifier) { }
  didDeactivateAcceleratedCompositing() { }

Hmm, actually... I wonder why we need to say "accelerated" in these method
names.	Everywhere else we pretty much leave off that prefix.

Could just go with this:

  didActivateCompositor(int compositorIdentifier) { }
  didDeactivateCompositor() { }

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:73
> +

nit: extra new line