Closed
Bug 473390
Opened 12 years ago
Closed 12 years ago
[FIX]Switch XUL/HTML/MathML/SVG frame construction to be more table-driven and eliminate IsSpecialContent
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(19 files, 8 obsolete files)
I've been thinking about this for a while. This is not quite the same as bug 323233, but that bug could build on this patch. That is, the way we find the frame construction data could use a hashtable if desired. I've tried to avoid making behavior changes as much as I could, or at least landing them before now, but there are a few left in the patch series coming up anyway. I think they're minor enough to not be an issue. Performance seems to be a wash, as far as I can tell. There's a bit of a codesize win, and I think this code is a good bit more readable...
![]() |
Assignee | |
Comment 1•12 years ago
|
||
Attachment #356746 -
Flags: superreview?(roc)
Attachment #356746 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 2•12 years ago
|
||
![]() |
Assignee | |
Comment 3•12 years ago
|
||
Attachment #356750 -
Flags: superreview?(roc)
Attachment #356750 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 4•12 years ago
|
||
Attachment #356752 -
Flags: superreview?(roc)
Attachment #356752 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 5•12 years ago
|
||
Attachment #356753 -
Flags: superreview?(roc)
Attachment #356753 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 6•12 years ago
|
||
Attachment #356754 -
Flags: superreview?(roc)
Attachment #356754 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 7•12 years ago
|
||
Attachment #356756 -
Flags: superreview?(roc)
Attachment #356756 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 8•12 years ago
|
||
Attachment #356757 -
Flags: superreview?(roc)
Attachment #356757 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 9•12 years ago
|
||
Attachment #356758 -
Flags: superreview?(roc)
Attachment #356758 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 10•12 years ago
|
||
Attachment #356759 -
Flags: superreview?(roc)
Attachment #356759 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 11•12 years ago
|
||
Attachment #356761 -
Flags: superreview?(roc)
Attachment #356761 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 12•12 years ago
|
||
Attachment #356762 -
Flags: superreview?(roc)
Attachment #356762 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 13•12 years ago
|
||
Attachment #356763 -
Flags: superreview?(roc)
Attachment #356763 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 14•12 years ago
|
||
![]() |
Assignee | |
Comment 15•12 years ago
|
||
Attachment #356766 -
Flags: superreview?(roc)
Attachment #356766 -
Flags: review?(roc)
![]() |
Assignee | |
Updated•12 years ago
|
Attachment #356765 -
Attachment is patch: true
Attachment #356765 -
Attachment mime type: application/octet-stream → text/plain
Attachment #356765 -
Flags: superreview?(roc)
Attachment #356765 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 16•12 years ago
|
||
Attachment #356768 -
Flags: superreview?(roc)
Attachment #356768 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 17•12 years ago
|
||
Attachment #356770 -
Flags: superreview?(roc)
Attachment #356770 -
Flags: review?(roc)
Attachment #356770 -
Flags: review?(jwatt)
![]() |
Assignee | |
Comment 18•12 years ago
|
||
Attachment #356771 -
Flags: superreview?(roc)
Attachment #356771 -
Flags: review?(roc)
Attachment #356771 -
Flags: review?(jwatt)
![]() |
Assignee | |
Comment 19•12 years ago
|
||
Attachment #356773 -
Flags: superreview?(roc)
Attachment #356773 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 20•12 years ago
|
||
Review notes: The only parts that actually introduce behavior changes are part 1 (for a root XUL content with display:table and a tag that doesn't get frame construction by tag we'll now construct a box, not a table), part 2 (now make some SetInitialChildList calls we didn't use to make), part 4 (we will actually have working form controls even if the XBL prefs are set), and part 16 (removes some checks that we used to assert, but leaves the asserts). The only one of those that could be worrisome is part 16, but none of those asserts should be firing anyway given our current code, so I think it's OK.
Comment on attachment 356746 [details] [diff] [review] Part 1. Drop one IsSpecialContent caller. Makes us construct a box, not a table for a XUL root with display:table You attached the wrong patch --- the diff -w version below looks good though :-)
Attachment #356746 -
Flags: superreview?(roc)
Attachment #356746 -
Flags: superreview-
Attachment #356746 -
Flags: review?(roc)
Attachment #356746 -
Flags: review-
Attachment #356749 -
Flags: superreview+
Attachment #356749 -
Flags: review+
Attachment #356750 -
Flags: superreview?(roc)
Attachment #356750 -
Flags: superreview-
Attachment #356750 -
Flags: review?(roc)
Attachment #356750 -
Flags: review-
Comment on attachment 356750 [details] [diff] [review] Part 2. Align the way we call SetInitialChildList on HTML frames with the rest of the world Looks like the wrong patch
Attachment #356752 -
Flags: superreview?(roc)
Attachment #356752 -
Flags: superreview+
Attachment #356752 -
Flags: review?(roc)
Attachment #356752 -
Flags: review+
Attachment #356753 -
Flags: superreview?(roc)
Attachment #356753 -
Flags: superreview+
Attachment #356753 -
Flags: review?(roc)
Attachment #356753 -
Flags: review+
Attachment #356754 -
Flags: superreview?(roc)
Attachment #356754 -
Flags: superreview+
Attachment #356754 -
Flags: review?(roc)
Attachment #356754 -
Flags: review+
Attachment #356756 -
Flags: superreview?(roc)
Attachment #356756 -
Flags: superreview+
Attachment #356756 -
Flags: review?(roc)
Attachment #356756 -
Flags: review+
![]() |
Assignee | |
Comment 23•12 years ago
|
||
![]() |
Assignee | |
Comment 24•12 years ago
|
||
Attachment #356891 -
Flags: superreview?(roc)
Attachment #356891 -
Flags: review?(roc)
Comment on attachment 356757 [details] [diff] [review] Part 7. New setup for HTML; read the .h diff first + @param nsStyleContext the style context to be used for the frame.w Typo at end of line + static const FrameConstructionDataByInt sHTMLInputData[]; + // <html:object>, <html:embed>, <html:applet> construction + static const FrameConstructionDataByInt sHTMLObjectData[]; + // HTML by-tag frame construction + static const FrameConstructionDataByTag sHTMLData[]; Document sentinel values used? + /* If the FCDATA_DATA_IS_BOOL_GETTER bit is set, then the mData of the + FrameConstructionData is a function that will return PR_TRUE if the + mFunc.mCreationFunc should be used and PR_FALSE otherwise. */ +#define FCDATA_DATA_IS_BOOL_GETTER 0x2 So if it's not used, we just don't construct a frame for it? Why not just allow the creator to return a null frame with a success code to signal that case? + /* If the FCDATA_FUNC_IS_INT_GETTER bit is set, then the mFunc of the + FrameConstructionData is a getter function that can be used to get an int + to then use with FindDataByInt. In this case the mData should be the + array to use for the int search and its length. */ +#define FCDATA_FUNC_IS_INT_GETTER 0x4 It feels like maybe you're trying a bit too hard here? Why not just make these cases use a FrameFullConstructor which explicitly calls FindDataByTag with the correct index? Then we'd only need two kinds of construction: FrameFullConstructors and FrameCreationFuncs. We wouldn't need mData or bool-getters or int-getters. This seems worth it to me, since SIMPLE_TAG_CREATE_BY_INT already requires you to create a helper function for the int-getter, so adding a call to FindDataByInt to that helper doesn't seem worth avoiding ... but what do you think?
(In reply to comment #25) > Document sentinel values used? Oh, I just realized there are none, so never mind.
Attachment #356891 -
Flags: superreview?(roc)
Attachment #356891 -
Flags: superreview+
Attachment #356891 -
Flags: review?(roc)
Attachment #356891 -
Flags: review+
Comment 27•12 years ago
|
||
Comment on attachment 356770 [details] [diff] [review] Part 16. Move around some SVG assertions and eliminate the corresponding checks You might consider moving SVG_GetFirstNonAAncestorFrame to be nsSVGUtils::GetFirstNonAAncestorFrame so we get one prototype now that you are using it outside nsCSSFrameConstructor.
![]() |
Assignee | |
Comment 28•12 years ago
|
||
> Typo at end of line Fixed (and the "otherdata" typo before that too, in case you run into it). > So if it's not used, we just don't construct a frame for it? No, it means we go on and look for another way to construct a frame (typically by display). An example is <img>: if we don't construct an nsImageFrame, we need to construct a frame by display for the alt text to live in. The documentation for FrameCreationTester is a bit clearer about this, possibly. I'll think about how to make the docs more readable. > Why not just make these cases use a FrameFullConstructor which explicitly > calls FindDataByTag with the correct index? The problem is that I want to know up front, before I try to create it, what sort of frame I'm going to be creating, and most importantly whether I'm going to be creating the frame by display type or not. For <object>, for example, we switch on the nsIObjectLoadingContent::TYPE_* value. For most of them, we just create a particular kind of frame. But for TYPE_NULL (and actually also for some object intrinsic states) I want to create a frame by display type. Same thing for <input> (hidden inputs get created by display type). This business of some tag names falling through to creation by display is basically why I ended up with the bool- and int-getters....
![]() |
Assignee | |
Comment 29•12 years ago
|
||
> nsSVGUtils::GetFirstNonAAncestorFrame
Ah, good idea. I've made that change. Needed to merge parts 17 and 18 to it. Let me know which of those 3 diffs you want to see regenerated?
![]() |
Assignee | |
Comment 30•12 years ago
|
||
OK, roc and I talked about the int/bool getter thing, and decided to switch to a setup in which we have either a creator, a full constructor, or a function that will return frame construction data, as a generalization of the bool getter and int getter. This change only affects parts 7, 9, 15, 17, 18. Updated diffs for those coming up. Codesize actually ended up the same with this approach, while the data was smaller since the FrameConstructionData structs shrank.
![]() |
Assignee | |
Comment 31•12 years ago
|
||
Attachment #356757 -
Attachment is obsolete: true
Attachment #357163 -
Flags: superreview?(roc)
Attachment #357163 -
Flags: review?(roc)
Attachment #356757 -
Flags: superreview?(roc)
Attachment #356757 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 32•12 years ago
|
||
Attachment #356746 -
Attachment is obsolete: true
Attachment #356750 -
Attachment is obsolete: true
Attachment #357165 -
Flags: superreview?(roc)
Attachment #357165 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 33•12 years ago
|
||
Attachment #356759 -
Attachment is obsolete: true
Attachment #356768 -
Attachment is obsolete: true
Attachment #356759 -
Flags: superreview?(roc)
Attachment #356759 -
Flags: review?(roc)
Attachment #356768 -
Flags: superreview?(roc)
Attachment #356768 -
Flags: review?(roc)
![]() |
Assignee | |
Updated•12 years ago
|
Attachment #357166 -
Attachment is patch: true
Attachment #357166 -
Attachment mime type: application/octet-stream → text/plain
Attachment #357166 -
Flags: superreview?(roc)
Attachment #357166 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 34•12 years ago
|
||
Attachment #356771 -
Attachment is obsolete: true
Attachment #357169 -
Flags: superreview?(roc)
Attachment #357169 -
Flags: review?(roc)
Attachment #357169 -
Flags: review?(jwatt)
Attachment #356771 -
Flags: superreview?(roc)
Attachment #356771 -
Flags: review?(roc)
Attachment #356771 -
Flags: review?(jwatt)
![]() |
Assignee | |
Comment 35•12 years ago
|
||
Attachment #357170 -
Flags: superreview?(roc)
Attachment #357170 -
Flags: review?(roc)
![]() |
Assignee | |
Updated•12 years ago
|
Attachment #356773 -
Attachment is obsolete: true
Attachment #356773 -
Flags: superreview?(roc)
Attachment #356773 -
Flags: review?(roc)
![]() |
Assignee | |
Comment 36•12 years ago
|
||
I ran into an assertion due to nsSVGAFrame inheriting from nsSVGTSpanFrame. I have no idea why I didn't hit this when I ran tests before... In any case, the only change from the previous part 16 is in nsSVGTSpanFrame::Init.
Attachment #356770 -
Attachment is obsolete: true
Attachment #357182 -
Flags: superreview?(roc)
Attachment #357182 -
Flags: review?(roc)
Attachment #357182 -
Flags: review?(jwatt)
Attachment #356770 -
Flags: superreview?(roc)
Attachment #356770 -
Flags: review?(roc)
Attachment #356770 -
Flags: review?(jwatt)
Attachment #357163 -
Flags: superreview?(roc)
Attachment #357163 -
Flags: superreview+
Attachment #357163 -
Flags: review?(roc)
Attachment #357163 -
Flags: review+
Attachment #356758 -
Flags: superreview?(roc)
Attachment #356758 -
Flags: superreview+
Attachment #356758 -
Flags: review?(roc)
Attachment #356758 -
Flags: review+
Attachment #357165 -
Flags: superreview?(roc)
Attachment #357165 -
Flags: superreview+
Attachment #357165 -
Flags: review?(roc)
Attachment #357165 -
Flags: review+
Attachment #356761 -
Flags: superreview?(roc)
Attachment #356761 -
Flags: superreview+
Attachment #356761 -
Flags: review?(roc)
Attachment #356761 -
Flags: review+
Attachment #356762 -
Flags: superreview?(roc)
Attachment #356762 -
Flags: superreview+
Attachment #356762 -
Flags: review?(roc)
Attachment #356762 -
Flags: review+
Attachment #356763 -
Flags: superreview?(roc)
Attachment #356763 -
Flags: superreview+
Attachment #356763 -
Flags: review?(roc)
Attachment #356763 -
Flags: review+
Attachment #356765 -
Flags: superreview?(roc)
Attachment #356765 -
Flags: superreview+
Attachment #356765 -
Flags: review?(roc)
Attachment #356765 -
Flags: review+
Attachment #356766 -
Flags: superreview?(roc)
Attachment #356766 -
Flags: superreview+
Attachment #356766 -
Flags: review?(roc)
Attachment #356766 -
Flags: review+
Comment on attachment 357166 [details] [diff] [review] Part 15. New setup for XUL; updated to new approach + if (rootBox) + rootBox->AddTooltipSupport(aContent); {} around the statement, I guess
Attachment #357166 -
Flags: superreview?(roc)
Attachment #357166 -
Flags: superreview+
Attachment #357166 -
Flags: review?(roc)
Attachment #357166 -
Flags: review+
Attachment #357182 -
Flags: superreview?(roc)
Attachment #357182 -
Flags: superreview+
Attachment #357182 -
Flags: review?(roc)
Attachment #357182 -
Flags: review+
// Note that just returning is probably not right. According // to the spec, <use> is allowed to use an element that fails its // conditional, but because we never actually create the frame when // a conditional fails and when we use GetReferencedFrame to find the // references, things don't work right. // XXX FIXME XXX This comment is probably obsolete since our <use> code doesn't use GetReferencedFrame. Something to clean up another day.
Attachment #357169 -
Flags: superreview?(roc)
Attachment #357169 -
Flags: superreview+
Attachment #357169 -
Flags: review?(roc)
Attachment #357169 -
Flags: review+
Attachment #357170 -
Flags: superreview?(roc)
Attachment #357170 -
Flags: superreview+
Attachment #357170 -
Flags: review?(roc)
Attachment #357170 -
Flags: review+
Comment on attachment 357170 [details] [diff] [review] Part 18. Eliminate IsSpecialContent; updated to new approach That's very nice.
![]() |
Assignee | |
Comment 40•12 years ago
|
||
> {} around the statement, I guess
Done locally.
![]() |
Assignee | |
Comment 41•12 years ago
|
||
I found one issue with the xul patch by running mochitest. The fix is: - SCROLLABLE_XUL_CREATE(resizer, NS_NewTitleBarFrame), + SCROLLABLE_XUL_CREATE(resizer, NS_NewResizerFrame), Made that change locally.
Updated•12 years ago
|
Attachment #357182 -
Flags: review?(jwatt) → review+
Updated•12 years ago
|
Attachment #357169 -
Flags: review?(jwatt) → review+
![]() |
Assignee | |
Comment 42•12 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/1478b7f877f6 http://hg.mozilla.org/mozilla-central/rev/84e613c14c71 http://hg.mozilla.org/mozilla-central/rev/ea0963a0bf01 http://hg.mozilla.org/mozilla-central/rev/2ce70aa933a4 http://hg.mozilla.org/mozilla-central/rev/f40919be02bb http://hg.mozilla.org/mozilla-central/rev/aa3a5a9358d2 http://hg.mozilla.org/mozilla-central/rev/692ae2bf70de http://hg.mozilla.org/mozilla-central/rev/935b530a288e http://hg.mozilla.org/mozilla-central/rev/dc16f1b1eb7a http://hg.mozilla.org/mozilla-central/rev/e099a47f752e http://hg.mozilla.org/mozilla-central/rev/a5381d3f2c90 http://hg.mozilla.org/mozilla-central/rev/8711935e60ad http://hg.mozilla.org/mozilla-central/rev/e340f8fa90ca http://hg.mozilla.org/mozilla-central/rev/a08c5203f75f http://hg.mozilla.org/mozilla-central/rev/e2842400a87a http://hg.mozilla.org/mozilla-central/rev/422ebbfbdc87 http://hg.mozilla.org/mozilla-central/rev/a50d9d9c5408 http://hg.mozilla.org/mozilla-central/rev/cc3b3a8f35cb Seems to have no obviout perf impact. On Linux we have: Zdiff:-5376 (+15685/-21061) On Mac we have: Zdiff:-20472 (+149896/-170368) (some of which is bogus, because it has nothing to do with this patch). Marking fixed. Thank you for the quick reviews!
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Wow, this rocks! Thanks everyone!!
Updated•12 years ago
|
Target Milestone: --- → mozilla1.9.2a1
Comment 44•12 years ago
|
||
I just pushed a simple bustage fix for Thunderbird trunk as it builds without MathML: http://hg.mozilla.org/mozilla-central/rev/8e432630fed0 diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp --- a/layout/base/nsCSSFrameConstructor.cpp +++ b/layout/base/nsCSSFrameConstructor.cpp @@ -5136,7 +5136,9 @@ nsCSSFrameConstructor::ConstructFrameFro CHECK_ONLY_ONE_BIT(FCDATA_SKIP_FRAMEMAP, FCDATA_MAY_NEED_SCROLLFRAME); CHECK_ONLY_ONE_BIT(FCDATA_FUNC_IS_FULL_CTOR, FCDATA_DISALLOW_OUT_OF_FLOW); CHECK_ONLY_ONE_BIT(FCDATA_FUNC_IS_FULL_CTOR, FCDATA_FORCE_NULL_ABSPOS_CONTAINER); +#ifdef MOZ_MATHML CHECK_ONLY_ONE_BIT(FCDATA_FUNC_IS_FULL_CTOR, FCDATA_WRAP_KIDS_IN_BLOCKS); +#endif CHECK_ONLY_ONE_BIT(FCDATA_FUNC_IS_FULL_CTOR, FCDATA_MAY_NEED_SCROLLFRAME); CHECK_ONLY_ONE_BIT(FCDATA_FUNC_IS_FULL_CTOR, FCDATA_IS_POPUP); CHECK_ONLY_ONE_BIT(FCDATA_FUNC_IS_FULL_CTOR, FCDATA_SKIP_ABSPOS_PUSH);
Comment 45•12 years ago
|
||
Comment on attachment 356753 [details] [diff] [review] Part 4. Nuke vestiges of XBL form controls. Is this part (at least) wanted on 1.9.1 ? (See bug 172288)
![]() |
Assignee | |
Comment 46•12 years ago
|
||
No. The code is not hurting anything on 1.9.1, and I see no reason to touch it there.
Updated•3 years ago
|
Product: Core → Core Graveyard
Updated•3 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•