Closed Bug 473390 Opened 11 years ago Closed 11 years ago

[FIX]Switch XUL/HTML/MathML/SVG frame construction to be more table-driven and eliminate IsSpecialContent

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(19 files, 8 obsolete files)

3.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.43 KB, patch
roc
: review+
Details | Diff | Splinter Review
15.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.82 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.57 KB, patch
roc
: review+
Details | Diff | Splinter Review
39.24 KB, patch
roc
: review+
Details | Diff | Splinter Review
12.91 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.91 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.28 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.48 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.22 KB, patch
Details | Diff | Splinter Review
4.27 KB, patch
roc
: review+
Details | Diff | Splinter Review
57.24 KB, patch
roc
: review+
Details | Diff | Splinter Review
17.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
45.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
31.33 KB, patch
roc
: review+
jwatt
: review+
Details | Diff | Splinter Review
22.17 KB, patch
roc
: review+
Details | Diff | Splinter Review
79.39 KB, patch
jwatt
: review+
roc
: review+
Details | Diff | Splinter Review
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...
Attachment #356753 - Flags: superreview?(roc)
Attachment #356753 - Flags: review?(roc)
Attachment #356756 - Flags: superreview?(roc)
Attachment #356756 - Flags: review?(roc)
Attachment #356757 - Flags: superreview?(roc)
Attachment #356757 - Flags: review?(roc)
Attachment #356758 - Flags: superreview?(roc)
Attachment #356758 - Flags: review?(roc)
Attached patch Part 9. New setup for MathML (obsolete) — Splinter Review
Attachment #356759 - Flags: superreview?(roc)
Attachment #356759 - Flags: review?(roc)
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)
Attached patch Part 15. New setup for XUL (obsolete) — Splinter Review
Attachment #356768 - Flags: superreview?(roc)
Attachment #356768 - Flags: review?(roc)
Attachment #356770 - Flags: superreview?(roc)
Attachment #356770 - Flags: review?(roc)
Attachment #356770 - Flags: review?(jwatt)
Attached patch Part 17. new setup for SVG (obsolete) — Splinter Review
Attachment #356771 - Flags: superreview?(roc)
Attachment #356771 - Flags: review?(roc)
Attachment #356771 - Flags: review?(jwatt)
Attachment #356773 - Flags: superreview?(roc)
Attachment #356773 - Flags: review?(roc)
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 #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+
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 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.
> 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....
> 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?
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.
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)
Attachment #356746 - Attachment is obsolete: true
Attachment #356750 - Attachment is obsolete: true
Attachment #357165 - Flags: superreview?(roc)
Attachment #357165 - Flags: review?(roc)
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)
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)
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)
Attachment #356773 - Attachment is obsolete: true
Attachment #356773 - Flags: superreview?(roc)
Attachment #356773 - Flags: review?(roc)
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.
> {} around the statement, I guess

Done locally.
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.
Attachment #357182 - Flags: review?(jwatt) → review+
Attachment #357169 - Flags: review?(jwatt) → review+
Wow, this rocks! Thanks everyone!!
Target Milestone: --- → mozilla1.9.2a1
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);
Depends on: 475582
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)
No.  The code is not hurting anything on 1.9.1, and I see no reason to touch it there.
Product: Core → Core Graveyard
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.