Closed Bug 1391816 Opened 7 years ago Closed 7 years ago

Move as much layers free code out of WebRenderLayerManager as is reasonable.

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
defect

Tracking

()

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

People

(Reporter: jrmuizel, Assigned: ethlin)

References

Details

(Whiteboard: [wr-mvp])

Attachments

(2 files)

Bug 1391816 - Move layers-free related functions and variables from WebRenderLayerManager to WebRenderCommandsBuilder.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1391816 - Follow-up to rename WebRenderCommandsBuilder to WebRenderCommandBuilder.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
WebRenderLayerManager is becoming a mega class for everything having to do with layers-free. It's probably looking at breaking it out into more manageable pieces.
I think I can have some refactoring for it.
Assignee: nobody → ethlin
After some investigation, I will move CreateImageKey, PushImage, GenerateFallbackData, BuildWrMaskImage, PushItemAsImage, CreateWebRenderCommandsFromDisplayList, and CreateOrRecycleWebRenderUserData from WebRenderLayerManager to another file.
I leave CreateWebRenderCommandsFromDisplayList for now since this function use more WebRenderLayerManager members. For other functions, I move them to WebRenderUtils.
Attachment #8911693 - Flags: review?(jmuizelaar)
In this patch, I create a class "WebRenderCommandsBuilder" and move most of layers-free code to there. The changes look lots, but most of them are code moving and interface change. So I didn't split patches.
Comment on attachment 8911693 [details]
Bug 1391816 - Move layers-free related functions and variables from WebRenderLayerManager to WebRenderCommandsBuilder.

https://reviewboard.mozilla.org/r/183088/#review189922

::: gfx/layers/wr/WebRenderLayerManager.h:168
(Diff revision 3)
> -  }
> -
> -  bool ShouldNotifyInvalidation() const { return mShouldNotifyInvalidation; }
>    void SetNotifyInvalidation(bool aShouldNotifyInvalidation) { mShouldNotifyInvalidation = aShouldNotifyInvalidation; }
> +  bool ShouldNotifyInvalidation() const { return mShouldNotifyInvalidation; }
> +  WebRenderCommandsBuilder* GetCommandsBuilder() { return &mWebRenderCommandsBuilder; }

It would probably better if this returned a reference instead of a pointer. And just CommandsBuilder() instead of GetCommandsBuilder is nicer. Generally Get() is not required if it can't return null. I'd also just use CommandBuilder instead of CommandsBuilder as that sounds better.
Attachment #8911693 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 8911693 [details]
> Bug 1391816 - Move layers-free related functions and variables from
> WebRenderLayerManager to WebRenderCommandsBuilder.
> 
> https://reviewboard.mozilla.org/r/183088/#review189922
> 
> ::: gfx/layers/wr/WebRenderLayerManager.h:168
> (Diff revision 3)
> > -  }
> > -
> > -  bool ShouldNotifyInvalidation() const { return mShouldNotifyInvalidation; }
> >    void SetNotifyInvalidation(bool aShouldNotifyInvalidation) { mShouldNotifyInvalidation = aShouldNotifyInvalidation; }
> > +  bool ShouldNotifyInvalidation() const { return mShouldNotifyInvalidation; }
> > +  WebRenderCommandsBuilder* GetCommandsBuilder() { return &mWebRenderCommandsBuilder; }
> 
> It would probably better if this returned a reference instead of a pointer.
> And just CommandsBuilder() instead of GetCommandsBuilder is nicer. Generally
> Get() is not required if it can't return null. I'd also just use
> CommandBuilder instead of CommandsBuilder as that sounds better.

Okay, I will address this and rebase. I may need some time to rebase the patch.
Blocks: 1403915
(In reply to Ethan Lin[:ethlin] from comment #9)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Okay, I will address this and rebase. I may need some time to rebase the
> patch.

Can you try to get to it as soon as possible. Some things are beginning to pile up waiting for this to land. Otherwise, if it makes more sense, Kats can land the layer-full removal stuff and that might make it less work to do the rebase. Thoughts?
I started doing the rebase of this patch on top of my layers-full deletion patches, but haven't completed it yet. I'm not sure if I'll have time to finish it today, but so far what I've done is moved the code unmodified from WRLM to WebRenderCommandBuilder. (I also made the changes jeff suggested, calling the class WebRenderCommandBuilder and having the getter CommandBuilder() return a reference).

At this point the code doesn't compile because I need to update the code in WebRenderCommandBuilder to refer to |mManager| instead of |this| and apply any other similar changes that are needed. I noticed for example in your patch you had a `data->GetType() == WebRenderUserData::UserDataType::eCanvas` condition in the RemoveUnusedAndResetWebRenderUserData function after moving it, but that wasn't in the original code in WRLM. I'm not sure if that was intentional and something I need to preserve.

Anyway the work I have done so far is at https://github.com/staktrace/gecko-dev/commits/no-layers (top patch) and I'll continue on it tomorrow if you don't get to it first.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #10) 
> Can you try to get to it as soon as possible. Some things are beginning to
> pile up waiting for this to land. Otherwise, if it makes more sense, Kats
> can land the layer-full removal stuff and that might make it less work to do
> the rebase. Thoughts?

Okay, I will land it as soon as possible. There are many patches change those functions that we want to move. Some patches landed and some are still in the autoland repo. I'll do the rebase carefully.
No longer blocks: 1403915
Depends on: 1403915
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ca16bd7a8d3
Move layers-free related functions and variables from WebRenderLayerManager to WebRenderCommandsBuilder. r=jrmuizel
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> I started doing the rebase of this patch on top of my layers-full deletion
> patches, but haven't completed it yet. I'm not sure if I'll have time to
> finish it today, but so far what I've done is moved the code unmodified from
> WRLM to WebRenderCommandBuilder. (I also made the changes jeff suggested,
> calling the class WebRenderCommandBuilder and having the getter
> CommandBuilder() return a reference).
> 
> At this point the code doesn't compile because I need to update the code in
> WebRenderCommandBuilder to refer to |mManager| instead of |this| and apply
> any other similar changes that are needed. I noticed for example in your
> patch you had a `data->GetType() ==
> WebRenderUserData::UserDataType::eCanvas` condition in the
> RemoveUnusedAndResetWebRenderUserData function after moving it, but that
> wasn't in the original code in WRLM. I'm not sure if that was intentional
> and something I need to preserve.
> 
> Anyway the work I have done so far is at
> https://github.com/staktrace/gecko-dev/commits/no-layers (top patch) and
> I'll continue on it tomorrow if you don't get to it first.

Thanks. I just pushed the patch. Hopefully it won't be backed out. About the canvas condition, I add it intentionally since it should be a small flaw of the original code.
(In reply to Ethan Lin[:ethlin] from comment #16)
> Thanks. I just pushed the patch. Hopefully it won't be backed out. About the
> canvas condition, I add it intentionally since it should be a small flaw of
> the original code.

Ah, I see. It would have been better to move that into a separate patch to separate the functional changes from the code-moving but I guess it's done now.

I'll finish doing my rebase and compare my results with your results, just to double-check nothing got lost since it was a tricky rebase.
Ok, I finished my rebase. There are no functional differences between my result and what you landed already (except for the eCanvas thing I already mentioned above). However in my version the class is called WebRenderCommandBuilder (without the "s") so there's a bunch of naming diffs from (the file is also named differently) and there's some whitespace/include/namespace changes. There was also a random "GenerateFallbackData" text at the end of a line [1] that seems like a typo.

If we want to change the class to WebRenderCommandBuilder (without the "s") then I can make that change. Otherwise we can leave it as-is.

[1] https://hg.mozilla.org/integration/autoland/rev/5ca16bd7a8d3#l5.171
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> If we want to change the class to WebRenderCommandBuilder (without the "s")
> then I can make that change. Otherwise we can leave it as-is.

Let's make that change.
Comment on attachment 8915146 [details]
Bug 1391816 - Follow-up to rename WebRenderCommandsBuilder to WebRenderCommandBuilder.

https://reviewboard.mozilla.org/r/186396/#review191446


C/C++ static analysis didn't find any defects in this patch. Hooray!
Comment on attachment 8915146 [details]
Bug 1391816 - Follow-up to rename WebRenderCommandsBuilder to WebRenderCommandBuilder.

https://reviewboard.mozilla.org/r/186396/#review191456
Attachment #8915146 - Flags: review?(jmuizelaar) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbd85646f444
Follow-up to rename WebRenderCommandsBuilder to WebRenderCommandBuilder. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/5ca16bd7a8d3
https://hg.mozilla.org/mozilla-central/rev/dbd85646f444
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Priority: P3 → P1
Whiteboard: [wr-mvp]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: