Last Comment Bug 714839 - Make nsCSSFrameConstructor inherit nsFrameManager
: Make nsCSSFrameConstructor inherit nsFrameManager
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on:
Blocks: prescontext
  Show dependency treegraph
 
Reported: 2012-01-03 09:39 PST by Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
Modified: 2012-02-03 11:43 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (73.02 KB, patch)
2012-01-03 09:40 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
patch (74.56 KB, patch)
2012-01-04 10:12 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
bzbarsky: review+
Details | Diff | Review
patch with review comments addressed (78.65 KB, patch)
2012-02-01 05:16 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
bzbarsky: review+
Details | Diff | Review

Description Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-01-03 09:39:05 PST
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] (Away Jun. 27 - Jul. 13) 2012-01-03 09:40:32 PST
Created attachment 585440 [details] [diff] [review]
WIP
Comment 2 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-01-03 09:44:44 PST
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 [:bz] (Out June 25-July 6) 2012-01-03 23:49:09 PST
> 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] (Away Jun. 27 - Jul. 13) 2012-01-04 10:12:44 PST
Created attachment 585801 [details] [diff] [review]
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] (Away Jun. 27 - Jul. 13) 2012-01-04 10:13:24 PST
Oh, and this patch is looking good on Try:

https://tbpl.mozilla.org/?tree=Try&rev=113c20090ab7
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-31 21:31:33 PST
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] (Away Jun. 27 - Jul. 13) 2012-02-01 05:16:56 PST
Created attachment 593401 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2012-02-01 14:49:35 PST
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] (Away Jun. 27 - Jul. 13) 2012-02-02 02:29:58 PST
(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] 2012-02-03 11:43:27 PST
https://hg.mozilla.org/mozilla-central/rev/6837feb4fe4d

Note You need to log in before you can comment on or make changes to this bug.