864447 - Support drawing display port on nested APZC frames

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Benoit Girard (:BenWa)]

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

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Set display port for given scroll id (WIP)

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Set display port for given scroll id

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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 5

[Benoit Girard (:BenWa)]

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.

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Remove timing tracking from APZC (WIP)We're going to need one paint throttler for multiple APZCs

so we can move the paint timing into the throttler.

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Remove timing tracking from APZC (WIP)We're going to need one paint throttler for multiple APZCs

so we can move the paint timing into the throttler.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Remove timing tracking from APZC (WIP)We're going to need one paint throttler for multiple APZCs

so we can move the paint timing into the throttler.

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Set display port for given scroll id;

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Move timing tracking out of APZC;

Comment 11

[Benoit Girard (:BenWa)]

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 12

[Benoit Girard (:BenWa)]

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.

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(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.

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Set display port for given scroll id;

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Move timing tracking out of APZC;

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=a196c1441be4

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Move timing tracking out of APZC;

Fixed gtest failures; carrying r+

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=ee3b06cf283d

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 20

[Ed Morley [:emorley]]

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

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Oops. Looks like I pushed out of date patches.

Comment 22

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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