[webkit-reviews] review denied: [Bug 69048] Separate compositor platform support from webkit's platform support : [Attachment 109116] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Antoine Labour
<piman at chromium.org>'s request for review:
Bug 69048: Separate compositor platform support from webkit's platform support
https://bugs.webkit.org/show_bug.cgi?id=69048

Attachment 109116: Patch
https://bugs.webkit.org/attachment.cgi?id=109116&action=review

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


> Source/WebKit/chromium/public/WebKit.h:58
> +WEBKIT_EXPORT void initializeWithoutV8AndCompositor(WebKitPlatformSupport*);


lol, when we just recently added initializeWithoutV8, there was a debate about
whether or not we should just add flags to initialize().  I argued that since
we only had the use case of running without V8, that we should just go for the
simpler form.

I'm pretty sure that initializeWithoutV8AndCompositor is getting out of hand
though ;-)

This might be better:

  struct WebKitInitializationOptions {
      bool initializeV8;
      bool initializeCompositor;
      // ... default ctor sets them to true ...
  };
  WEBKIT_EXPORT void initializeWithOptions(WebKitPlatformSupport*, const
WebKitInitializationOptions&);

> Source/WebKit/chromium/src/WebKit.cpp:61
> +class WebKitCompositorSupport : public WebCore::CompositorSupportDelegate {

I'm guessing there is some next patch that will make it possible for there to
be
a CompositorSupportDelegate that doesn't just thunk to WebKitPlatformSupport?
Can you explain how that might look?

Also, it seems like the 4 functions on this interface could be defined as safe
to
call from any thread in WebKit.  I'm not quite sure why they need to be
factored
out like this.	It may already be the case that they are safe to call from
background
threads.  I think webworkers can query the current time, and the trace and
histogram
systems are threadsafe.  Am I missing something?