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.

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.

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.

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

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

try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a331a217bcf4ddb07331ec3abf688b1aaf75923&selectedJob=134841116

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.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd4fa203f7d0ba7053268320ce5d3bc28fd9d52b Builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8febc480b60a90cb8e954c2d6a5824156e3e43e

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

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