Bug 88821 - [chromium] Quad list IPC infrastructure
Summary: [chromium] Quad list IPC infrastructure
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandre Elias
URL:
Keywords:
Depends on: 90582 90736
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-11 17:10 PDT by Alexandre Elias
Modified: 2013-04-15 07:50 PDT (History)
11 users (show)

See Also:


Attachments
Patch (9.96 KB, patch)
2012-06-11 17:13 PDT, Alexandre Elias
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2012-06-11 17:10:44 PDT
[chromium] Quad list IPC infrastructure
Comment 1 Alexandre Elias 2012-06-11 17:13:06 PDT
Created attachment 146970 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-11 17:17:38 PDT
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 3 Dana Jansens 2012-06-11 17:20:28 PDT
Comment on attachment 146970 [details]
Patch

I'm not sure what this patch is meant to do, and the changelog didn't help so some questions. Why is this changing InputHandler stuff? And why is a frame a single RenderPass?
Comment 4 James Robinson 2012-06-11 17:24:13 PDT
Comment on attachment 146970 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146970&action=review

> Source/WebKit/chromium/public/WebCompositorFrame.h:40
> +struct WebCompositorMatrix {

would using WebTransformationMatrix instead work for you? we should not need two matrix types

> Source/WebKit/chromium/public/WebCompositorFrame.h:59
> +struct WebCompositorQuad {

One type per header, please

> Source/WebKit/chromium/public/WebCompositorFrame.h:72
> +struct WebCompositorFrame {

This is misplaced - this should live in Platform/chromium/... so the compositor implementation can depend on it directly. it doesn't depend on any other WebKit client API notions

> Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:52
> +    virtual void sendCompositorFrame(const WebCompositorFrame&) = 0;

This isn't an input handling concern, it shouldn't live on the input handler interface.  instead, we should wire up a peer class for "other" compositor thread stuff.

One possible refactoring, closer to the original design, would be to have WebCompositor::fromIdentifier() return a class with tear-offs for input handling and for whatever we're calling this

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:345
> +    WTF::Vector<WebCompositorQuad> destQuadList;

nit: you pretty much never say WTF:: in WebKit code, <wtf/Vector.h> has a using statement that pulls WTF::Vector into the global namespace so you can use it as just "Vector"
Comment 5 Alexandre Elias 2012-06-11 17:34:55 PDT
See also Chromium side: https://chromiumcodereview.appspot.com/10546115

You guys beat me to review because of the auto-CC, I was going to post some initial comments explaining some of this :)

This is an initial version I've tested works to pipe a quad from LayerRendererChromium to RenderWidgetHostImpl.  It takes the simplest approach of using the same structures for going across the WebKit->Chromium boundary and IPC boundary.  This involves the following tradeoffs:

- We need to introduce yet another matrix data structure because WebTransformationMatrix is hostile to being pickled.
- The data format will get bloated because the WebCompositorQuad will need to include every possible field for a derived quad.

(In reply to comment #4)
> (From update of attachment 146970 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146970&action=review
> 
> > Source/WebKit/chromium/public/WebCompositorFrame.h:40
> > +struct WebCompositorMatrix {
> 
> would using WebTransformationMatrix instead work for you? we should not need two matrix types

I tried to add message_traits to WebTransformationMatrix to pickle it, but it didn't work -- the ownership semantics of the internal pointer are strange, and I'm not sure it's even allowed to have a WebCore object living in another process.

The alternative to introducing this new matrix type would be to make ui/gfx/transformation.h picklable and introduce yet another quad data structure on the Chromium side.

> > Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:52
> > +    virtual void sendCompositorFrame(const WebCompositorFrame&) = 0;
> 
> This isn't an input handling concern, it shouldn't live on the input handler interface.  instead, we should wire up a peer class for "other" compositor thread stuff.

Agreed it's an abuse of the class as currently named, but it makes sense at some level because it's an IPC going in the opposite direction, and InputHandler is essentially there to handle IPCs.  I was thinking that we could rename it to IPCHandler?
Comment 6 James Robinson 2012-06-11 17:42:38 PDT
I think it's more trouble than it's worth to try to design a WebKit API type that is complex, pickleable, and makes sense as WebKit API.  I would rather design a nice-looking WebKit API and figure out how to pickle/unpickle it in chromium code.
Comment 7 Dana Jansens 2012-06-11 17:44:15 PDT
(In reply to comment #5)
> - The data format will get bloated because the WebCompositorQuad will need to include every possible field for a derived quad.

This makes me kinda sad, I'm doing work to split quads up to be more specific so the drawing code is more clear - but then they can be even smaller for IPC too.

I think the quads can be entirely opaque to anything outside of the compositor, right? Can we make a TransportQuad thing, that has an enum (type of quad), a size, and a block of opaque memory of said size? And we just stick the quad into this opaque block of memory.. Maybe this is not very IPC friendly? Is there some way we can do better than this?
Comment 8 James Robinson 2012-06-11 17:47:17 PDT
(In reply to comment #5)
> > > Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:52
> > > +    virtual void sendCompositorFrame(const WebCompositorFrame&) = 0;
> > 
> > This isn't an input handling concern, it shouldn't live on the input handler interface.  instead, we should wire up a peer class for "other" compositor thread stuff.
> 
> Agreed it's an abuse of the class as currently named, but it makes sense at some level because it's an IPC going in the opposite direction, and InputHandler is essentially there to handle IPCs.  I was thinking that we could rename it to IPCHandler?

InputHandler is there to handle input events.  The fact that IPCs are involved (or that IPC is a thing that exists at all) is not a concern of the API or of WebKit at all.

WebKit::WebCompositor is intended to be the interface to the compositor thread - I think it's fine to have other handler functions accessible through that API.
Comment 9 Alexandre Elias 2012-06-11 18:03:22 PDT
Okay, seems reasonable.  I knew this patch was questionable and just wanted to put an initial version out for comments before working on it further.

(In reply to comment #7)
> (In reply to comment #5)
> > - The data format will get bloated because the WebCompositorQuad will need to include every possible field for a derived quad.
> 
> This makes me kinda sad, I'm doing work to split quads up to be more specific so the drawing code is more clear - but then they can be even smaller for IPC too.
> 
> I think the quads can be entirely opaque to anything outside of the compositor, right? Can we make a TransportQuad thing, that has an enum (type of quad), a size, and a block of opaque memory of said size? And we just stick the quad into this opaque block of memory.. Maybe this is not very IPC friendly? Is there some way we can do better than this?

Well, this would mean basically reinventing pickling.  Which makes a degree of sense because the types of structures pickling supports are not very rich for our purposes (no pointers or inheritance).  If we are going to use an alternate transfer mechanism, another alternative would be to use protocol buffers, which support dropping arbitrary fields.

Alternately, I was thinking one way to make pickling work without bloating the quads would be for the top-level WebCompositorFrame have a different vector for each type of quad, and have the SharedQuadState referred to by ID.  This would be way too ugly for WebKit API though.
Comment 10 James Robinson 2012-06-11 18:05:44 PDT
(In reply to comment #9)
> Okay, seems reasonable.  I knew this patch was questionable and just wanted to put an initial version out for comments before working on it further.
> 

Always a good idea!

> (In reply to comment #7)
> > (In reply to comment #5)
> > > - The data format will get bloated because the WebCompositorQuad will need to include every possible field for a derived quad.
> > 
> > This makes me kinda sad, I'm doing work to split quads up to be more specific so the drawing code is more clear - but then they can be even smaller for IPC too.
> > 
> > I think the quads can be entirely opaque to anything outside of the compositor, right? Can we make a TransportQuad thing, that has an enum (type of quad), a size, and a block of opaque memory of said size? And we just stick the quad into this opaque block of memory.. Maybe this is not very IPC friendly? Is there some way we can do better than this?
> 
> Well, this would mean basically reinventing pickling.  Which makes a degree of sense because the types of structures pickling supports are not very rich for our purposes (no pointers or inheritance).  If we are going to use an alternate transfer mechanism, another alternative would be to use protocol buffers, which support dropping arbitrary fields.
> 
> Alternately, I was thinking one way to make pickling work without bloating the quads would be for the top-level WebCompositorFrame have a different vector for each type of quad, and have the SharedQuadState referred to by ID.  This would be way too ugly for WebKit API though.

Oh, what a pickle!

The combination of API and repository boundaries are quite a pain in the butt here.  Maybe we could sync up offline and talk about timelines to see if there's some way to relax one of these constraints?
Comment 11 Alexandre Elias 2012-06-11 19:40:59 PDT
OK, after talking offline, we agreed to use the same trick as InputEvents to have variable-length pickling.  That should solve the bloat problem while still allowing us to pickle the WebKit API objects directly.
Comment 12 Alexandre Elias 2012-06-11 19:51:19 PDT
Hmm, but the pickled std::vector's really not going to work.  It has to be a vector of pointers.

I now think we have to write a custom pickler for the top-level WebCompositorFrame object.  Then we don't need the quads to contain a size field and we can use pointers everywhere.  The big custom pickler will take care of everything and we can have a perfectly clean WebKit API.
Comment 13 WebKit Review Bot 2012-06-11 21:47:55 PDT
Comment on attachment 146970 [details]
Patch

Attachment 146970 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12935929
Comment 14 Darin Fisher (:fishd, Google) 2012-06-12 00:16:10 PDT
NOTE:  We usually don't let IPC concerns leak into WebKit.  If a WebKit type is POD, then we can easily write an IPC::ParamTraits<> specialization for it, but otherwise, we typically create a mirror struct on the Chromium side for non-POD WebKit types.
Comment 15 Darin Fisher (:fishd, Google) 2012-07-08 23:17:24 PDT
Comment on attachment 146970 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146970&action=review

> Source/WebCore/ChangeLog:3
> +        [chromium] Quad list IPC infrastructure

For Chromium, we try to keep IPC details out of WebKit.  This means that the code
we add to WebKit should make sense without anyone having to think about IPC details,
ideally.

>>> Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:52
>>> +    virtual void sendCompositorFrame(const WebCompositorFrame&) = 0;
>> 
>> This isn't an input handling concern, it shouldn't live on the input handler interface.  instead, we should wire up a peer class for "other" compositor thread stuff.
>> 
>> One possible refactoring, closer to the original design, would be to have WebCompositor::fromIdentifier() return a class with tear-offs for input handling and for whatever we're calling this
> 
> Agreed it's an abuse of the class as currently named, but it makes sense at some level because it's an IPC going in the opposite direction, and InputHandler is essentially there to handle IPCs.  I was thinking that we could rename it to IPCHandler?

As mentioned above, please avoid IPC-specific details at the WebKit level.  A class named IPCHandler definitely conflicts with that goal :)

Catch-all routing classes tend to be problematic because they can easily grow unwieldy.  It is helpful when classes/interfaces have related methods.  In Chrome, RenderWidgetHelper is a good example of a catch-all routing class that has a number of unrelated methods.  It is a bit messy to have so many different concerns all jumbled up together.
Comment 16 Alexandre Elias 2012-07-09 09:11:03 PDT
Comment on attachment 146970 [details]
Patch

Based on this feedback, I split this patch into somewhat bigger compositor refactorings that result in a cleaner WebKit API: see Bug 90582 and Bug 90736.  Cancelling review on this one.