Closed Bug 864447 Opened 8 years ago Closed 8 years ago

Support drawing display port on nested APZC frames

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: BenWa, Assigned: ajones)

References

Details

Attachments

(2 files, 8 obsolete files)

3.93 KB, patch
Details | Diff | Splinter Review
16.21 KB, patch
Details | Diff | Splinter Review
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.
Blocks: multi-apzc
Assignee: nobody → ajones
Attachment #765175 - Attachment is obsolete: true
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.
Attachment #765203 - Flags: feedback?(bgirard)
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.
Attachment #765203 - Flags: feedback?(bgirard) → feedback+
Attachment #765782 - Attachment is obsolete: true
Attachment #767572 - Attachment is obsolete: true
Attachment #765203 - Attachment is obsolete: true
Attachment #767584 - Attachment is obsolete: true
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.
Attachment #767602 - Flags: review?(bgirard) → review+
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.
Attachment #767603 - Flags: review?(bgirard) → review+
(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.
No longer blocks: 888084
Attachment #767602 - Attachment is obsolete: true
Attachment #767603 - Attachment is obsolete: true
Blocks: 889883
Fixed gtest failures; carrying r+
Attachment #768708 - Attachment is obsolete: true
Oops. Looks like I pushed out of date patches.
https://hg.mozilla.org/mozilla-central/rev/a55236cbafd6
https://hg.mozilla.org/mozilla-central/rev/fcf0c2f4fb0b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 894288
You need to log in before you can comment on or make changes to this bug.