Closed Bug 1259529 Opened 4 years ago Closed 4 years ago

Improve APZ minimap rendering behaviour for subframes

Categories

(Core :: Graphics: Layers, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Keywords: polish, Whiteboard: [gfx-noted])

Attachments

(2 files)

For subframes in general, but in particular big subframes, like on [1] the minimap on the subframe doesn't behave as well as it could. Really what we want is for the minimap to be always visible, and scaled to fit in the visible part of the composition bounds of the subframe. I haz patches.

[1] https://github.com/mozilla/gecko-dev/blob/master/layout/base/FrameLayerBuilder.cpp
Assignee: nobody → bugmail.mozilla
Attachment #8734461 - Flags: review?(bgirard) → review+
Comment on attachment 8734461 [details]
MozReview Request: Bug 1259529 - Clean up the APZ minimap rendering code a bit. No functional changes. r=BenWa

https://reviewboard.mozilla.org/r/42267/#review38795

::: gfx/layers/composite/ContainerLayerComposite.cpp
(Diff revision 1)
> -  Rect r;
> -  r = transform.TransformBounds(scrollRect.ToUnknownRect());
> -  compositor->FillRect(r, backgroundColor, clipRect, aContainer->GetEffectiveTransform());
> -
> -  /* Disabled because on long pages SlowDrawRect becomes a bottleneck.
> -  int tileW = gfxPrefs::LayersTileWidth();

I might of seen this code perhaps coming back later. I don't think it did a lot of harm staying like that and it can be useful later. Maybe I should of put it behind a preference actually. But I'm fine with it being removed since it's easy to re-write.
Comment on attachment 8734462 [details]
MozReview Request: Bug 1259529 - Ensure that the APZ minimap for subframes remains scaled to the visible portion of the composition bounds. r?BenWa

https://reviewboard.mozilla.org/r/42269/#review38801

This patch needs a better summary since it's very vague. Suggestion of the top of my head if I understood the aim correctly 'Keep the minimap in the composition bounds' or something similar.

::: gfx/layers/composite/ContainerLayerComposite.cpp:523
(Diff revision 1)
>    // Compute a scale with an appropriate aspect ratio
>    // We allocate up to 100px of width and the height of this layer.
>    float scaleFactor;
>    float scaleFactorX;
>    float scaleFactorY;
>    scaleFactorX = 100.f / scrollRect.width;

You're not adjusting the scaleFactorX. If your aim is to keep the minimap always render within the ClipRect & composition bound then you might still go outside in the X axis. This probably very rarely happen in practice however.
Attachment #8734462 - Flags: review?(bgirard)
(In reply to Benoit Girard (:BenWa) from comment #3)
> I might of seen this code perhaps coming back later. I don't think it did a
> lot of harm staying like that and it can be useful later. Maybe I should of
> put it behind a preference actually. But I'm fine with it being removed
> since it's easy to re-write.

I was thinking about keeping it but then I realized that with the virtual tiling behaviour on the layout side (128px on linux/windows, max-256 on OS X) we might want to update this code to reflect that. If we just reenabled it as-is it might be misleading. At least, if the intent is to reflect displayport increments as opposed to actual tiles.
That's a good point. I agree.
(In reply to Benoit Girard (:BenWa) from comment #4)
> This patch needs a better summary since it's very vague. Suggestion of the
> top of my head if I understood the aim correctly 'Keep the minimap in the
> composition bounds' or something similar.

Ah, sorry about that. I'll change it to "Ensure that the APZ minimap for subframes remains scaled to the visible portion of the composition bounds."

> ::: gfx/layers/composite/ContainerLayerComposite.cpp:523
> >    scaleFactorX = 100.f / scrollRect.width;
> 
> You're not adjusting the scaleFactorX. If your aim is to keep the minimap
> always render within the ClipRect & composition bound then you might still
> go outside in the X axis. This probably very rarely happen in practice
> however.

True. I wasn't too concerned about the x-axis but it's a simple change, so I'll update that too.
Comment on attachment 8734461 [details]
MozReview Request: Bug 1259529 - Clean up the APZ minimap rendering code a bit. No functional changes. r=BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42267/diff/1-2/
Attachment #8734461 - Attachment description: MozReview Request: Bug 1259529 - Clean up the APZ minimap rendering code a bit. No functional changes. r?BenWa → MozReview Request: Bug 1259529 - Clean up the APZ minimap rendering code a bit. No functional changes. r=BenWa
Attachment #8734462 - Attachment description: MozReview Request: Bug 1259529 - Make minimap for subframes behave better. r?BenWa → MozReview Request: Bug 1259529 - Ensure that the APZ minimap for subframes remains scaled to the visible portion of the composition bounds. r?BenWa
Attachment #8734462 - Flags: review?(bgirard)
Comment on attachment 8734462 [details]
MozReview Request: Bug 1259529 - Ensure that the APZ minimap for subframes remains scaled to the visible portion of the composition bounds. r?BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42269/diff/1-2/
Comment on attachment 8734462 [details]
MozReview Request: Bug 1259529 - Ensure that the APZ minimap for subframes remains scaled to the visible portion of the composition bounds. r?BenWa

https://reviewboard.mozilla.org/r/42269/#review38817
Attachment #8734462 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/486e7f9f2347
https://hg.mozilla.org/mozilla-central/rev/cc40dd2d5817
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.