Closed
Bug 1055760
Opened 10 years ago
Closed 10 years ago
Allow storing multiple FrameMetrics and APZC instances per layer
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(22 files, 21 obsolete files)
As part of the work for bug 967844 we need to be able to store multiple FrameMetrics and APZC instances per layer. See the discussion at bug 967844 comment 39 onwards to comment 51 (and maybe beyond...).
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
If we have multiple FM objects per layer, then we need multiple handoff parents per layer, and so it makes more sense just to move the handoff parent into the FrameMetrics.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8476887 -
Flags: feedback?(roc)
Attachment #8476887 -
Flags: feedback?(matt.woodrow)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8475978 -
Attachment is obsolete: true
Attachment #8475979 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
This was my idea to encapsulate the ugly that results from having multiple FrameMetrics per layer. I also updated the APZCTreeManager code to show how it would be used. For the most part it just replaces the Layer* with nsRefPtr<LayerMetricsWrapper>, provided that we have all the necessary delegator functions on LayerMetricsWrapper.
Also we can play around with making LayerMetricsWrapper less memory-intensive (it generates a lot of transient garbage while using it) but for now I just wanted it to be as close to a drop-in replacement as possible to assess if this is a viable way forward.
Attachment #8476889 -
Flags: feedback?(roc)
Attachment #8476889 -
Flags: feedback?(matt.woodrow)
Attachment #8476889 -
Flags: feedback?(bgirard)
Assignee | ||
Comment 7•10 years ago
|
||
Propagating the LayerMetricsWrapper into more places that use the FrameMetrics.
Attachment #8476890 -
Flags: feedback?(roc)
Attachment #8476890 -
Flags: feedback?(matt.woodrow)
Attachment #8476890 -
Flags: feedback?(bgirard)
Assignee | ||
Updated•10 years ago
|
Attachment #8476887 -
Flags: feedback?(bgirard)
Attachment #8476887 -
Flags: feedback?(roc) → feedback+
Comment on attachment 8476889 [details] [diff] [review]
Part 4 WIP - Add LayerMetricsWrapper to encapsulate multi-FrameMetrics ugliness
Review of attachment 8476889 [details] [diff] [review]:
-----------------------------------------------------------------
Looks like a reasonable idea.
Attachment #8476889 -
Flags: feedback?(roc) → feedback+
Comment on attachment 8476890 [details] [diff] [review]
Part 5 WIP - More LayerMetricsWrapper propagation
Review of attachment 8476890 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/Layers.h
@@ +346,5 @@
> * Returns a list of all descendant layers for which
> * GetFrameMetrics().IsScrollable() is true and that
> * do not already have an ancestor in the return list.
> */
> + void GetScrollableLayers(nsTArray<nsRefPtr<LayerMetricsWrapper> >& aArray);
I don't feel good about exposing LayerMetricsWrapper in Layers.h. Can we keep it private to the compositor/APZC?
Attachment #8476890 -
Flags: feedback?(roc) → feedback-
Assignee | ||
Comment 10•10 years ago
|
||
I can put it in some other header file but i still would like to use it for the two functions in Layers.cpp. GetPrimaryScrollableFrame in particular is called from the tiling code and if I don't use it for that function then it's going to be hard to use LayerMetricsWrapper in the rest of the tiling code as well.
Put it in a separate header file, that's OK.
Comment 12•10 years ago
|
||
Comment on attachment 8476887 [details] [diff] [review]
Part 2 WIP - Change Layers API
Review of attachment 8476887 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/Layers.h
@@ +1553,5 @@
> void* mImplData;
> nsRefPtr<Layer> mMaskLayer;
> gfx::UserData mUserData;
> nsIntRegion mVisibleRegion;
> + nsTArray<FrameMetrics> mFrameMetrics;
Do we have somewhere appropriate to document what it means for a layer to have multiple FM. This is probably a good place to explain this. Better yet point to doxygen here.
Here's my attempt but I have only a partial understanding so perhaps you can write something better:
A layer can have multiple FM, this means that this layer has multiple scrollable ancestor. The same FM (matching scrollid) can be added to many layers, this means that the scrollable container has multiple active layer children. These may be interleaved with layers that don't match this FM which wont scroll. FM in the layer tree with the same scrollid should be equal.
Like I said on IRC it would be nice to have assertions to check that a layer doesn't have more than more FM with the same scroll id. And any FM in a layer tree with the same scroll id are equal.
Attachment #8476887 -
Flags: feedback?(bgirard) → feedback+
Comment 13•10 years ago
|
||
Comment on attachment 8476889 [details] [diff] [review]
Part 4 WIP - Add LayerMetricsWrapper to encapsulate multi-FrameMetrics ugliness
Review of attachment 8476889 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/Layers.h
@@ +1611,5 @@
> + * region and clip rect as the layer.
> + * This class purposely does not expose the target layer directly to avoid
> + * user code from accidentally calling functions directly on it. Instead
> + * any necessary functions should be wrapped in this class.
> + */
Maybe an ASCII art picture of what you sent me here would help. The comment explains this well but if you don't understand it's not evident what you're describing here.
http://asciiflow.com/ might help.
@@ +1637,5 @@
> +
> + nsRefPtr<LayerMetricsWrapper> GetLastChild() const
> + {
> + if (!AtBottomLayer()) {
> + return new LayerMetricsWrapper(mLayer, mIndex - 1);
iterating calls new which is slow and locks the arena. The iterator pattern would fix this but I'm not sure if it's worth it.
@@ +1655,5 @@
> + }
> + return nullptr;
> + }
> +
> + FrameMetrics Metrics() const
why not const&?
@@ +1708,5 @@
> + }
> +
> + nsIntRegion GetVisibleRegion() const
> + {
> + if (AtBottomLayer()) {
I don't understand why this is a special case but FN 1..n are not. This could use a comment.
@@ +1726,5 @@
> + {
> + return mLayer->GetContentDescription();
> + }
> +
> + void* LayerPointer() const
Not a fan of starting to use <X>Pointer. Normally we use Layer()/GetLayer().
Attachment #8476889 -
Flags: feedback?(bgirard) → feedback+
Assignee | ||
Comment 14•10 years ago
|
||
red try |
Try push with my latest set of patches: https://tbpl.mozilla.org/?tree=Try&rev=8e535f0634e6
I think this has all the code changes needed, but other than running the gtests I haven't tested it at all. It also needs documentation, and I'm going to convert the LayerMetricsWrapper to a MOZ_STACK_CLASS so we don't allocate gobs of them on the heap while walking around in the layer tree.
Assignee | ||
Comment 15•10 years ago
|
||
red try |
Updated to hopefully not break so badly. https://tbpl.mozilla.org/?tree=Try&rev=41ab4f1c9aed
Assignee | ||
Comment 16•10 years ago
|
||
orange try |
This one should be even better: https://tbpl.mozilla.org/?tree=Try&rev=85370fb53718
Assignee | ||
Comment 17•10 years ago
|
||
That last try push seems to be doing well, except for a bunch of android reftest failures which look worrisome. I also did some sanity checks locally on B2G and Fennec and neither had any obvious problems.
Assignee | ||
Comment 18•10 years ago
|
||
build-only try |
Now with LayerMetricsWrapper changed to be a MOZ_STACK_CLASS: https://tbpl.mozilla.org/?tree=Try&rev=380cf525a4e3
I'm able to repro the android reftest failures locally but having a hard time isolating and debugging them because for some reason my changes to the reftest harness are being ignored. :(
Assignee | ||
Comment 19•10 years ago
|
||
Bisection narrowed down the reftest failure to part 6 (cset b1bf7a5da754 in the try push from comment 18).
Assignee | ||
Comment 20•10 years ago
|
||
In hindsight https://bugzilla.mozilla.org/show_bug.cgi?id=982888#c36 was a poor decision on my part :)
Attachment #8476886 -
Attachment is obsolete: true
Attachment #8479138 -
Flags: review?(botond)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8479140 -
Flags: review?(botond)
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
This still needs more documentation
Attachment #8476887 -
Attachment is obsolete: true
Attachment #8476887 -
Flags: feedback?(matt.woodrow)
Assignee | ||
Comment 24•10 years ago
|
||
This also needs more documentation
Attachment #8476889 -
Attachment is obsolete: true
Attachment #8476889 -
Flags: feedback?(matt.woodrow)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8476890 -
Attachment is obsolete: true
Attachment #8476890 -
Flags: feedback?(matt.woodrow)
Attachment #8476890 -
Flags: feedback?(bgirard)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8476888 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Assignee | ||
Comment 28•10 years ago
|
||
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Comment 30•10 years ago
|
||
Assignee | ||
Comment 31•10 years ago
|
||
Assignee | ||
Comment 32•10 years ago
|
||
Assignee | ||
Comment 33•10 years ago
|
||
Assignee | ||
Comment 34•10 years ago
|
||
Assignee | ||
Comment 35•10 years ago
|
||
Assignee | ||
Comment 36•10 years ago
|
||
green try |
Try push that should have everything good (fingers crossed): https://tbpl.mozilla.org/?tree=Try&rev=5885415783de
Assignee | ||
Comment 37•10 years ago
|
||
Since I was touching this code I figured I might as well fix https://bugzilla.mozilla.org/show_bug.cgi?id=1056159#c9
Attachment #8479266 -
Flags: review?(botond)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8479142 -
Attachment is obsolete: true
Attachment #8479267 -
Flags: review?(roc)
Attachment #8479267 -
Flags: review?(bgirard)
Assignee | ||
Comment 39•10 years ago
|
||
Attachment #8479144 -
Attachment is obsolete: true
Attachment #8479269 -
Flags: review?(botond)
Attachment #8479269 -
Flags: review?(bgirard)
Assignee | ||
Comment 40•10 years ago
|
||
Attachment #8479145 -
Attachment is obsolete: true
Attachment #8479274 -
Flags: review?(botond)
Attachment #8479274 -
Flags: review?(bgirard)
Assignee | ||
Comment 41•10 years ago
|
||
Attachment #8479148 -
Attachment is obsolete: true
Attachment #8479276 -
Flags: review?(botond)
Assignee | ||
Comment 42•10 years ago
|
||
Attachment #8479149 -
Attachment is obsolete: true
Attachment #8479277 -
Flags: review?(botond)
Assignee | ||
Comment 43•10 years ago
|
||
Attachment #8479151 -
Attachment is obsolete: true
Attachment #8479279 -
Flags: review?(botond)
Assignee | ||
Comment 44•10 years ago
|
||
Attachment #8479152 -
Attachment is obsolete: true
Attachment #8479280 -
Flags: review?(botond)
Assignee | ||
Comment 45•10 years ago
|
||
I think some of the bits of code in ContainerRender now need to be moved out because they assume container layers are the only scrollable layers which is no longer true. However it's mostly debug/diagnostic tool code so I'll file a follow-up bug for that. I don't want it to hold up landing this in 34.
Attachment #8479153 -
Attachment is obsolete: true
Attachment #8479281 -
Flags: review?(botond)
Attachment #8479281 -
Flags: review?(bgirard)
Assignee | ||
Comment 46•10 years ago
|
||
Attachment #8479154 -
Attachment is obsolete: true
Attachment #8479283 -
Flags: review?(bgirard)
Assignee | ||
Comment 47•10 years ago
|
||
It looks like the patch you have on bug 967844 rewrites most of this code. Let me know if you'd rather I didn't land this patch.
Attachment #8479155 -
Attachment is obsolete: true
Attachment #8479285 -
Flags: review?(roc)
Assignee | ||
Comment 48•10 years ago
|
||
Yeah this is kinda ugly. I'll file a follow-up to do this more elegantly.
Attachment #8479156 -
Attachment is obsolete: true
Attachment #8479286 -
Flags: review?(botond)
Assignee | ||
Comment 49•10 years ago
|
||
Attachment #8479158 -
Attachment is obsolete: true
Attachment #8479287 -
Flags: review?(bgirard)
Assignee | ||
Comment 50•10 years ago
|
||
Attachment #8479159 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8479138 -
Flags: review?(botond) → review+
Assignee | ||
Comment 51•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #45)
> I think some of the bits of code in ContainerRender now need to be moved out
> because they assume container layers are the only scrollable layers which is
> no longer true. However it's mostly debug/diagnostic tool code so I'll file
> a follow-up bug for that. I don't want it to hold up landing this in 34.
Filed bug 1058884 for this.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #48)
> Yeah this is kinda ugly. I'll file a follow-up to do this more elegantly.
Filed bug 1058886 for this.
Comment 52•10 years ago
|
||
Comment on attachment 8479141 [details] [diff] [review]
Part 2 (diff ignoring whitespace changes)
Review of attachment 8479141 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +258,5 @@
> + // need to recurse any deeper because the fixed layers are relative to their
> + // nearest scrollable layer. ApplyAsyncContentTransformToTree will call this
> + // function again for nested scrollable layers.
> + if (aLayer == aTransformedSubtreeRoot || !needsAlignment) {
> + if (aLayer == aTransformedSubtreeRoot || !aLayer->GetFrameMetrics().IsScrollable()) {
'if (A || B) { if (A || C) { ... } }' can be folded into 'if (A || B || C) { ... }'
Attachment #8479141 -
Flags: review+
Comment 53•10 years ago
|
||
Comment on attachment 8479140 [details] [diff] [review]
Part 2 - Rearrange some code (no functional changes)
See r+ in whitespace-ignoring patch.
Attachment #8479140 -
Flags: review?(botond)
Comment 54•10 years ago
|
||
Comment on attachment 8479141 [details] [diff] [review]
Part 2 (diff ignoring whitespace changes)
Review of attachment 8479141 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +253,5 @@
> aTransformedSubtreeRoot->GetFrameMetrics().GetScrollId();
> + bool needsAlignment = (isRootFixed || isStickyForSubtree);
> +
> + // We want to process all the fixed and sticky children of
> + // aTransformedSubtreeRoot. Also, once we do encounter such a child, we don't
Should this say "once we encounter a scrollable child"?
Updated•10 years ago
|
Attachment #8479266 -
Flags: review?(botond) → review+
Attachment #8479267 -
Flags: review?(roc) → review+
Comment on attachment 8479285 [details] [diff] [review]
Part 9 - Update SetAsyncScrollOffset API
Review of attachment 8479285 [details] [diff] [review]:
-----------------------------------------------------------------
I'll change this again in my patch. (I've already done the rebase.) This is fine to land as-is.
Attachment #8479285 -
Flags: review?(roc) → review+
Assignee | ||
Comment 56•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #52)
> Comment on attachment 8479141 [details] [diff] [review]
>
> 'if (A || B) { if (A || C) { ... } }' can be folded into 'if (A || B || C)
> { ... }'
There's a return statement inside the first if but outside the second that prevents that from happening.
Comment 57•10 years ago
|
||
Comment on attachment 8479141 [details] [diff] [review]
Part 2 (diff ignoring whitespace changes)
Review of attachment 8479141 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> (In reply to Botond Ballo [:botond] from comment #52)
> > Comment on attachment 8479141 [details] [diff] [review]
> >
> > 'if (A || B) { if (A || C) { ... } }' can be folded into 'if (A || B || C)
> > { ... }'
>
> There's a return statement inside the first if but outside the second that
> prevents that from happening.
Oops, you're right! Ignore that.
::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +250,5 @@
> !aLayer->GetParent()->GetIsFixedPosition();
> bool isStickyForSubtree = aLayer->GetIsStickyPosition() &&
> aLayer->GetStickyScrollContainerId() ==
> aTransformedSubtreeRoot->GetFrameMetrics().GetScrollId();
> + bool needsAlignment = (isRootFixed || isStickyForSubtree);
I think it would enhance the readability of this code (particularly in conjunction with the comment below) if this were called isFixedOrSticky.
@@ +253,5 @@
> aTransformedSubtreeRoot->GetFrameMetrics().GetScrollId();
> + bool needsAlignment = (isRootFixed || isStickyForSubtree);
> +
> + // We want to process all the fixed and sticky children of
> + // aTransformedSubtreeRoot. Also, once we do encounter such a child, we don't
I think I answered my own question - no it shouldn't, we mean "a fixed or sticky child", and that's clear.
@@ +256,5 @@
> + // We want to process all the fixed and sticky children of
> + // aTransformedSubtreeRoot. Also, once we do encounter such a child, we don't
> + // need to recurse any deeper because the fixed layers are relative to their
> + // nearest scrollable layer. ApplyAsyncContentTransformToTree will call this
> + // function again for nested scrollable layers.
Please move the last sentence inside the outer 'if', as that pertains to why we are skipping scrollable layers.
Comment 58•10 years ago
|
||
Comment on attachment 8479269 [details] [diff] [review]
Part 4 - Update APZCTreeManager code and add a LayerMetricsWrapper class
Review of attachment 8479269 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with comments addressed. Nice work at abstracting away the multi-FrameMetrics messiness from APZCTreeManager!
::: gfx/layers/LayerMetricsWrapper.h
@@ +103,5 @@
> + *
> + * It is possible to wrap a nullptr in a LayerMetricsWrapper, in which case
> + * the IsValid() function will return false. This is required to allow
> + * LayerMetricsWrapper to be a MOZ_STACK_CLASS (desirable because it is used
> + * in loops and recursion).
The Maybe class might be a good fit here. We can leave any exploration of this to a later bug, though.
@@ +109,5 @@
> + * This class purposely does not expose the wrapped layer directly to avoid
> + * user code from accidentally calling functions directly on it. Instead
> + * any necessary functions should be wrapped in this class. It does expose
> + * the wrapped layer as a void* for printf purposes, and in case there is
> + * some use case where the underlying layer is actually required.
I'd remove "and in case ... actually required". Let's not encourage people to cast void*'s to things :)
@@ +153,5 @@
> + {
> + return mLayer != nullptr;
> + }
> +
> + operator bool() const
Please add MOZ_EXPLICIT_CONVERSION in front of this. Classes with implicit conversions to bool are Evil(tm).
(The use case you're going for, which is using a LayerMetricsWrapper object in the test-expression of a for loop, will still work.)
@@ +182,5 @@
> + FrameMetrics Metrics() const
> + {
> + MOZ_ASSERT(IsValid());
> +
> + if (mIndex >= mLayer->GetFrameMetricsCount()) {
Should this ever be called under these conditions? If not, perhaps we should assert instead. (As a bonus, the function can then return by reference-to-const).
@@ +192,5 @@
> + AsyncPanZoomController* Apzc() const
> + {
> + MOZ_ASSERT(IsValid());
> +
> + if (mIndex >= mLayer->GetFrameMetricsCount()) {
See comment about Metrics().
If, however, 'nullptr' is in fact a valid return value, the function should be called GetApzc().
@@ +195,5 @@
> +
> + if (mIndex >= mLayer->GetFrameMetricsCount()) {
> + return nullptr;
> + }
> + return mLayer->GetAsyncPanZoomController(mIndex);
Should we assert GetFrameMetrics(mIndex).IsScrollable()?
@@ +257,5 @@
> + const nsIntRect* GetClipRect() const
> + {
> + MOZ_ASSERT(IsValid());
> +
> + return mLayer->GetClipRect();
Just to make sure I understand this right - is the reason we apply the transform in GetVisibleRegion() (if we're not AtBottomLayer()), but not here, that the clip is applied after the transform, but the visible region is a pre-transform region?
@@ +270,5 @@
> +
> + // Expose an opaque pointer to the layer. Mostly used for printf
> + // purposes. This is not intended to be a general-purpose accessor
> + // for the underlying layer.
> + void* GetLayer() const
Might as well make it const void*.
Attachment #8479269 -
Flags: review?(botond) → review+
Comment 59•10 years ago
|
||
Comment on attachment 8479274 [details] [diff] [review]
Part 5 - Update tiling code to use LayerMetricsWrapper
Review of attachment 8479274 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/LayerMetricsWrapper.h
@@ +120,5 @@
> * to cast to uint32_t all over the place.
> */
> class MOZ_STACK_CLASS LayerMetricsWrapper {
> public:
> + enum StartAt {
Please use MOZ_BEGIN_NESTED_ENUM_CLASS, especially as we're overloading the constructor on StartAt vs. uint32_t.
@@ +181,5 @@
> + if (!AtTopLayer()) {
> + return LayerMetricsWrapper(mLayer, mIndex + 1);
> + }
> + if (mLayer->GetParent()) {
> + return LayerMetricsWrapper((Layer*)mLayer->GetParent(), BOTTOM);
You don't need to cast from derived to base explicitly.
@@ +350,5 @@
> + {
> + return !(*this == aOther);
> + }
> +
> + static bool HasScrollableMetrics(Layer* aLayer)
Any reason this isn't a method of Layer?
Attachment #8479274 -
Flags: review?(botond) → review+
Assignee | ||
Comment 60•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #57)
> I think it would enhance the readability of this code (particularly in
> conjunction with the comment below) if this were called isFixedOrSticky.
Done.
> Please move the last sentence inside the outer 'if', as that pertains to why
> we are skipping scrollable layers.
Done.
(In reply to Botond Ballo [:botond] from comment #58)
> I'd remove "and in case ... actually required". Let's not encourage people
> to cast void*'s to things :)
Done.
> Please add MOZ_EXPLICIT_CONVERSION in front of this. Classes with implicit
> conversions to bool are Evil(tm).
There's a few other places I use the implicit casting feature. I suppose I can make it explicit in those places but I might as well call IsValid() then for clarity.
> @@ +182,5 @@
> > + FrameMetrics Metrics() const
> > + {
> > + MOZ_ASSERT(IsValid());
> > +
> > + if (mIndex >= mLayer->GetFrameMetricsCount()) {
>
> Should this ever be called under these conditions? If not, perhaps we should
> assert instead. (As a bonus, the function can then return by
> reference-to-const).
Yes, it might be if GetFrameMetricsCount() == mIndex == 0.
> @@ +192,5 @@
> See comment about Metrics().
>
> If, however, 'nullptr' is in fact a valid return value, the function should
> be called GetApzc().
Yeah it is a valid return value, I'll rename the function.
> > + return mLayer->GetAsyncPanZoomController(mIndex);
>
> Should we assert GetFrameMetrics(mIndex).IsScrollable()?
It's valid to call this function with an mIndex that points to a non-scrollable metrics. It's just that should always return null.
> @@ +257,5 @@
> Just to make sure I understand this right - is the reason we apply the
> transform in GetVisibleRegion() (if we're not AtBottomLayer()), but not
> here, that the clip is applied after the transform, but the visible region
> is a pre-transform region?
Yup, exactly.
> @@ +270,5 @@
> > + void* GetLayer() const
>
> Might as well make it const void*.
Done.
(In reply to Botond Ballo [:botond] from comment #59)
> Please use MOZ_BEGIN_NESTED_ENUM_CLASS, especially as we're overloading the
> constructor on StartAt vs. uint32_t.
Done.
> @@ +181,5 @@
> > + return LayerMetricsWrapper((Layer*)mLayer->GetParent(), BOTTOM);
>
> You don't need to cast from derived to base explicitly.
Whoops, fixed. Left over from a previous version of the code.
> @@ +350,5 @@
> > + static bool HasScrollableMetrics(Layer* aLayer)
>
> Any reason this isn't a method of Layer?
Not really, I moved it.
Assignee | ||
Comment 61•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8479885 -
Attachment description: Address review comments so far → Part 12 - Address review comments so far
Updated•10 years ago
|
Attachment #8479267 -
Flags: review?(bgirard) → review+
Updated•10 years ago
|
Attachment #8479269 -
Flags: review?(bgirard) → review+
Comment 62•10 years ago
|
||
Comment on attachment 8479274 [details] [diff] [review]
Part 5 - Update tiling code to use LayerMetricsWrapper
Review of attachment 8479274 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/LayerMetricsWrapper.h
@@ +131,5 @@
> , mIndex(0)
> {
> }
>
> + explicit LayerMetricsWrapper(Layer* aRoot, StartAt aStart = StartAt::TOP)
Optional if it causes too much rot to fix something minor:
I don't really think the StartAt is something that belongs in the constructor. Omitting it doesn't leave the object in an inconsistent state. Looks like the other constructor also has that. Perhaps we should have a default position and have a separate helper call or let the user do it.
::: gfx/layers/Layers.cpp
@@ +57,1 @@
> LayerManager::GetPrimaryScrollableLayer()
Unrelated to this patch (so no need to fix it here) but I think this makes even less sense with all these changes. For instance you might have two sibling thebes layer with the same scrollid.
Attachment #8479274 -
Flags: review?(bgirard) → review+
Comment 63•10 years ago
|
||
Comment on attachment 8479281 [details] [diff] [review]
Part 7 - Various other bits of compositor code
Review of attachment 8479281 [details] [diff] [review]:
-----------------------------------------------------------------
r-: I'd like to check the correction.
::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +256,5 @@
> // with the background color by drawing a rectangle of the background color
> // over the entire composited area before drawing the container contents.
> + // Make sure not to do this on a "scrollinfo" layer (one with an empty visible
> + // region) because it's just a placeholder for APZ purposes.
> + if (LayerMetricsWrapper::HasScrollableMetrics(aContainer) && !aContainer->GetVisibleRegion().IsEmpty()) {
I don't think it's correct to have that 'a container layer is a scrollinfo layer iff it's has an empty region'. I think we should at the very least for now use IsScrollInfoLayer() so it's obvious what parts of the code have this assumption. I would rather that we have a better signal however for a scrollinfo layer.
I'm concerned because a culled container layer with possibly valid content can have an empty visible region.
::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +800,5 @@
> + FrameMetrics rootMetrics = LayerMetricsWrapper::TopmostScrollableMetrics(root);
> + if (!rootMetrics.IsScrollable()) {
> + // The root may not have any scrollable metrics, in which case rootMetrics
> + // will just be an empty FrameMetrics. Instead use the actual metrics from
> + // the root layer.
We jump from the topmost to the bottom? It leaves the reader wondering why we're skip the middle FM?
Attachment #8479281 -
Flags: review?(bgirard) → review-
Comment 64•10 years ago
|
||
Comment on attachment 8479283 [details] [diff] [review]
Part 8 - Update frame uniformity code to use shadow transform
Review of attachment 8479283 [details] [diff] [review]:
-----------------------------------------------------------------
This significantly changes the behavior which will impact how the user of the code see it. Instead of seeing the scrolling data it will see the transform data. There's trade offs here (like we talked on IRC). If mchang can't weight in on time I say this will do for now and we can fix it in bug 1058884.
Attachment #8479283 -
Flags: review?(mchang)
Attachment #8479283 -
Flags: review?(bgirard)
Attachment #8479283 -
Flags: review+
Comment 65•10 years ago
|
||
Comment on attachment 8479283 [details] [diff] [review]
Part 8 - Update frame uniformity code to use shadow transform
Review of attachment 8479283 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM
Updated•10 years ago
|
Attachment #8479287 -
Flags: review?(bgirard) → review+
Updated•10 years ago
|
Attachment #8479283 -
Flags: review?(mchang) → review+
Comment 66•10 years ago
|
||
Comment on attachment 8479267 [details] [diff] [review]
Part 3 - Modify Layers API
Review of attachment 8479267 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/Layers.h
@@ +1489,5 @@
> // and can be used anytime.
> + // A layer has an APZC at index aIndex only-if GetFrameMetrics(aIndex).IsScrollable().
> + // The aIndex for these functions must be less than GetFrameMetricsCount().
> + void SetAsyncPanZoomController(uint32_t aIndex, AsyncPanZoomController *controller);
> + AsyncPanZoomController* GetAsyncPanZoomController(uint32_t aIndex) const;
I would mention explicitly that it's valid to call this function with an aIndex such that !GetFrameMetrics(aIndex).IsScrollable(), and in such a case, nullptr is returned.
Comment 67•10 years ago
|
||
Comment on attachment 8479269 [details] [diff] [review]
Part 4 - Update APZCTreeManager code and add a LayerMetricsWrapper class
Review of attachment 8479269 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/LayerMetricsWrapper.h
@@ +183,5 @@
> + {
> + MOZ_ASSERT(IsValid());
> +
> + if (mIndex >= mLayer->GetFrameMetricsCount()) {
> + return FrameMetrics();
It might make sense to have a 'static const FrameMetrics LayerMetricsWrapper::sNullMetrics' (or FrameMetrics::sNullMetrics, or wherever), and return either that or GetFrameMetrics(mIndex) by reference-to-const. I don't feel strongly about this, though.
Assignee | ||
Comment 68•10 years ago
|
||
I can fold this into previous patches if needed but leaving it on top for easier review right now.
Attachment #8480003 -
Flags: review?(bgirard)
Assignee | ||
Comment 69•10 years ago
|
||
Attachment #8480004 -
Flags: review?(bgirard)
Assignee | ||
Updated•10 years ago
|
Attachment #8480004 -
Attachment is obsolete: true
Attachment #8480004 -
Flags: review?(bgirard)
Comment 70•10 years ago
|
||
Comment on attachment 8479276 [details] [diff] [review]
Part 6a - AsyncCompositionManager transform code updates
Review of attachment 8479276 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/LayerMetricsWrapper.h
@@ +362,5 @@
> }
>
> + static FrameMetrics TopmostScrollableMetrics(Layer* aLayer)
> + {
> + for (uint32_t i = aLayer->GetFrameMetricsCount(); i-- > 0;) {
I find the 'i-- > 0' hard to read. I would move the 'i--' into the update-expression, and use 'i - 1' in the loop body.
@@ +387,5 @@
> + return aLayer->GetFrameMetrics(0);
> + }
> + return FrameMetrics();
> + }
> +
Consider making these methods of Layer as well. Also, if you introduce the sNullMetrics, have these return by reference-to-const as well.
Attachment #8479276 -
Flags: review?(botond) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8480003 -
Attachment description: Part 12b - Address BenWa's comment about GetPrimaryScrollableLayer → Part 12b - Address BenWa's comment about GetPrimaryScrollableLayer (comment 62)
Assignee | ||
Comment 71•10 years ago
|
||
Attachment #8480005 -
Flags: review?(bgirard)
Assignee | ||
Comment 72•10 years ago
|
||
Assignee | ||
Comment 73•10 years ago
|
||
Comment 74•10 years ago
|
||
Comment on attachment 8479277 [details] [diff] [review]
Part 6b - AsyncCompositionManager scrollbar code updates
Review of attachment 8479277 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +630,5 @@
>
> appliedTransform = true;
> }
>
> + if (aLayer->GetScrollbarDirection() != Layer::NONE) {
Is the scrollbar not having to be a ContainerLayer related to multi-layer-apz?
@@ +666,5 @@
>
> static void
> +ApplyAsyncTransformToScrollbarForContent(Layer* aScrollbar,
> + const LayerMetricsWrapper& aContent,
> + bool aScrollbarIsChild)
Unrelated to this change, but sounds like this parameter should be named aScrollBarIsDescendant.
@@ +673,5 @@
> // children (i.e. when it has some possibly-visible content). This is to
> // avoid moving scroll-bars in the situation that only a scroll information
> // layer has been built for a scroll frame, as this would result in a
> // disparity between scrollbars and visible content.
> + if (!LayerHasNonContainerDescendants(aContent)) {
Given the Part 12c patch, do we want to introduce LayerMetricsWrapper::IsScrollInfoLayer() (which forwards to Layer::IsScrollInfoLayer()), and replace this check with that?
Attachment #8479277 -
Flags: review?(botond) → review+
Comment 75•10 years ago
|
||
Comment on attachment 8479267 [details] [diff] [review]
Part 3 - Modify Layers API
Review of attachment 8479267 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/Layers.h
@@ +1186,5 @@
> uint32_t GetContentFlags() { return mContentFlags; }
> const nsIntRegion& GetVisibleRegion() const { return mVisibleRegion; }
> + const FrameMetrics& GetFrameMetrics(uint32_t aIndex) const;
> + uint32_t GetFrameMetricsCount() const { return mFrameMetrics.Length(); }
> + nsTArray<FrameMetrics>& GetAllFrameMetrics() { return mFrameMetrics; }
Is there any reason this returns by reference-to-nonconst? The only usage I see in the patch set is the one in EndTransaction(), and returning by reference-to-const suffices for that.
Comment 76•10 years ago
|
||
Comment on attachment 8479279 [details] [diff] [review]
Part 6c - AsyncCompositionManager fixed/sticky layer code updates
Review of attachment 8479279 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +626,5 @@
> // parameter. This ensures that the overscroll transform is not unapplied,
> // and therefore that the visual effect applies to fixed and sticky layers.
> Matrix4x4 transformWithoutOverscroll = AdjustAndCombineWithCSSTransform(
> combinedAsyncTransformWithoutOverscroll, aLayer);
> + AlignFixedAndStickyLayers(aLayer, aLayer, bottom.GetScrollId(), oldTransform,
Let's add a comment saying why it's appropriate to use the bottom-most metrics' scroll id here.
For reference, some relevant IRC discussion:
[14:08] <kats> botond: we only care about the most recent scrolling ancestor to the sticky layer
[14:09] <kats> botond: which is the bottommost scrollable metrics on the scrollable layer
...
[14:10] <botond> kats: the scenario i'm wondering about is where layout gives us a layer with say 3 scrollable framemetrics, with ids (top-to-bottom) of 1, 2, and 3
[14:10] <botond> kats: and then some sticky layer with GetStickyScrollContainerId() of 2
[14:10] <botond> kats: are you saying that will never happen?
[14:11] <kats> botond: with the old code, the sticky layer would have to be a descendant of 2 but not of 3 for it to work
[14:11] <kats> and that can't happen if 1, 2, and 3 are on the same layer
[14:12] <kats> botond: so basically yeah, that should never happen. if it does then it was probably broken in the old code too
Attachment #8479279 -
Flags: review?(botond) → review+
Comment 77•10 years ago
|
||
Comment on attachment 8479280 [details] [diff] [review]
Part 6d - AsyncCompositionManager animation sampling code updates
Review of attachment 8479280 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +968,5 @@
> // find a frame that is async scrollable. Note that the fallback
> // code also includes Fennec which is rendered async. Fennec uses
> // its own platform-specific async rendering that is done partially
> // in Gecko and partially in Java.
> + wantNextFrame |= SampleAPZAnimations(LayerMetricsWrapper(root, LayerMetricsWrapper::TOP), aCurrentFrame);
TOP is the default, right?
Attachment #8479280 -
Flags: review?(botond) → review+
Updated•10 years ago
|
Attachment #8479281 -
Flags: review?(botond) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8480011 -
Flags: review?(bgirard)
Updated•10 years ago
|
Attachment #8480003 -
Flags: review?(bgirard) → review+
Updated•10 years ago
|
Attachment #8479286 -
Flags: review?(botond) → review+
Comment 78•10 years ago
|
||
Comment on attachment 8480011 [details] [diff] [review]
Part 12e - Fix a rather large omission to store multiple APZCs
Review of attachment 8480011 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/Layers.cpp
@@ +474,5 @@
> void
> Layer::SetAsyncPanZoomController(uint32_t aIndex, AsyncPanZoomController *controller)
> {
> MOZ_ASSERT(aIndex < GetFrameMetricsCount());
> + mApzcs[aIndex] = controller;
One nit that I have is that FM are manipulated in one part of the code and the APZC are set elsewhere. You have two 'associative' arrays here where their index must match. It feels too easy for the code that modifies the FM the second time around to modify the FM and break the associative match between the two arrays leaving the state inconsistent.
::: gfx/layers/Layers.h
@@ +865,5 @@
> {
> if (mFrameMetrics != aMetricsArray) {
> MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) FrameMetrics", this));
> mFrameMetrics = aMetricsArray;
> + FrameMetricsChanged();
For instance when you assigned a new FM array the scrollid may no longer associate with the APZC.
Attachment #8480011 -
Flags: review?(bgirard) → review-
Comment 79•10 years ago
|
||
Comment on attachment 8480005 [details] [diff] [review]
Part 12c - Fix check for scrollinfo layer (comment 63)
Review of attachment 8480005 [details] [diff] [review]:
-----------------------------------------------------------------
r+, but I would of though we had code conditional on ScrollInfo layer that could of been updated.
Attachment #8480005 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 80•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #66)
> I would mention explicitly that it's valid to call this function with an
> aIndex such that !GetFrameMetrics(aIndex).IsScrollable(), and in such a
> case, nullptr is returned.
I did this in part 12e.
(In reply to Botond Ballo [:botond] from comment #67)
> It might make sense to have a 'static const FrameMetrics
> LayerMetricsWrapper::sNullMetrics' (or FrameMetrics::sNullMetrics, or
> wherever), and return either that or GetFrameMetrics(mIndex) by
> reference-to-const. I don't feel strongly about this, though.
Yeah I was debating whether or not to do this at one point. It makes sense, I'll add a FrameMetrics::sNullMetrics and update the return values to be ref-to-const.
(In reply to Botond Ballo [:botond] from comment #70)
> I find the 'i-- > 0' hard to read. I would move the 'i--' into the
> update-expression, and use 'i - 1' in the loop body.
Ok, will do.
> @@ +387,5 @@
> Consider making these methods of Layer as well. Also, if you introduce the
> sNullMetrics, have these return by reference-to-const as well.
Yup, will do.
(In reply to Botond Ballo [:botond] from comment #74)
> > + if (aLayer->GetScrollbarDirection() != Layer::NONE) {
>
> Is the scrollbar not having to be a ContainerLayer related to
> multi-layer-apz?
Not really. I think it will still be a ContainerLayer, but since GetScrollbarDirection() exists on Layer, and we don't actually need it as a ContainerLayer* anywhere, it didn't seem worth it to do the whole AsContainerLayer() business.
> @@ +666,5 @@
> Unrelated to this change, but sounds like this parameter should be named
> aScrollBarIsDescendant.
True, I'll change it.
> @@ +673,5 @@
> > + if (!LayerHasNonContainerDescendants(aContent)) {
>
> Given the Part 12c patch, do we want to introduce
> LayerMetricsWrapper::IsScrollInfoLayer() (which forwards to
> Layer::IsScrollInfoLayer()), and replace this check with that?
I'm not sure they're equivalent. This function checks the entire descendant subtree whereas IsScrollInfoLayer just checks for direct children. I suppose I could probably write one in terms of the other but it seems like it wouldn't be that clear to use a function called IsScrollInfoLayer in this context.
(In reply to Botond Ballo [:botond] from comment #75)
> Is there any reason this returns by reference-to-nonconst? The only usage I
> see in the patch set is the one in EndTransaction(), and returning by
> reference-to-const suffices for that.
Not that I can recall, will fix.
(In reply to Botond Ballo [:botond] from comment #76)
> > + AlignFixedAndStickyLayers(aLayer, aLayer, bottom.GetScrollId(), oldTransform,
>
> Let's add a comment saying why it's appropriate to use the bottom-most
> metrics' scroll id here.
Will do.
(In reply to Botond Ballo [:botond] from comment #77)
> > + wantNextFrame |= SampleAPZAnimations(LayerMetricsWrapper(root, LayerMetricsWrapper::TOP), aCurrentFrame);
>
> TOP is the default, right?
Yeah. At one point I was considering not giving it a default value; this is probably left over from that. I'll remove it.
Assignee | ||
Comment 81•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #78)
> > MOZ_ASSERT(aIndex < GetFrameMetricsCount());
> > + mApzcs[aIndex] = controller;
>
> One nit that I have is that FM are manipulated in one part of the code and
> the APZC are set elsewhere. You have two 'associative' arrays here where
> their index must match. It feels too easy for the code that modifies the FM
> the second time around to modify the FM and break the associative match
> between the two arrays leaving the state inconsistent.
Their indices don't have to match at all times. Right after a transaction, the FrameMetrics will have been updated with the new version from the client side, but the APZCs will still be the old ones, and the indices might be wrong/out-of-date. Only after the APZCTreeManager::UpdatePanZoomControllerTree code runs will they get re-synced.
This used to be the case with the old code as well; the mFrameMetrics on the layer would get updated and the mAPZC would only get updated during the UpdatePanZoomControllerTree function which happened layer. The code in UpdatePZCTree is robust enough to handle this.
The only thing UpdatePZCTree assumes is that the APZC array in the layer is the same length as the FM array, because it tries to set APZCs at indices matching the scrollable FMs. The assumption also means it doesn't null out APZCs beyond GetFrameMetricsCount() so if I didn't resize the array in SetFrameMetrics then those nsRefPtrs<AsyncPanZoomController>s would never get cleared and they would leak.
> > if (mFrameMetrics != aMetricsArray) {
> > MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) FrameMetrics", this));
> > mFrameMetrics = aMetricsArray;
> > + FrameMetricsChanged();
>
> For instance when you assigned a new FM array the scrollid may no longer
> associate with the APZC.
Yup, as mentioned above this is fine. With the old code this happened as well, the mAPZC gets updated indpendently of the mFrameMetrics.
Comment 82•10 years ago
|
||
Comment on attachment 8480011 [details] [diff] [review]
Part 12e - Fix a rather large omission to store multiple APZCs
This isn't great but it's left inconsistent during the same event loop event only. Kats say he has a plan for cleaning this up later.
It's not easy to add a debug assert without messing with how the apzc are recycled.
Attachment #8480011 -
Flags: review- → review+
Comment 83•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #80)
> > @@ +673,5 @@
> > > + if (!LayerHasNonContainerDescendants(aContent)) {
> >
> > Given the Part 12c patch, do we want to introduce
> > LayerMetricsWrapper::IsScrollInfoLayer() (which forwards to
> > Layer::IsScrollInfoLayer()), and replace this check with that?
>
> I'm not sure they're equivalent. This function checks the entire descendant
> subtree whereas IsScrollInfoLayer just checks for direct children. I suppose
> I could probably write one in terms of the other but it seems like it
> wouldn't be that clear to use a function called IsScrollInfoLayer in this
> context.
The comment here suggests that the purpose of this check is to avoid transforming scrollbars for scroll-info layers. Is there a case where the layer is not a scroll-info layer, but we still don't want to draw the scrollbars?
Assignee | ||
Comment 84•10 years ago
|
||
Hm, good point; for some reason I didn't read that comment. I'll see if I can rewrite it to use IsScrollInfoLayer instead.
Comment 85•10 years ago
|
||
Comment on attachment 8480011 [details] [diff] [review]
Part 12e - Fix a rather large omission to store multiple APZCs
Review of attachment 8480011 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/Layers.cpp
@@ +481,5 @@
> AsyncPanZoomController*
> Layer::GetAsyncPanZoomController(uint32_t aIndex) const
> {
> MOZ_ASSERT(aIndex < GetFrameMetricsCount());
> #ifdef DEBUG
What's the purpose of the '#ifdef DEBUG' here? MOZ_ASSERT is already conditional on DEBUG.
Assignee | ||
Comment 86•10 years ago
|
||
orange try |
I addressed all the above review comments and squashed the fixes into their respective patches. The only one I left in a patch of its own was the GetPrimaryScrollableLayer change. And I rebased the whole set on master. Here's a try push:
https://tbpl.mozilla.org/?tree=Try&rev=acb240120485
Assignee | ||
Comment 87•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #84)
> Hm, good point; for some reason I didn't read that comment. I'll see if I
> can rewrite it to use IsScrollInfoLayer instead.
I put a IsScrollInfoLayer() on LayerMetricsWrapper and used that, although I couldn't make it just forward the call to mLayer->IsScrollInfoLayer. I implemented it directly like this: https://hg.mozilla.org/try/rev/757cdf36bc41#l1.12
The reason is that if you have a scroll info layer with FM[0], FM[1] where FM[1] is scrollable and FM[0] is not, then the LayerMetricsWrapper for FM[0] needs to return false while Layer::IsScrollInfoLayer needs to return true.
Still, adding IsScrollInfoLayer on LayerMetricsWrapper made the code a lot cleaner so it was still worth it.
Assignee | ||
Comment 88•10 years ago
|
||
Looks like Windows doesn't like the strongly-typed enum. It's using the wrong constructor I think.
Assignee | ||
Comment 89•10 years ago
|
||
green try |
https://tbpl.mozilla.org/?tree=Try&rev=331c534b0884 looks better
Assignee | ||
Comment 90•10 years ago
|
||
landing |
MOZ_BEGIN_NESTED_ENUM_CLASS, you have failed me for the last time!
I rolled it back to "enum" and landed on b2g-inbound:
https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=4a7c80b9ae9c
Comment 91•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #88)
> Looks like Windows doesn't like the strongly-typed enum. It's using the
> wrong constructor I think.
Ironic, that a change I asked for out of concern that we might call the wrong constructor, caused us to call the wrong constructor :/
I'm going to stop asking in reviews for class-scope enums to be defined using MOZ_BEGIN_NESTED_ENUM_CLASS until we have real 'enum class' support (which, given the activity around upgrading our GCC and MSVC toolchains, hopefully isn't very far off).
Comment 92•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/33bbbab21337
https://hg.mozilla.org/mozilla-central/rev/33fad6086b32
https://hg.mozilla.org/mozilla-central/rev/933f499c2586
https://hg.mozilla.org/mozilla-central/rev/2b593e85dbdb
https://hg.mozilla.org/mozilla-central/rev/dae1f96f88af
https://hg.mozilla.org/mozilla-central/rev/97d5ad0a7743
https://hg.mozilla.org/mozilla-central/rev/c017465be449
https://hg.mozilla.org/mozilla-central/rev/434f551aae4d
https://hg.mozilla.org/mozilla-central/rev/d6f6214621ef
https://hg.mozilla.org/mozilla-central/rev/ff3695004803
https://hg.mozilla.org/mozilla-central/rev/8e218c04840d
https://hg.mozilla.org/mozilla-central/rev/d331d1b70652
https://hg.mozilla.org/mozilla-central/rev/504e6547f98c
https://hg.mozilla.org/mozilla-central/rev/986b2273be29
https://hg.mozilla.org/mozilla-central/rev/4a1907885d78
https://hg.mozilla.org/mozilla-central/rev/4a7c80b9ae9c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•