WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
Bug 88821
[chromium] Quad list IPC infrastructure
https://bugs.webkit.org/show_bug.cgi?id=88821
Summary
[chromium] Quad list IPC infrastructure
Alexandre Elias
Reported
2012-06-11 17:10:44 PDT
[chromium] Quad list IPC infrastructure
Attachments
Patch
(9.96 KB, patch)
2012-06-11 17:13 PDT
,
Alexandre Elias
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Alexandre Elias
Comment 1
2012-06-11 17:13:06 PDT
Created
attachment 146970
[details]
Patch
WebKit Review Bot
Comment 2
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
.
Dana Jansens
Comment 3
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?
James Robinson
Comment 4
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"
Alexandre Elias
Comment 5
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?
James Robinson
Comment 6
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.
Dana Jansens
Comment 7
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?
James Robinson
Comment 8
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.
Alexandre Elias
Comment 9
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.
James Robinson
Comment 10
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?
Alexandre Elias
Comment 11
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.
Alexandre Elias
Comment 12
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.
WebKit Review Bot
Comment 13
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
Darin Fisher (:fishd, Google)
Comment 14
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.
Darin Fisher (:fishd, Google)
Comment 15
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.
Alexandre Elias
Comment 16
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug