Make nsCSSFrameConstructor inherit nsFrameManager

RESOLVED FIXED in mozilla13

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks: 1 bug)

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 585440 [details] [diff] [review]
WIP
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
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 ?
> 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.
(Assignee)

Comment 4

6 years ago
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 ?
Attachment #585440 - Attachment is obsolete: true
Attachment #585801 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

6 years ago
Oh, and this patch is looking good on Try:

https://tbpl.mozilla.org/?tree=Try&rev=113c20090ab7
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.
Attachment #585801 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

5 years ago
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.
Attachment #585801 - Attachment is obsolete: true
Attachment #593401 - Flags: review?(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.
Attachment #593401 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

5 years ago
Blocks: 154199
(Assignee)

Comment 9

5 years ago
(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
No longer blocks: 154199
Target Milestone: --- → mozilla13
(Assignee)

Updated

5 years ago
Blocks: 154199
https://hg.mozilla.org/mozilla-central/rev/6837feb4fe4d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.