441367 - SetDiscrete() for nsRuleNode.cpp

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: patch

This is a first step in eliminating the many if-else chains in nsRuleNode.cpp.  It introduces a helper function, SetDiscrete(), analogous to the existing SetCoord() and SetColor(), which can be used for any CSS property whose value is an integer or enumeration.

Potential issues with the patch as it stands:
 - SetDiscrete takes a reference to the style struct field to set, which can be any integer type; the most natural way to do that in modern C++ is to make it a template function, but I'm not sure whether that's kosher with Moz style rules.
 - Most of the data-source arguments are PRInt32s, which is the lowest common denominator; unfortunately, this causes even more -Wconversion warnings.
 - There are six different data-source arguments, and no caller uses all of them.  Given the lack of keyword arguments in C++ I do not have a better idea, but it's difficult to call the function correctly without having its prototype visible for reference.
 - I had to make several fields in nsFont and nsStyleBackground not be bitfields, so they could have references taken.  Given that bug 339358 (which made more things into bitfields) was backed out for the sake of bug 332335, I'm guessing this is okay, but it should be mentioned.

asking roc to review as dbaron is out this week.
note that the patch as shown is on top of my patches for bug 363706 and bug 437335; i can respin without that dependency if desired.

Comment 1

[Zack Weinberg (:zwol)]

I should have mentioned that in the patch, SetDiscrete() replaces 52 if-else chains, and could replace a few more if not for special cases in the handling of eStyleUnit_Enumerated.  Also, this passes reftests (trying mochitests next, over lunch break).

Comment 2

[Zack Weinberg (:zwol)]

mochitest currently blows up horribly when run on *unmodified* trunk.  Bleah.

Comment 3

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

Not on tinderboxes it doesn't.

You really need to run mochitest on a dedicated machine. If the window loses focus you will get errors.

Comment 4

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

Comment on attachment 326345 [details] [diff] [review]
patch

Sorry, but I'm actually not a style system peer. It looks good though.

Comment 5

[Zack Weinberg (:zwol)]

(In reply to comment #3)
> You really need to run mochitest on a dedicated machine. If the window loses
> focus you will get errors.

I did it in an xvfb server (see bug 434365) which *used* to work; there were about 110 failures, most of which appear not to have anything to do with focus; and then it stopped making forward progress after about 64000 tests.

With both mochitest and reftest I always get a handful of failures that don't show up on the tinderboxes, but they're always the same handful and I assume it's something to do with my local machine, like which fonts I have.  This is different.

(In reply to comment #4)
> Sorry, but I'm actually not a style system peer. It looks good though.

Can you at least comment on the coding-style concerns outlined in comment #1 (esp. use of templates and removal of bitfields)?

Comment 6

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

(In reply to comment #0)
>  - SetDiscrete takes a reference to the style struct field to set, which can
> be any integer type; the most natural way to do that in modern C++ is to make
> it a template function, but I'm not sure whether that's kosher with Moz style
> rules.

I think we should allow it.

>  - Most of the data-source arguments are PRInt32s, which is the lowest common
> denominator; unfortunately, this causes even more -Wconversion warnings.

You can silence them with constructor casts, right?

>  - There are six different data-source arguments, and no caller uses all of
> them.  Given the lack of keyword arguments in C++ I do not have a better idea,
> but it's difficult to call the function correctly without having its prototype
> visible for reference.

Perhaps use a struct to get syntax like
DiscreteSetter<PRUint8>().SetInitial(...).SetAuto(...).Apply(...)?

>  - I had to make several fields in nsFont and nsStyleBackground not be
> bitfields, so they could have references taken.  Given that bug 339358 (which
> made more things into bitfields) was backed out for the sake of bug 332335,
> I'm guessing this is okay, but it should be mentioned.

I think that's OK.

Comment 7

[Zack Weinberg (:zwol)]

(In reply to comment #6)
> (In reply to comment #0)
> >  - SetDiscrete [is] a template function, but I'm not sure whether
> >    that's kosher with Moz style rules.
> 
> I think we should allow it.

Ok, I won't worry about that any more.

> > this causes even more -Wconversion warnings.
> 
> You can silence them with constructor casts, right?

I haven't tried.  There are so many warnings in nsRuleNode.cpp already, it's really hard to tell which ones come from new code.  I think I might back out all my patches and then do some warnings patrol (which is going to involve
figuring out how to shut up "invalid use of offsetof()", fun fun).

> Perhaps use a struct to get syntax like
> DiscreteSetter<PRUint8>().SetInitial(...).SetAuto(...).Apply(...)?

I like this idea but I am not sure it will work with dbaron's ultimate
goal of making the file table-driven.  So I'm going to hold off on that
till he can comment.

> >  - I had to make several fields in nsFont and nsStyleBackground not be
> > bitfields
> 
> I think that's OK.

Check.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 326345 [details] [diff] [review]
patch

I think SetDiscrete should have assertions when it doesn't set a value,
since those cases shouldn't happen.  In particular:
 * each of the eCSSUnit_* cases should have an NS_ASSERTION that the
   correct bit in the mask is set
 * the default case should have an NS_NOTREACHED().

Additionally, it seems like:
 * aParentValue should just be FieldType, not FieldType const &.
 * aInitialValue ... aSystemFontValue should also be FieldType.

That said, the template stuff would need to go away before it's actually
table-driven, although I'm not sure if completely table-driven will
actually be needed.

+  // direction: rtl will only be effective if bidi is enabled
+  if (visibility->mDirection == NS_STYLE_DIRECTION_RTL &&
+      displayData.mDirection.GetUnit() == eCSSUnit_Enumerated)
+    mPresContext->SetBidiEnabled();

It might actually be worth moving this chunk here:
http://hg.mozilla.org/mozilla-central/index.cgi/annotate/6b3d5ee8d6a782bc280adcd9baebb6db8cd3f362/layout/base/nsCSSFrameConstructor.cpp#l7520


r+sr=dbaron with that.

Sorry for taking so long to get to this ... I'm going to try to do
better in the future.

Comment 9

[Zack Weinberg (:zwol)]

(In reply to comment #8)
> (From update of attachment 326345 [details] [diff] [review])
> I think SetDiscrete should have assertions when it doesn't set a value,
> since those cases shouldn't happen.

I'll try it, but ... really?  My impression of the code this is replacing was that other units would just be silently ignored.

>  * aParentValue should just be FieldType, not FieldType const &.

Ok.

>  * aInitialValue ... aSystemFontValue should also be FieldType.

This doesn't *quite* work; you have to permit them to be different from FieldType or you get errors like so:

nsRuleNode.cpp:2338: error: no matching function for call to 
  ‘SetDiscrete(const nsCSSValue&, PRUint8&, int&, int, const PRUint8&, 
               const PRUint8&, int, int, int, PRUint8&)’

because there's no way to tell the compiler that it should pick FieldType based only on the type of the second argument, and then do implicit conversions for all of the other arguments declared as FieldType.  What I've currently got that and works, is

template <typename FieldT,
          typename T1, typename T2, typename T3, typename T4, typename T5>
static void
SetDiscrete(const nsCSSValue& aValue, FieldT & aField,
            PRBool& aInherited, PRUint32 aMask,
            FieldT aParentValue,
            T1 aInitialValue,
            T2 aAutoValue,
            T3 aNoneValue,
            T4 aNormalValue,
            T5 aSystemFontValue)

but this is ugly enough that I'd appreciate being told yes/no on it specifically.  It does increase the number of instantiations emitted in the object file, but only from three to four.

> That said, the template stuff would need to go away before it's actually
> table-driven, although I'm not sure if completely table-driven will
> actually be needed.

Right.  I figure we'll cross that bridge when we come to it.

> +  // direction: rtl will only be effective if bidi is enabled
> 
> It might actually be worth moving this chunk here:
> http://hg.mozilla.org/mozilla-central/index.cgi/annotate/6b3d5ee8d6a782bc280adcd9baebb6db8cd3f362/layout/base/nsCSSFrameConstructor.cpp#l7520

Lemme make sure I understand what you mean: the code you quoted comes out of ComputeVisibilityData, and the part of nsCSSFrameConstructor you pointed to becomes something like

  // If the page contains markup that overrides text direction, and
  // does not contain any characters that would activate the Unicode
  // bidi algorithm, we need to call |SetBidiEnabled| on the pres
  // context before reflow starts.  This requires us to resolve some
  // style information now.  See bug 115921.
  {
    if (styleContext->GetStyleVisibility()->mDirection ==
        NS_STYLE_DIRECTION_RTL)
      mPresContext->SetBidiEnabled();
  }

Note that this will call SetBidiEnabled() even if the DIRECTION_RTL setting wasn't explicit in the style, but SetBidiEnabled() appears to just set a flag on the document object, so that shouldn't be a problem.

Revised patch will follow after a test cycle.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: patch v2: dbaron's requested changes

Here's the new patch.  Besides dbaron's requested changes, there's one other tweak: lots of places were calling SetDiscrete() with aValue being eCSSUnit_Null, so I just had that do nothing.

Requesting re-review despite dbaron's "approval with changes" because of the hideous six-argument template that SetDiscrete has now become - see previous comment.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: (followup patch) SetDiscrete cleanups, SetFactor

Since I came back wanting more review, I figure I might as well piggyback some more changes on this bug.  The attached applies *on top of* attachment 333364 [details] [diff] [review] and does three things: simplify the assertions in SetDiscrete, convert a couple cases to SetDiscrete that I missed before, and introduce a new function SetFactor that can take over another handful of if-else chains.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 333364 [details] [diff] [review]
patch v2: dbaron's requested changes

(In reply to comment #9)
> I'll try it, but ... really?  My impression of the code this is replacing was
> that other units would just be silently ignored.

The only unit that needed to be silently ignored was eCSSUnit_Null.  All 
the if-else chains really should have ended with assertions that that 
was the unit -- otherwise that would mean there were values that we
parsed but then ignored during the cascade... in somewhat random ways
because of the optimizations in our implementation.  (Per CSS error
handling, we shouldn't have parsed them at all.)

r+sr=dbaron

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 333471 [details] [diff] [review]
(followup patch) SetDiscrete cleanups, SetFactor

>More nsRuleNode helper functions: SetFactor and SetBoolean.

SetBoolean isn't in this patch.

>+    if (aFlags & SETFCT_OPACITY) {
>+      aField = PR_MAX(aField, 0.0f);
>+      aField = PR_MIN(aField, 1.0f);
>+    }

I slightly prefer the way it was done for 'opacity' rather than in SetSVGOpacity:

>-    if (display->mOpacity > 1.0f)
>-      display->mOpacity = 1.0f;
>-    if (display->mOpacity < 0.0f)
>-      display->mOpacity = 0.0f;

but leave it if you want.

r+sr=dbaron

Comment 14

[Zack Weinberg (:zwol)]

(In reply to comment #13)
> SetBoolean isn't in this patch.

Oh right.  I thought I was going to do that one and then I realized that the places where it would be useful didn't have enough in common, but I forgot to edit the patch header.

> I slightly prefer the way it was done for 'opacity' rather than in
> SetSVGOpacity:
> 
> >-    if (display->mOpacity > 1.0f)
> >-      display->mOpacity = 1.0f;
> >-    if (display->mOpacity < 0.0f)
> >-      display->mOpacity = 0.0f;

I'll change it.  It's less compact but it's more obvious that it's correct -- I always have to stare at max/min a bit to be sure they're the right way around.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: combined final patch [Checkin: Comment 17]

This patch combines both the original and follow-up changes, includes the small change requested above, and is ready to check in.

Comment 16

[Zack Weinberg (:zwol)]

tagging for checkin

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed: http://hg.mozilla.org/mozilla-central/rev/08bf39f0754a

(Not sure if that makes this bug fixed or not; leaving to Zack to mark.)

Comment 18

[Zack Weinberg (:zwol)]

yeah, I'll file new bugs for further work.