1056155 - Don't assume FrameMetrics and APZC objects are only on ContainerLayers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

I missed a few spots in bug 1051985 when I moved the scrolling-related properties from ContainerLayer to Layer

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8475964 [details] [diff] [review]
Patch

Review of attachment 8475964 [details] [diff] [review]:
-----------------------------------------------------------------

Also, do you understand the code in RenderFrameParent? Specifically the code at [1] - it looks like it's doing something very similar to AsyncCompositionManager and I'm wondering if the order of transforms there needs to be flipped to go with bug 1052063 (cset 7dc893ff9125).

[1] http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp?rev=1ca58eced657#227

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Try push including this patch: https://tbpl.mozilla.org/?tree=Try&rev=ca10ab9d4739

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Bug 1056427 obsoletes the RenderFrameParent changes here but the rest is still valid I think.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8475964 [details] [diff] [review]
Patch

Review of attachment 8475964 [details] [diff] [review]:
-----------------------------------------------------------------

Feel free to leave out the RenderFrameParent bits as I'll be removing them.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Updated to remove the RFP bits; carrying r+

Comment 7

[Kartikaya Gupta (email:[e-mail])]

checkin-needed because it's a simple patch and can be squashed with other things to save resources. Try push in comment 3 is green (except for the intermittents and the stuff I aborted).

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/734451332c5b

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Hm.. it looks like the latest version of your patch on bug 1056427 still keeps the bit in the RenderFrameParent constructor that gets the frame metrics from lm->GetRoot()->AsContainerLayer(). Will that also get axed at some point, or is that going to stick around?

Comment 10

[Matt Woodrow (:mattwoodrow)]

Metro and test-ipcbrowser both still use the nsIContentViewManager. I'm sure it's all dead code, because that interface can't have been working previously, and it needs to be ripped out. I don't know much about the metro code though, someone else might want to do that.

Comment 11

[Botond Ballo [:botond]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> Metro and test-ipcbrowser both still use the nsIContentViewManager. I'm sure
> it's all dead code, because that interface can't have been working
> previously, and it needs to be ripped out. I don't know much about the metro
> code though, someone else might want to do that.

I filed bug 936690 a while ago about removing nsIContentView / nsIContentViewManager.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Jim, do you know about the code in metro that uses the nsIContentView[Manager] interfaces? It looks like that code is used for scrolling but is probably not used with APZC enabled, is that correct? Can we rip it out?

[1] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.xml?rev=cc298e4b0f47#25

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(This discussion should probably be happening on bug 1056427 and/or bug 936690, but whatever...)

Comment 14

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> Jim, do you know about the code in metro that uses the
> nsIContentView[Manager] interfaces? It looks like that code is used for
> scrolling but is probably not used with APZC enabled, is that correct? Can
> we rip it out?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> bindings/browser.xml?rev=cc298e4b0f47#25

I haven't interacted with this code at all so I'm not sure what it does. It looks like it's mostly untouched from the original landing.

The plan was to move over to using toolkits remote-browser, but that work was never completed.

Maybe we could just orphan this in the metro code, and file a bug on investigating what it's for knowing that the removal of the interfaces will break whatever it does.

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Jim Mathies [:jimm] from comment #14)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> > Jim, do you know about the code in metro that uses the
> > nsIContentView[Manager] interfaces? It looks like that code is used for
> > scrolling but is probably not used with APZC enabled, is that correct? Can
> > we rip it out?
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> > bindings/browser.xml?rev=cc298e4b0f47#25
> 
> I haven't interacted with this code at all so I'm not sure what it does. It
> looks like it's mostly untouched from the original landing.
> 
> The plan was to move over to using toolkits remote-browser, but that work
> was never completed.
> 
> Maybe we could just orphan this in the metro code, and file a bug on
> investigating what it's for knowing that the removal of the interfaces will
> break whatever it does.

The code is already broken, because those interfaces aren't hooked up to anything useful. We just want it ripped out so we can remove the interface definitions and finish cleaning things up.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

My vote is to set this._contentViewManager to null with a TODO and leave the rest of the code as is. We can deal with the fallout if any the next time we merge m-c to the metro branch. My assumption is that the code is not exercised in any meaningful way and so there should be no problem. I can do a metro build with that tomorrow to verify.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/734451332c5b

Comment 18

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> I can do
> a metro build with that tomorrow to verify.

I verified that setting _contentViewManager to null seems to have no effect on the latest code in the metro branch. The browser still starts and works as expected. Moving discussion over to bug 936690.