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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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...

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 1. Drop one IsSpecialContent caller. Makes us construct a box, not a table for a XUL root with display:table

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 1 diff -w for ease of review

Comment 3

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 2. Align the way we call SetInitialChildList on HTML frames with the rest of the world

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 3. Move some HTML frame construction assertions around.

Comment 5

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 4. Nuke vestiges of XBL form controls.

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 5. Simplify checkbox/radio frame construction

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 6. Move file control flag setting.

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 7. New setup for HTML; read the .h diff first

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 8. <math:math> simplification.

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 9. New setup for MathML

Comment 11

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 10. Move some nsIBoxLayout stuff out of frame construction

Comment 12

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 11. Give some nsIBoxLayout creation functions nicer signatures

Comment 13

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 12. Simplify some of the parent frame stuff in XUL frame construction

Comment 14

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 13. Remove unused argument from BuildScrollFrame

Comment 15

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 14. Remove mBoxFrame member from the frame constructor state

Comment 16

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 15. New setup for XUL

Comment 17

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 16. Move around some SVG assertions and eliminate the corresponding checks

Comment 18

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 17. new setup for SVG

Comment 19

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 18. Eliminate IsSpecialContent

Comment 20

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-)

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 23

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 1, the correct diff.

Comment 24

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 2, correct diff

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #25)
> Document sentinel values used?

Oh, I just realized there are none, so never mind.

Comment 27

[Robert Longson [:longsonr]]

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.

Comment 28

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> 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....

Comment 29

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> 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?

Comment 30

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 31

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 7. New setup for HTML; updated to new approach.

Comment 32

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 9. New setup for MathML; updated to new approach

Comment 33

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 15. New setup for XUL; updated to new approach

Comment 34

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 17. New setup for SVG; updated to new approach

Comment 35

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 18. Eliminate IsSpecialContent; updated to new approach

Comment 36

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 16 update. Move around some SVG asserts.

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.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

     // 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.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 357170 [details] [diff] [review]
Part 18.  Eliminate IsSpecialContent; updated to new approach

That's very nice.

Comment 40

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> {} around the statement, I guess

Done locally.

Comment 41

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 42

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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!

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wow, this rocks! Thanks everyone!!

Comment 44

[Mark Banner (:standard8)]

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

[Serge Gautherie (:sgautherie)]

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)

Comment 46

[Boris Zbarsky [:bzbarsky, bz on IRC]]

No.  The code is not hurting anything on 1.9.1, and I see no reason to touch it there.