714839 - Make nsCSSFrameConstructor inherit nsFrameManager

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Watt [:jwatt]]

One of the steps given in bug 154199 comment 16 is to make nsCSSFrameConstructor inherit nsFrameManager. I'm breaking that step out into this separate bug.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: WIP

Comment 2

[Jonathan Watt [:jwatt]]

There are a few outstanding issues to think about/fix.

* Both classes have AppendFrames and NotifyDestroyingFrame methods
* Some code in nsCSSFrameConstructor checks aState.mFrameManager is non-null, which makes me wonder if nsFrameManagers can die/change, or if those checks can just be removed
* In PresShell::Init, it seems like the construction and initialization of nsCSSFrameConstructor and nsFrameConstructor should be unified, or moved closer together
* nsIPresShell::GetRootFrame() is no longer inline in the WIP, which may be bad given the effort in https://bugzilla.mozilla.org/attachment.cgi?id=435528&action=diff ?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Some code in nsCSSFrameConstructor checks aState.mFrameManager is non-null

That code is just old and tired.  aState.mFrameManager can't be null.  Frame managers can in fact die, but when they do the relevant frame constructor better die too.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

There are a few things to consider during review:

* Both classes have a non-virtual AppendFrames, requiring explicit calls.
* In PresShell::Init, I'd still like to move the nsCSSFrameConstructor ctor and the nsFrameManager's Init() call closer together, but it's not clear if that's safe given the dependencies. Seems okay to leave as-is for the moment though.
* nsIPresShell::GetRootFrame() is no longer inline in the WIP, which may be bad given the effort in https://bugzilla.mozilla.org/attachment.cgi?id=435528&action=diff ?

Comment 5

[Jonathan Watt [:jwatt]]

Oh, and this patch is looking good on Try:

https://tbpl.mozilla.org/?tree=Try&rev=113c20090ab7

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 585801 [details] [diff] [review]
patch

I'm sorry for the lag here...

As far as the items from comment 4 go:

1)  The AppendFrames methods have different signatures.  So why do we need explicit calls?  Is it because the nsCSSFC method hides the nsFrameManager one?  If so, I think renaming the CSSFC method to AppendFramesToParent or something would be fine by me and preferable to the changes in this patch.

2)  Yes, probably OK as-is for now.

3)  The new GetRootFrame code doesn't make sense (e.g. it can no longer be called from outside libxul).  It's probably not a huge deal to have GetRootFrame out of line, but this part needs to be fixed; we probably just need to leave GetRootFrame inlined but calling through to out-of-line internal/external variants (with the latter virtual).

r- pending updates for 1 and 3 above.  Next review will be _way_ faster.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch with review comments addressed

(In reply to Boris Zbarsky (:bz) from comment #6)
> I'm sorry for the lag here...

No worries, I know you're busy.

> 1)  The AppendFrames methods have different signatures.  So why do we need
> explicit calls?  Is it because the nsCSSFC method hides the nsFrameManager
> one?

Without them I'd get "no matching function for call" compiler errors on Mac (10.7, with Xcode 4.2).

I renamed the CSSFC method to AppendFramesToParent.

> 3)  The new GetRootFrame code doesn't make sense (e.g. it can no longer be
> called from outside libxul).  It's probably not a huge deal to have
> GetRootFrame out of line, but this part needs to be fixed; we probably just
> need to leave GetRootFrame inlined but calling through to out-of-line
> internal/external variants (with the latter virtual).

Err, right. I've made internal/external variants as you suggested.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 593401 [details] [diff] [review]
patch with review comments addressed

The second hunk in InsertFirstLineFrames should have a space before kPrincipalList.

You changed the two ReparentFrame callers in InsertFirstLineFrames, but not the callee function.  Luckily, those two callers are #if 0.  But fix those callsites to pass |this| anyway?

Reading through this more carefully, we're also taking FrameManager() out of line...  I would almost rather we left an nsFrameManagerBase* member on presshell, and then we can leave FrameManager() and GetRootFrame() inlined, at the cost of the extra word of memory...

Is the crashreporter change really supposed to be in this diff?  I doubt it...

r=me with the above nits fixed.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky (:bz) from comment #8)
> I would almost rather we left an nsFrameManagerBase* member on
> presshell, and then we can leave FrameManager() and GetRootFrame() inlined,
> at the cost of the extra word of memory...

Good idea. I've addressed that and the other comments, and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6837feb4fe4d

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6837feb4fe4d