Closed Bug 1331509 Opened 3 years ago Closed 3 years ago

Initialize TabChild's rendering state earlier

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(5 files)

Right now the TabChild::InitRenderingState function gets called pretty late (in the case where the tab creation is created from the parent side). It happens via a call to RecvShow, which is often after other functions have called GetLayerManager. I'd like to change that, because if we want to make the type of layer manager conditional on the rendering state (as I'm trying to do in bug 1326421), it's not really possible.
Latest version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73f8001703c838abddc6eea8b0a22f35d4c6d452

I think this should be good, will request review once the try is done. I also verified it provides what I need to finish off bug 1326421, at least the PuppetWidget::CreateLayerManager bits.
Comment on attachment 8827670 [details]
Bug 1331509 - Extract a helper function to initialize the RenderFrameParent from TabParent.

https://reviewboard.mozilla.org/r/105290/#review106130
Attachment #8827670 - Flags: review?(dvander) → review+
Comment on attachment 8827671 [details]
Bug 1331509 - Move InitRenderFrame call to TabParent construction.

https://reviewboard.mozilla.org/r/105292/#review106172
Attachment #8827671 - Flags: review?(dvander) → review+
Comment on attachment 8827672 [details]
Bug 1331509 - Rearrange TabChild::RecvShow to make it a bit more obvious how InitRenderingState can be extracted.

https://reviewboard.mozilla.org/r/105294/#review106176
Attachment #8827672 - Flags: review?(dvander) → review+
Comment on attachment 8827673 [details]
Bug 1331509 - Extract the InitRenderingState function in TabChild and invoke it earlier from TabParent.

https://reviewboard.mozilla.org/r/105296/#review106178
Attachment #8827673 - Flags: review?(dvander) → review+
Comment on attachment 8827674 [details]
Bug 1331509 - Always return the correct APZ state, since we should have it.

https://reviewboard.mozilla.org/r/105298/#review106184

Are we still guaranteed this in the DoFakeShow case? In that scenario it looks like we create the PRenderFrame from the child side, and since InitRenderingState is no longer called from RecvShow, it's not clear if it gets called at all. But the TabChild init code is so complicated it's hard to tell. r+ is conditional on making sure that works.

(Anyway, I like this direction a lot, it's much nicer to have layers decisions made up-front.)
Attachment #8827674 - Flags: review?(dvander) → review+
Duplicate of this bug: 1331847
(In reply to David Anderson [:dvander] from comment #12)
> Comment on attachment 8827674 [details]
> Bug 1331509 - Always return the correct APZ state, since we should have it.
> 
> https://reviewboard.mozilla.org/r/105298/#review106184
> 
> Are we still guaranteed this in the DoFakeShow case?

I think so, yes.

> In that scenario it
> looks like we create the PRenderFrame from the child side, and since
> InitRenderingState is no longer called from RecvShow, it's not clear if it
> gets called at all.

In part 4 I add a call to InitRenderingState directly in DoFakeShow, and it should have a valid layers id at that point, so InitRenderingState can get the compositor options from the compositor. The only loophole is if somebody calls AsyncPanZoomEnabled() between the creation of the TabChild and the DoFakeShow call, but since both of those happen inside ContentChild::ProvideWindowCommon, it should be safe. (That function is exercised during automated tests, and doesn't crash, so unless somebody *adds* a call to AsyncPanZoomEnabled() in that function we shouldn't be getting that scenario happening).

> But the TabChild init code is so complicated it's hard
> to tell.

Yeah, no kidding :)

> r+ is conditional on making sure that works.
> 
> (Anyway, I like this direction a lot, it's much nicer to have layers
> decisions made up-front.)

Thanks, that's good to know. I knew that the patches would either be "good" or be "horribly horribly wrong", am glad it's the former.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d8dfb30875e
Extract a helper function to initialize the RenderFrameParent from TabParent. r=dvander
https://hg.mozilla.org/integration/autoland/rev/570f8327ea9a
Move InitRenderFrame call to TabParent construction. r=dvander
https://hg.mozilla.org/integration/autoland/rev/bf830a2e2346
Rearrange TabChild::RecvShow to make it a bit more obvious how InitRenderingState can be extracted. r=dvander
https://hg.mozilla.org/integration/autoland/rev/edcd2bd8340d
Extract the InitRenderingState function in TabChild and invoke it earlier from TabParent. r=dvander
https://hg.mozilla.org/integration/autoland/rev/8d8e6116ed6d
Always return the correct APZ state, since we should have it. r=dvander
Whoops, you're right, I missed that in the diff.
Depends on: 1370089
You need to log in before you can comment on or make changes to this bug.