Closed Bug 1411238 Opened 7 years ago Closed 7 years ago

Remove layers-full dependencies in mochitest codepaths

Categories

(Core :: Graphics: WebRender, enhancement, P1)

Product:
Core
Component:
Graphics: WebRender
Version:
Other Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(2 files)

Bug 1411238 - Make OMTA queries work with layers-free webrender.
59 bytes, text/x-review-board-request
mtseng
: review+
Details
Bug 1411238 - Make APZ test logging work in layers-free WR mode.
59 bytes, text/x-review-board-request
botond
: review+
Details 
We have some bits of code that are exercised in mochitests that rely on having layers. With layers-free webrender these bits don't work and need fixing. The code paths in question are test-only so they don't affect production behaviour, but do prevent us from turning on mochitests.
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted][wr-mvp][triage] → [wr-mvp] [gfx-noted]
Comment on attachment 8921440 [details]
Bug 1411238 - Make OMTA queries work with layers-free webrender.

https://reviewboard.mozilla.org/r/192474/#review197910
Attachment #8921440 - Flags: review?(mtseng) → review+
For the record, the second patch will need rebasing on top of bug 1409446 and bug 1405359 but conceptually the change will stay the same (in that ScrollingLayersHelper will need to keep a handle to the WebRenderLayerManager). I'll do that rebase in a bit.
Comment on attachment 8921441 [details]
Bug 1411238 - Make APZ test logging work in layers-free WR mode.

https://reviewboard.mozilla.org/r/192476/#review198198

::: gfx/layers/wr/WebRenderScrollData.cpp:87
(Diff revision 2)
>    aItem->UpdateScrollData(&aOwner, this);
>    for (const ActiveScrolledRoot* asr = aItem->GetActiveScrolledRoot();
>         asr && asr != aStopAtAsr;
>         asr = asr->mParent) {
>      Maybe<ScrollMetadata> metadata = asr->mScrollableFrame->ComputeScrollMetadata(
> -        nullptr, aItem->ReferenceFrame(), ContainerLayerParameters(), nullptr);
> +        nullptr, aOwner.GetManager(), aItem->ReferenceFrame(),

Should we assert that |aOwner.GetManager()| is not null here (since WebRenderScrollData has a default constructor that leaves its mManager null)?

::: layout/generic/nsGfxScrollFrame.h:448
(Diff revision 2)
>      }
>    }
>    bool WantAsyncScroll() const;
>    Maybe<mozilla::layers::ScrollMetadata> ComputeScrollMetadata(
>      Layer* aLayer,
> +    mozilla::layers::LayerManager* aLayerManager,

If Layer (in the previous parameter type) doesn't need to be qualified, then neither should LayerManager. 

(Likewise in a couple of other places in this file.)

::: layout/generic/nsGfxScrollFrame.cpp:3844
(Diff revision 2)
>              aParameters.mXScale,
>              aParameters.mYScale,
>              mScrolledFrame->PresContext()->AppUnitsPerDevPixel()));
>  
>        ParentLayerIntRect layerClip;
>        if (const ParentLayerIntRect* origClip = aLayer->GetClipRect().ptrOr(nullptr)) {

The call site in ScrollingLayersHelper passes in |aLayer=nullptr|, but we use |aLayer| without null-checking it here. What gives?

::: layout/generic/nsIScrollableFrame.h:414
(Diff revision 2)
>     * scroll frame.
>     */
>    virtual bool WantAsyncScroll() const = 0;
>    /**
>     * aLayer's animated geometry root is this frame. If there needs to be a
>     * ScrollMetadata contributed by this frame, append it to aOutput.

Unrelated to this patch, but while you're touching this function declaration could you update the comment (which refers to a no-longer-existent |aOutput| parameter)?
Comment on attachment 8921441 [details]
Bug 1411238 - Make APZ test logging work in layers-free WR mode.

https://reviewboard.mozilla.org/r/192476/#review198198

> Should we assert that |aOwner.GetManager()| is not null here (since WebRenderScrollData has a default constructor that leaves its mManager null)?

Sure, I can do that.

> If Layer (in the previous parameter type) doesn't need to be qualified, then neither should LayerManager. 
> 
> (Likewise in a couple of other places in this file.)

Layer had a typedef, I guess I can add one for LayerManager too.

> The call site in ScrollingLayersHelper passes in |aLayer=nullptr|, but we use |aLayer| without null-checking it here. What gives?

This use only happens if parentLayerClip.isSome() which only happens if aClip is non-null. The call site in ScrollingLayersHelper (and all the other WR call sites) never pass a non-null aClip.

> Unrelated to this patch, but while you're touching this function declaration could you update the comment (which refers to a no-longer-existent |aOutput| parameter)?

Sure, will do
Comment on attachment 8921441 [details]
Bug 1411238 - Make APZ test logging work in layers-free WR mode.

https://reviewboard.mozilla.org/r/192476/#review198198

> This use only happens if parentLayerClip.isSome() which only happens if aClip is non-null. The call site in ScrollingLayersHelper (and all the other WR call sites) never pass a non-null aClip.

Thanks. I think it's worth making that explicit by adding a couple of comments:

  - At the declaration of nsIScrollableFrame::ComputeScrollMetadata(): "if aClip is non-null, aLayer must also be non-null"
  - At the call site in ScrollignLayersHelper: "it's OK to pass aLayer=null since we are also passing aClip=null"
Comment on attachment 8921441 [details]
Bug 1411238 - Make APZ test logging work in layers-free WR mode.

https://reviewboard.mozilla.org/r/192476/#review198208
Attachment #8921441 - Flags: review?(botond) → review+
Comment on attachment 8921441 [details]
Bug 1411238 - Make APZ test logging work in layers-free WR mode.

https://reviewboard.mozilla.org/r/192476/#review198198

> Thanks. I think it's worth making that explicit by adding a couple of comments:
> 
>   - At the declaration of nsIScrollableFrame::ComputeScrollMetadata(): "if aClip is non-null, aLayer must also be non-null"
>   - At the call site in ScrollignLayersHelper: "it's OK to pass aLayer=null since we are also passing aClip=null"

Done, thanks!
Oops, I screwed up the LayerManager typedef. Fixing...
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f10aa9d47c5e
Make OMTA queries work with layers-free webrender. r=mtseng
https://hg.mozilla.org/integration/autoland/rev/73cd4a9105a0
Make APZ test logging work in layers-free WR mode. r=botond
https://hg.mozilla.org/mozilla-central/rev/f10aa9d47c5e
https://hg.mozilla.org/mozilla-central/rev/73cd4a9105a0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: