Closed
Bug 745148
Opened 13 years ago
Closed 12 years ago
Support content processes pushing layer transactions directly to omtc in parent process
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: cjones, Assigned: cjones)
References
Details
Attachments
(12 files, 7 obsolete files)
19.03 KB,
patch
|
roc
:
feedback+
|
Details | Diff | Splinter Review |
1.25 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
2.42 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
11.62 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
7.60 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
8.06 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
6.13 KB,
patch
|
ajuma
:
review+
|
Details | Diff | Splinter Review |
28.76 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
222.73 KB,
patch
|
Details | Diff | Splinter Review | |
45.24 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition, v2
26.90 KB,
patch
|
BenWa
:
review+
cjones
:
superreview+
|
Details | Diff | Splinter Review |
32.47 KB,
patch
|
BenWa
:
review+
cjones
:
review+
|
Details | Diff | Splinter Review |
In b2g, we'll have a set of content processes that want to push their updates directly to the off-main-thread compositor in the top-most parent (server) process.
Basically, we need to hand the widget code an "opens" or "bridge" channel to the compositor's thread context.
Updated•12 years ago
|
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → jones.chris.g
Should we extend CompositorFrameParent to support this? We don't want to duplicate that code.
Assignee | ||
Comment 2•12 years ago
|
||
There's no such animal. My WIP is http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/file/6e11a9641634/745148-directcompositor if you want to get a sense of what things look like.
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
The real patch is of course much larger than this, but it's all boilerplate to get here.
I suspect adding a RefLayer type will be contentious, hence feedback request. Alternate suggestions welcome.
Attachment #636647 -
Attachment is obsolete: true
Attachment #640912 -
Flags: feedback?(roc)
Assignee | ||
Comment 5•12 years ago
|
||
Please ignore the |sCurrent| stuff in CompositorParent, it's a temporary hack.
Comment on attachment 640912 [details] [diff] [review]
Core of the logic being added here
Review of attachment 640912 [details] [diff] [review]:
-----------------------------------------------------------------
This seems reasonable assuming we can later extend RefLayer to refer to layer subtrees in the same layer manager, perhaps by adding a way to attach referent IDs to individual layers.
Attachment #640912 -
Flags: feedback?(roc) → feedback+
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 9•12 years ago
|
||
This really stinks, but blame win32 and its lack of fork().
The motivating problem is, when we're using PCompositor to send messages directly from the content main thread to the compositor thread in the parent process, we need to PCompositor::Open() *right* after launching the content process. Before any PBrowsers are created.
But to open the new channel, we have to use ShareHandle() on windows to send the child end of the pipe, and this requires having the HANDLE for the new process. And getting that HANDLE requires disk IO. Sigh.
This is bad, but I don't see a way around it, unfortunately. On the bright side though, we only do this when we're using off-main-thread compositing, which means we can hide any latency here with an async animation.
Attachment #641668 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 10•12 years ago
|
||
I had one more patch for you, but looking back over it I'm not sure it's needed. Will clean up and verify asap.
Assignee | ||
Comment 11•12 years ago
|
||
Oh yeah, I remember what the problem was now. Sigh.
So here's the story. Chapter 1. With content processes and OMTC, the main process does PCompositor::Open() to the content process, sending it a compositor Channel. Previously, when opening these kinds of Channels, we blocked until we received the other side's pid. That turns out to be unnecessary for "Opens" channels and child-process channels --- we already have the PID.
Who cares, you're probably asking yourself. Well, we PCompositor::Open() asynchronously from the parent main thread. The main thread goes back to its business, let's say painting using the sync PLayers protocol. Meanwhile, we initialize the CompositorParent on the compositor thread, blocking in AsyncChannel::Open() on the other-side pid to be returned by the content-process main thread. And when the content process starts up, the first thing it does is initialize the crash reporter, through sync SendPCrashReporterConstructor. Hm.
parent main thread --blocked on--> compositor thread
compositor thread --blocked on--> content process
content process --blocked on--> parent main thread
Whups. The reason compositor thread blocks on content process is just to find out its pid basically, even though it already has it. So we break the cycle there.
This patch doesn't wait for OnConnected, since we can send/recv IPC just fine before.
Attachment #641668 -
Attachment is obsolete: true
Attachment #641668 -
Flags: review?(bent.mozilla)
Attachment #641754 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 12•12 years ago
|
||
Chapter 2. After that fix, I noticed the content process would hang on startup sometimes. The content main thread was blocked on the sync PCrashReporter ctor again.
After some poking, I found that the ctor message was arriving at the parent process and being dispatch to the Channel listener. We were just never sending a reply, so the content process stayed hung.
Then this hypothesis came to me while eating pizza, and I added a MOZ_NOT_REACHED() to GeckoChildProcessHost::OnMessageReceived(), and it blew up. So what was happening is that the sync ctor message was being dropped on the floor.
This patch queues up those too-early messages and then hands them off for delivery when we switch channel listeners.
This bug has been latent forever. I think the combination of SyncLaunch() (waiting for OnConnected) and the sync PCrashReporter() ctor asap after Channel::Open exarcerbated the problem. But, even without those this bug could still pop up.
Attachment #641756 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 13•12 years ago
|
||
Same patch as before.
Attachment #641757 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 14•12 years ago
|
||
I'm going to reuse some machinery Nico built in bug 598868 for finding the right CompositorParent to trigger recomposition for on async updates.
Depends on: 598868
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #642046 -
Flags: review?(roc)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #642047 -
Flags: review?(ajuma)
Assignee | ||
Comment 17•12 years ago
|
||
This won't compile everywhere but I'll mop up those trivial compilation fixes in parallel.
Attachment #642049 -
Flags: superreview?(roc)
Attachment #642049 -
Flags: review?
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #642050 -
Flags: superreview?(roc)
Attachment #642050 -
Flags: review?(bgirard)
Assignee | ||
Comment 19•12 years ago
|
||
Not really a good way to split this up more, but it's mostly boilerplate. The CompositorParent hunks are most interesting. Please let me know if you don't comfortable reviewing the IPC bits.
Attachment #642053 -
Flags: review?(bgirard)
Attachment #642053 -
Flags: review?(ajuma)
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #642054 -
Flags: review?(roc)
Assignee | ||
Comment 21•12 years ago
|
||
Includes dependencies.
Attachment #641193 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #642047 -
Flags: review?(ajuma) → review+
Comment on attachment 642046 [details] [diff] [review]
part 4: Remove duplicated code in Basic*ContainerLayer
Review of attachment 642046 [details] [diff] [review]:
-----------------------------------------------------------------
Nice.
Attachment #642046 -
Flags: review?(roc) → review+
Updated•12 years ago
|
Attachment #641666 -
Flags: review?(bent.mozilla) → review+
Updated•12 years ago
|
Attachment #641754 -
Flags: review?(bent.mozilla) → review+
Comment on attachment 641756 [details] [diff] [review]
2: GeckoChildProcessHost can't drop messages on the floor. Queue them and hand them off to the *Channel.
Review of attachment 641756 [details] [diff] [review]:
-----------------------------------------------------------------
::: ipc/glue/AsyncChannel.cpp
@@ +715,5 @@
> +
> + // Dispatch whatever messages the previous listener had queued up.
> + while (!pending.empty()) {
> + OnMessageReceived(pending.front());
> + pending.pop();
Should you pop before calling OnMessageReceived in case it reenters somehow? Something like:
Message* msg = pending.front();
pending.pop();
OnMessageReceived(msg);
::: ipc/glue/GeckoChildProcessHost.cpp
@@ +692,4 @@
> void
> GeckoChildProcessHost::OnChannelError()
> {
> // XXXbent Notify that the child process is gone?
Should probably file a followup since this can happen too, right?
::: ipc/glue/GeckoChildProcessHost.h
@@ +106,5 @@
> base::ProcessArchitecture arch);
> +
> + // There's a short window in between we launch the subprocess and we
> + // hand the channel off that messages can be received by this, while
> + // it's still the channel listener. If so, the messages would
Nit: This comment is all sorts of grammatical nightmare. Must have been late ;) Let's clean it up a little.
Attachment #641756 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 24•12 years ago
|
||
(In reply to ben turner [:bent] from comment #23)
> Comment on attachment 641756 [details] [diff] [review]
> 2: GeckoChildProcessHost can't drop messages on the floor. Queue them and
> hand them off to the *Channel.
>
> Review of attachment 641756 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: ipc/glue/AsyncChannel.cpp
> @@ +715,5 @@
> > +
> > + // Dispatch whatever messages the previous listener had queued up.
> > + while (!pending.empty()) {
> > + OnMessageReceived(pending.front());
> > + pending.pop();
>
> Should you pop before calling OnMessageReceived in case it reenters somehow?
There's no possibility of reentry, but also no reason to use a nonstandard idiom. Will change.
> > {
> > // XXXbent Notify that the child process is gone?
>
> Should probably file a followup since this can happen too, right?
>
Sure. Our wild youth is catching up with us ;).
> ::: ipc/glue/GeckoChildProcessHost.h
> @@ +106,5 @@
> > base::ProcessArchitecture arch);
> > +
> > + // There's a short window in between we launch the subprocess and we
> > + // hand the channel off that messages can be received by this, while
> > + // it's still the channel listener. If so, the messages would
>
> Nit: This comment is all sorts of grammatical nightmare. Must have been late
> ;) Let's clean it up a little.
It was both late and a nightmare.
Comment on attachment 641757 [details] [diff] [review]
part 3: SyncLaunch() content processes when we're using direct-to-omtc drawing
Review of attachment 641757 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/ipc/ContentParent.cpp
@@ +5,5 @@
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> +#if defined(ANDROID) || defined(XP_UNIX)
> +# include <sys/time.h>
> +# include <sys/resource.h>
Aha! You're the one who does this crazy indent thing. Ordinarily I'd balk, but you did alphabetize everything... Tough choice.
Attachment #641757 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 26•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> (In reply to ben turner [:bent] from comment #23)
> > Comment on attachment 641756 [details] [diff] [review]
> > 2: GeckoChildProcessHost can't drop messages on the floor. Queue them and
> > hand them off to the *Channel.
> >
> > Review of attachment 641756 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > ::: ipc/glue/AsyncChannel.cpp
> > @@ +715,5 @@
> > > +
> > > + // Dispatch whatever messages the previous listener had queued up.
> > > + while (!pending.empty()) {
> > > + OnMessageReceived(pending.front());
> > > + pending.pop();
> >
> > Should you pop before calling OnMessageReceived in case it reenters somehow?
>
> There's no possibility of reentry, but also no reason to use a nonstandard
> idiom. Will change.
>
Oh, now I remember what this was doing. It's queue<Message> (not Message*), so the impl in the patch avoids a copy when calling OnMessageReceived. So I didn't change this.
Comment on attachment 642049 [details] [diff] [review]
part 6: Allow layer trees to be given IDs so that the referent can be used in another context
Review of attachment 642049 [details] [diff] [review]:
-----------------------------------------------------------------
This could also be considered an r+ if that helps.
Attachment #642049 -
Flags: superreview?(roc) → superreview+
Comment on attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition
Review of attachment 642050 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/Layers.h
@@ +1431,5 @@
> + virtual void InsertAfter(Layer* aChild, Layer* aAfter)
> + { MOZ_NOT_REACHED("no"); }
> +
> + virtual void RemoveChild(Layer* aChild)
> + { MOZ_NOT_REACHED("no"); }
It would be cleaner to not expose RefLayer's ContainerLayer-ness. Private inheritance might work but we don't often use that. This will be much easier to address once layers are split into content-side and compositor-side objects, so let's not worry about it for now.
@@ +1442,5 @@
> + * Set the ID of the layer's referent.
> + */
> + void SetReferentId(int64_t aId)
> + {
> + mId = aId;
Assert that aId is >= 0?
::: gfx/layers/ipc/CompositorParent.cpp
@@ +28,5 @@
> static Thread* sCompositorThread = nsnull;
>
> +/**
> + */
> +static Layer* GetIndirectShadowTree(int64_t aId);
Remove non-comment
Attachment #642050 -
Flags: superreview?(roc) → superreview+
Comment on attachment 642054 [details] [diff] [review]
part 9: Hook up the pieces and enable direct compositor
Review of attachment 642054 [details] [diff] [review]:
-----------------------------------------------------------------
I don't understand why we need mIndirectLayerManager. It seems a bit vestigial. Why can't we use some other means to ensure RenderFrameParent creates a RefLayer?
::: layout/ipc/RenderFrameParent.cpp
@@ +552,5 @@
> +
> + nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> + if (!layer) {
> + // Probably a temporary layer manager that doesn't know how to
> + // use ref layers.
Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?
Assignee | ||
Comment 30•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Comment on attachment 642049 [details] [diff] [review]
> part 6: Allow layer trees to be given IDs so that the referent can be used
> in another context
>
> Review of attachment 642049 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This could also be considered an r+ if that helps.
Sure! Thanks.
Assignee | ||
Updated•12 years ago
|
Attachment #642049 -
Flags: superreview+
Attachment #642049 -
Flags: review?
Attachment #642049 -
Flags: review+
Assignee | ||
Comment 31•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Comment on attachment 642054 [details] [diff] [review]
> part 9: Hook up the pieces and enable direct compositor
>
> Review of attachment 642054 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I don't understand why we need mIndirectLayerManager. It seems a bit
> vestigial. Why can't we use some other means to ensure RenderFrameParent
> creates a RefLayer?
>
We don't absolutely need it. I'll reorganize things so that RenderFrameParent doesn't use that layer manager anymore.
> ::: layout/ipc/RenderFrameParent.cpp
> @@ +552,5 @@
> > +
> > + nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> > + if (!layer) {
> > + // Probably a temporary layer manager that doesn't know how to
> > + // use ref layers.
>
> Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?
We do.
Assignee | ||
Comment 32•12 years ago
|
||
Pushed 0-2 since those are more generic things we might step on
https://hg.mozilla.org/integration/mozilla-inbound/rev/d84d1d760483
https://hg.mozilla.org/integration/mozilla-inbound/rev/453481dbb828
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f19e4f90e7
Please leave this bug open.
Assignee | ||
Comment 33•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > Comment on attachment 642054 [details] [diff] [review]
> > part 9: Hook up the pieces and enable direct compositor
> >
> > Review of attachment 642054 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > I don't understand why we need mIndirectLayerManager. It seems a bit
> > vestigial. Why can't we use some other means to ensure RenderFrameParent
> > creates a RefLayer?
> >
>
> We don't absolutely need it. I'll reorganize things so that
> RenderFrameParent doesn't use that layer manager anymore.
>
Hmm ... I started to change this but it's actually somewhat nontrivial. How strongly do you feel about changing this for this bug?
Comment 34•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d84d1d760483
https://hg.mozilla.org/mozilla-central/rev/453481dbb828
https://hg.mozilla.org/mozilla-central/rev/d3f19e4f90e7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Assignee | ||
Comment 35•12 years ago
|
||
See comment 32. Thanks for merging!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 36•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #33)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > > Comment on attachment 642054 [details] [diff] [review]
> > > part 9: Hook up the pieces and enable direct compositor
> > >
> > > Review of attachment 642054 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > >
> > > I don't understand why we need mIndirectLayerManager. It seems a bit
> > > vestigial. Why can't we use some other means to ensure RenderFrameParent
> > > creates a RefLayer?
> > >
> >
> > We don't absolutely need it. I'll reorganize things so that
> > RenderFrameParent doesn't use that layer manager anymore.
> >
>
> Hmm ... I started to change this but it's actually somewhat nontrivial. How
> strongly do you feel about changing this for this bug?
Never mind again, I have to do pretty much the same thing for bug 750977, so might as well knock it out here.
Assignee | ||
Comment 37•12 years ago
|
||
Attachment #642054 -
Attachment is obsolete: true
Attachment #642054 -
Flags: review?(roc)
Attachment #642346 -
Flags: review?(roc)
Assignee | ||
Comment 38•12 years ago
|
||
With latest inbound and gaia, I'm seeing a ColorLayer being set as the root of the layer tree a content process. That breaks a lot of things.
Assignee | ||
Comment 39•12 years ago
|
||
This doesn't happen on a desktop-b2g build. Very disturbing.
Assignee | ||
Comment 40•12 years ago
|
||
We're hitting
#1 0x406d52c2 in PresShell::Paint (this=0x2187370, aViewToPaint=0x216e5f0, aWidgetToPaint=0x1f1d168, aDirtyRegion=..., aIntDirtyRegion=..., aWillSendDidPaint=true) at /home/cjones/mozilla/inbound/layout/base/nsPresShell.cpp:5293
nsRefPtr<ColorLayer> root = layerManager->CreateColorLayer();
if (root) {
nsPresContext* pc = GetPresContext();
nsIntRect bounds =
pc->GetVisibleArea().ToOutsidePixels(pc->AppUnitsPerDevPixel());
bgcolor = NS_ComposeColors(bgcolor, mCanvasBackgroundColor);
root->SetColor(bgcolor);
root->SetVisibleRegion(bounds);
layerManager->SetRoot(root);
I was under the impression that the root layer must be a container layer, but that's not mentioned anywhere in Layers.h. OK.
Assignee | ||
Comment 41•12 years ago
|
||
Folded this patch
void TemporarilyCompensateForContentScrollOffset(Layer* aContainer,
Layer* aShadowContent)
{
ContainerLayer* c = aShadowContent->AsContainerLayer();
+ if (!c) {
+ return;
+ }
into part 8.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #40)
> I was under the impression that the root layer must be a container layer,
> but that's not mentioned anywhere in Layers.h. OK.
Right, sometimes we paint before we've constructed a root frame.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > ::: layout/ipc/RenderFrameParent.cpp
> > @@ +552,5 @@
> > > +
> > > + nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> > > + if (!layer) {
> > > + // Probably a temporary layer manager that doesn't know how to
> > > + // use ref layers.
> >
> > Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?
>
> We do.
Under what conditions?
Assignee | ||
Comment 43•12 years ago
|
||
At least for -moz-element.
Comment 44•12 years ago
|
||
Comment on attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition
Review of attachment 642050 [details] [diff] [review]:
-----------------------------------------------------------------
r- since I'd like to see the clarifications below before it goes in even if they are minor.
::: gfx/layers/Layers.h
@@ +1421,5 @@
> + * Composition:
> + * look up subtree for GetReferentId()
> + * ConnectReferentLayer(subtree)
> + * compose
> + * ClearReferentLayer()
It's not obvious to me why you want to connect/clear the referent layer for every composite. I think it's worth documenting that here.
@@ +1442,5 @@
> + * Set the ID of the layer's referent.
> + */
> + void SetReferentId(int64_t aId)
> + {
> + mId = aId;
Instead of asserting that mId is >= 0 shouldn't we just use uint?
@@ +1462,5 @@
> + /**
> + * DRAWING PHASE ONLY
> + * |aLayer| is the same as the argument to ConnectReferentLayer().
> + */
> + void ClearReferentLayer(Layer* aLayer)
Maybe we should rename this to disconnect/detach to better reflect that we're not clearing the referent layer but rather just detaching.
@@ +1491,5 @@
> +
> + virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix);
> +
> + Layer* mTempReferent;
> + int64_t mId;
Should we document that '0' is a special value? If for nothing else it's useful for asserting that mId has been set without introducing a debug only flag.
::: gfx/layers/ipc/CompositorParent.cpp
@@ +294,5 @@
> +/**
> + * DRAWING PHASE ONLY
> + *
> + * For reach RefLayer in |aRoot|, look up its referent and connect it
> + * to the layer tree, if found. On exiting scope, clears all resolved
clear=disconnect?
@@ +313,5 @@
> +
> +private:
> + enum Op { Resolve, Clear };
> + template<Op OP>
> + void WalkTheTree(Layer* aLayer, Layer* aParent)
Why is this a template instead of making Op a parameter to WalkTheTree? As I understand it this will create two instance of this function.
@@ +332,5 @@
> + }
> +
> + Layer* mRoot;
> +
> + AutoResolveRefLayers(const AutoResolveRefLayers&);
MOZ_DELETE
::: gfx/layers/ipc/ShadowLayersParent.cpp
@@ +265,5 @@
> specific.get_CanvasLayerAttributes().filter());
> break;
>
> + case Specific::TRefLayerAttributes:
> + MOZ_LAYERS_LOG(("[ParentSide] canvas layer"));
s/canvas/reference
Attachment #642050 -
Flags: review?(bgirard) → review-
Comment 45•12 years ago
|
||
Comment on attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor
Review of attachment 642053 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with followups filed for the three "FIXME/bug XXXXX" comments.
Attachment #642053 -
Flags: review?(ajuma) → review+
Assignee | ||
Comment 46•12 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #44)
> ::: gfx/layers/Layers.h
> @@ +1421,5 @@
> > + * Composition:
> > + * look up subtree for GetReferentId()
> > + * ConnectReferentLayer(subtree)
> > + * compose
> > + * ClearReferentLayer()
>
> It's not obvious to me why you want to connect/clear the referent layer for
> every composite. I think it's worth documenting that here.
>
* Clients will usually want to Connect/Clear() on each transaction to
* avoid difficulties managing memory across multiple layer subtrees.
> @@ +1442,5 @@
> > + * Set the ID of the layer's referent.
> > + */
> > + void SetReferentId(int64_t aId)
> > + {
> > + mId = aId;
>
> Instead of asserting that mId is >= 0 shouldn't we just use uint?
>
It was supposed to be uint64_t, this is a typo. But we want to assert that it's != 0, i.e. a valid ID. Having a RefLayer with the "null ID" is meaningless.
> @@ +1462,5 @@
> > + /**
> > + * DRAWING PHASE ONLY
> > + * |aLayer| is the same as the argument to ConnectReferentLayer().
> > + */
> > + void ClearReferentLayer(Layer* aLayer)
>
> Maybe we should rename this to disconnect/detach to better reflect that
> we're not clearing the referent layer but rather just detaching.
>
Disconnect() is quite overloaded in gfx/layers. Switched to Detach*() isn't.
> @@ +1491,5 @@
> > +
> > + virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix);
> > +
> > + Layer* mTempReferent;
> > + int64_t mId;
>
> Should we document that '0' is a special value? If for nothing else it's
> useful for asserting that mId has been set without introducing a debug only
> flag.
>
// 0 is a special value that means "no ID".
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +294,5 @@
> > +/**
> > + * DRAWING PHASE ONLY
> > + *
> > + * For reach RefLayer in |aRoot|, look up its referent and connect it
> > + * to the layer tree, if found. On exiting scope, clears all resolved
>
> clear=disconnect?
>
s/clear/detach/
> @@ +313,5 @@
> > +
> > +private:
> > + enum Op { Resolve, Clear };
> > + template<Op OP>
> > + void WalkTheTree(Layer* aLayer, Layer* aParent)
>
> Why is this a template instead of making Op a parameter to WalkTheTree? As I
> understand it this will create two instance of this function.
>
That's what we want. Compilers are highly optimized to boil away constant parameters of templates.
> @@ +332,5 @@
> > + }
> > +
> > + Layer* mRoot;
> > +
> > + AutoResolveRefLayers(const AutoResolveRefLayers&);
>
> MOZ_DELETE
>
Fixed.
> ::: gfx/layers/ipc/ShadowLayersParent.cpp
> @@ +265,5 @@
> > specific.get_CanvasLayerAttributes().filter());
> > break;
> >
> > + case Specific::TRefLayerAttributes:
> > + MOZ_LAYERS_LOG(("[ParentSide] canvas layer"));
>
> s/canvas/reference
Fixed.
Assignee | ||
Comment 47•12 years ago
|
||
Addressed comments, carrying over sr=roc.
Attachment #642050 -
Attachment is obsolete: true
Attachment #642662 -
Flags: superreview+
Attachment #642662 -
Flags: review?(bgirard)
Assignee | ||
Comment 48•12 years ago
|
||
(In reply to Ali Juma [:ajuma] from comment #45)
> Comment on attachment 642053 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor
>
> Review of attachment 642053 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r+ with followups filed for the three "FIXME/bug XXXXX" comments.
Thanks for the catch! This slipped off my to-do list.
Assignee | ||
Comment 49•12 years ago
|
||
Carrying over r=ajuma.
Attachment #642053 -
Attachment is obsolete: true
Attachment #642053 -
Flags: review?(bgirard)
Attachment #642666 -
Flags: review?(bgirard)
Assignee | ||
Updated•12 years ago
|
Attachment #642666 -
Flags: review+
Comment 50•12 years ago
|
||
Comment on attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor
Review of attachment 642053 [details] [diff] [review]:
-----------------------------------------------------------------
Posting my review draft I wrote before the patch was updated.
::: gfx/layers/ipc/CompositorChild.cpp
@@ +44,5 @@
> +{
> + // There's only one compositor per child process.
> + MOZ_ASSERT(!sCompositor);
> +
> + nsRefPtr<CompositorChild> cc(new CompositorChild(nsnull));
Not a fan of variable shorthand.
@@ +57,5 @@
> + NS_RUNTIMEABORT("Couldn't Open() Compositor channel.");
> + return nsnull;
> + }
> + // We release this ref in ActorDestroy().
> + return sCompositor = cc.forget().get();
Why not make sCompositor a RefPtr?
@@ +78,5 @@
> +CompositorChild::ActorDestroy(ActorDestroyReason aWhy)
> +{
> + MOZ_ASSERT(sCompositor == this);
> + sCompositor = NULL;
> + MessageLoop::current()->PostTask(
Comment to explain why we're posting this as a task would be nice. Is this accurate/
// Release doesn't need to happen synchronously, postpone it.
::: gfx/layers/ipc/CompositorChild.h
@@ +43,4 @@
> private:
> nsRefPtr<LayerManager> mLayerManager;
>
> + // When we're in a child process, this is the process-global
What about when we're not in a child process? Maybe we can add a getter and assert if this is only valid in the child process?
::: gfx/layers/ipc/CompositorParent.cpp
@@ +28,5 @@
> namespace mozilla {
> namespace layers {
>
> +// FIXME/bug XXXXXX: There can only be one. No wait, can be *more*
> +// than one. Fix this.
If you plan on landing this as-is you should clarify the comment.
Attachment #642053 -
Flags: review+
Updated•12 years ago
|
Attachment #642662 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 51•12 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #50)
> Comment on attachment 642053 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor
>
> ::: gfx/layers/ipc/CompositorChild.cpp
> @@ +44,5 @@
> > +{
> > + // There's only one compositor per child process.
> > + MOZ_ASSERT(!sCompositor);
> > +
> > + nsRefPtr<CompositorChild> cc(new CompositorChild(nsnull));
>
> Not a fan of variable shorthand.
>
s/cc/child/
> @@ +57,5 @@
> > + NS_RUNTIMEABORT("Couldn't Open() Compositor channel.");
> > + return nsnull;
> > + }
> > + // We release this ref in ActorDestroy().
> > + return sCompositor = cc.forget().get();
>
> Why not make sCompositor a RefPtr?
>
It creates static ctors which slow down startup. We need jlebar's StaticRefPtr to fix this properly.
> @@ +78,5 @@
> > +CompositorChild::ActorDestroy(ActorDestroyReason aWhy)
> > +{
> > + MOZ_ASSERT(sCompositor == this);
> > + sCompositor = NULL;
> > + MessageLoop::current()->PostTask(
>
> Comment to explain why we're posting this as a task would be nice. Is this
> accurate/
> // Release doesn't need to happen synchronously, postpone it.
>
// We don't want to release the ref to sCompositor here, during
// cleanup, because that will cause it to be deleted while it's
// still being used. So defer the deletion to after it's not in
// use.
> ::: gfx/layers/ipc/CompositorChild.h
> @@ +43,4 @@
> > private:
> > nsRefPtr<LayerManager> mLayerManager;
> >
> > + // When we're in a child process, this is the process-global
>
> What about when we're not in a child process? Maybe we can add a getter and
> assert if this is only valid in the child process?
>
I don't feel very strongly about this, but went ahead and moved the getter into the .cpp and added an assert.
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +28,5 @@
> > namespace mozilla {
> > namespace layers {
> >
> > +// FIXME/bug XXXXXX: There can only be one. No wait, can be *more*
> > +// than one. Fix this.
>
> If you plan on landing this as-is you should clarify the comment.
Yeah, it was early in the morning
// FIXME/bug 774386: we're assuming that there's only one
// CompositorParent, but that's not always true. This assumption only
// affects CrossProcessCompositorParent below.
Attachment #642346 -
Flags: review?(roc) → review+
Assignee | ||
Comment 52•12 years ago
|
||
Attachment #642666 -
Attachment is obsolete: true
Attachment #642666 -
Flags: review?(bgirard)
Attachment #642682 -
Flags: review?(bgirard)
Assignee | ||
Comment 53•12 years ago
|
||
Comment on attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3
Carrying over r=ajuma.
Attachment #642682 -
Flags: review+
Comment 54•12 years ago
|
||
Comment on attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3
Review of attachment 642682 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/ipc/CompositorParent.cpp
@@ +736,5 @@
> + static uint64_t ids;
> + return ++ids;
> +}
> +
> +/** */
Missing a comment here. Maybe how this class differs from PCompositorParent.
@@ +814,2 @@
> {
> + LayerTreeMap::const_iterator cit = sIndirectLayerTrees.find(aId);
Quite a few variable acronym in this page. I have no idea what the 'c' stands for here, let's use something more obvious.
Attachment #642682 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 55•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> Comment on attachment 642050 [details] [diff] [review]
> part 7: Create a RefLayer type to temporarily contain a foreign layer
> subtree during composition
>
> @@ +1442,5 @@
> > + * Set the ID of the layer's referent.
> > + */
> > + void SetReferentId(int64_t aId)
> > + {
> > + mId = aId;
>
> Assert that aId is >= 0?
>
Done, --> uint64_t and asserted != 0.
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +28,5 @@
> > static Thread* sCompositorThread = nsnull;
> >
> > +/**
> > + */
> > +static Layer* GetIndirectShadowTree(int64_t aId);
>
> Remove non-comment
I meant to document this ... added
/**
* Lookup the indirect shadow tree for |aId| and return it if it
* exists. Otherwise null is returned. This must only be called on
* the compositor thread.
*/
Assignee | ||
Comment 56•12 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #54)
> Comment on attachment 642682 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor, v3
>
> Review of attachment 642682 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +736,5 @@
> > + static uint64_t ids;
> > + return ++ids;
> > +}
> > +
> > +/** */
>
> Missing a comment here. Maybe how this class differs from PCompositorParent.
>
/**
* This class handles layer updates pushed directly from child
* processes to the compositor thread. It's associated with a
* CompositorParent on the compositor thread. While it uses the
* PCompositor protocol to manage these updates, it doesn't actually
* drive compositing itself. For that it hands off work to the
* CompositorParent it's associated with.
*/
> @@ +814,2 @@
> > {
> > + LayerTreeMap::const_iterator cit = sIndirectLayerTrees.find(aId);
>
> Quite a few variable acronym in this page. I have no idea what the 'c'
> stands for here, let's use something more obvious.
In this case here, "cit" is an STL idiom that's shorthand for "Const ITerator".
Assignee | ||
Comment 57•12 years ago
|
||
Kinda cute: every single try build failed on https://tbpl.mozilla.org/?tree=Try&rev=83b060905e12, but all of my local {mac,android,b2g,linux} builds succeed. (Even with clobbers.) :|
Assignee | ||
Comment 58•12 years ago
|
||
And the rest
https://hg.mozilla.org/integration/mozilla-inbound/rev/71300529a740
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed87a3a43a6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/13059586b02b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df6cd42330d
https://hg.mozilla.org/integration/mozilla-inbound/rev/100fd0a81f9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeafc64692a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5808967288
Comment 59•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/71300529a740
https://hg.mozilla.org/mozilla-central/rev/ed87a3a43a6c
https://hg.mozilla.org/mozilla-central/rev/13059586b02b
https://hg.mozilla.org/mozilla-central/rev/1df6cd42330d
https://hg.mozilla.org/mozilla-central/rev/100fd0a81f9e
https://hg.mozilla.org/mozilla-central/rev/aeafc64692a5
https://hg.mozilla.org/mozilla-central/rev/4a5808967288
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla16 → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•