This bug is about being able to draw display ports on nested frames. This might be non-trivial, particular in the case of overflow:scroll elements because they don't have their own window associated with them and some of the paint code might make assumptions about that.

Looks like this might not be this bad: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2049

Comment on attachment 765203 [details] [diff] [review] Set display port for given scroll id This patch will set the display port for the element matching the scroll id given in the frame metrics.

Comment on attachment 765203 [details] [diff] [review] Set display port for given scroll id Review of attachment 765203 [details] [diff] [review]: ----------------------------------------------------------------- Excellent. I wonder if we want to special case the root document lookup since it will be the thing you scroll 80+% of the time.

so we can move the paint timing into the throttler.

so we can move the paint timing into the throttler.

so we can move the paint timing into the throttler.

Comment on attachment 767602 [details] [diff] [review] Set display port for given scroll id; Review of attachment 767602 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +1556,5 @@ > + } > + > + nsCOMPtr<nsIDOMElement> element; > + if (aFrameMetrics.mScrollId == FrameMetrics::ROOT_SCROLL_ID) { > + domDoc->GetDocumentElement(getter_AddRefs(element)); Can you add a comment saying this is just for a faster lookup. That way it doesn't give the impression to the reader that the root layer really has special logic.

Comment on attachment 767603 [details] [diff] [review] Move timing tracking out of APZC; Review of attachment 767603 [details] [diff] [review]: ----------------------------------------------------------------- Cool, makes the APZC code cleaner. r+ once we resolved the following. ::: gfx/layers/ipc/TaskThrottler.cpp @@ +13,5 @@ > > TaskThrottler::TaskThrottler() > : mOutstanding(false) > , mQueuedTask(nullptr) > + , mMaxDurations(1) 1 seems like an odd default. Either you want to record these in which case it should be a param or it should default to 0 and we should honor 0 properly, see below. @@ +47,5 @@ > + // desired number of samples. > + if (mDurations.Length() >= mMaxDurations) { > + mDurations.RemoveElementAt(0); > + } > + mDurations.AppendElement(aTimeStamp - mStartTime); I wonder if we want to optimize this since it will happen frequently. This is O(n) and really shouldn't be. Also only do this if mMaxDurations > 0 ::: gfx/layers/ipc/TaskThrottler.h @@ +71,5 @@ > + * Clear average history. > + * > + * @param aMaxDurations The maximum number of durations to measure. > + */ > + void ClearHistory(uint32_t aMaxDurations) I don't really like this API. A clear shouldn't really change max duration. Either this wants to be two functions or have a better name. Best I can think is 'ResizeAndClearHistory' but it's not great.

(In reply to Benoit Girard (:BenWa) from comment #11) > ::: dom/ipc/TabChild.cpp > @@ +1556,5 @@ > > + nsCOMPtr<nsIDOMElement> element; > > + if (aFrameMetrics.mScrollId == FrameMetrics::ROOT_SCROLL_ID) { > > + domDoc->GetDocumentElement(getter_AddRefs(element)); > > Can you add a comment saying this is just for a faster lookup. That way it > doesn't give the impression to the reader that the root layer really has > special logic. No it really does have special logic. I'll add a comment explaining that. (In reply to Benoit Girard (:BenWa) from comment #12) > @@ +47,5 @@ > > + // desired number of samples. > > + if (mDurations.Length() >= mMaxDurations) { > > + mDurations.RemoveElementAt(0); > > + } > > + mDurations.AppendElement(aTimeStamp - mStartTime); > > I wonder if we want to optimize this since it will happen frequently. This > is O(n) and really shouldn't be. It could be optimised but I'm not sure that it's worthwhile when n is always 3. I'm going to do it anyway in bug 888084. > ::: gfx/layers/ipc/TaskThrottler.h > @@ +71,5 @@ > > + * Clear average history. > > + * > > + * @param aMaxDurations The maximum number of durations to measure. > > + */ > > + void ClearHistory(uint32_t aMaxDurations) > > I don't really like this API. A clear shouldn't really change max duration. > Either this wants to be two functions or have a better name. Best I can > think is 'ResizeAndClearHistory' but it's not great. I was on the fence on this. Two functions is a cleaner API.

Fixed gtest failures; carrying r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed80fcff168 https://hg.mozilla.org/integration/mozilla-inbound/rev/e6430ac52054

Backed out for: https://tbpl.mozilla.org/php/getParsedLog.php?id=25053467&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=25054116&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbd97f900b0 https://hg.mozilla.org/integration/mozilla-inbound/rev/729ca37b3a36

Oops. Looks like I pushed out of date patches.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a55236cbafd6 https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf0c2f4fb0b

https://hg.mozilla.org/mozilla-central/rev/a55236cbafd6 https://hg.mozilla.org/mozilla-central/rev/fcf0c2f4fb0b