323230 - Need FindAttrValueIn

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

We have lots of situations where we need to check an attribute value against N alternatives. AttrValueIs isn't a good fit for this because it looks up the attribute again each time. Exposing nsAttrValue to these callers is probably not a good idea.

I propose a new API:
PRInt32 nsIContent::FindAttrValueIn(PRInt32 aNameSpaceID,
                                    nsIAtom* aName,
                                    nsIAtom** aValues,
                                    nsCaseTreatment aCaseSensitive) const
aValues is a NULL-terminated list of value atoms. The method returns the index of the first matched value, or -1 if there is no match.

Then a caller like this:
http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsBoxFrame.cpp#245
  nsAutoString value;
  mContent->GetAttr(kNameSpaceID_None, nsXULAtoms::mousethrough, value);
  if (value.EqualsLiteral("never"))
    mMouseThrough = never;
  else if (value.EqualsLiteral("always"))
    mMouseThrough = always;
could be rewritten

  static nsIAtom* strings[] = { nsXULAtoms::never, nsXULAtoms::always, nsnull };
  static eMouseThrough values[] = { never, always };
  PRInt32 match = mContent->FindAttrValueIn(kNameSpaceID_None, nsXULAtoms::mousethrough, values);
  if (match >= 0) {
    mMouseThrough = values[match];
  }

In other situations you would need to do a switch on the result:
  static nsIAtom* strings[] = { nsXULAtoms::never, nsXULAtoms::always, nsnull };
  switch (mContent->FindAttrValueIn(kNameSpaceID_None, nsXULAtoms::mousethrough, values)) {
    case 0: mMouseThrough = never; break;
    case 1: mMouseThrough = always; break;
  }
That isn't great because it looks easy to misalign the switch cases with the corresponding strings. But I don't have any better ideas right now. Lots of cases don't need a switch, fortunately.

Comment 1

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

Alternativly, we could have a base class to nsAttrValue that only allowed to compare the value to a string and put a function on nsIContent that returns such a baseclass.

Comment 2

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

(In reply to comment #1)
> Alternativly, we could have a base class to nsAttrValue that only allowed to
> compare the value to a string and put a function on nsIContent that returns
> such a baseclass.

I don't like that so much. The caller then has to worry about the lifetime of the nsAttrValue. It also means a bunch of if/else code is still required, which can be avoided when the result is used to look up another array, as in my first example.

Comment 3

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

Jonas points out that using
static nsIAtom* strings[] = { &nsXULAtoms::never, &nsXULAtoms::always, nsnull
};
is more efficient than what I had, because the static array can be built at link time and doesn't require the guard code that C++ compilers introduce to prevent repeated initialization. So we should probably go with that.

Comment 4

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

If we're worried about the situations where a switch is needed and the labels might misalign with the strings, we could have another variant:

nsIAtom** nsIContent::FindAttrValueAtomIn(PRInt32 aNameSpaceID,
                                          nsIAtom* aName,
                                          nsIAtom*** aValues,
                                          nsCaseTreatment aCaseSensitive) const
which returns the actual value from the aValues array that matched. Then you could write

  static nsIAtom** strings[] = { &nsXULAtoms::never, &nsXULAtoms::always, nsnull
};
  nsIAtom** match = mContent->FindAttrValueIn(kNameSpaceID_None,
nsXULAtoms::mousethrough, values);
  if (match == &nsXULAtoms::never) {
    mMouseThrough = never;
  } else if (match == &nsXULAtoms::always) {
    mMouseThrough = always;
  }

Comment 5

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

I liked the index idea actually. You can use that safely by doing

n = cont->FindAttrValueIn(kNameSpaceID_None, nsXULAtoms::mousethrough, atoms);
if (n > 0) {
  nsIAtom* value = atoms[n];
  if (value = ...) {
  }
  else if (value = ...) {
  }
  ...


Or you could create two arrays, one with enum values and one with atoms and do

n = cont->FindAttrValueIn(kNameSpaceID_None, nsXULAtoms::mousethrough, atoms);
if (n > 0) {
  mMouseThrough = vals[n];
}

And hopefully some of us are grown up enough to even use the switch version in proper manner :)

Comment 6

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

I don't mean to fight your every suggestion, i just really liked your initial proposal :)

Comment 7

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

Btw, it might be worth considering adding an inline function on nsIContent like

PRBool IsAttrValueIn(PRInt32 aNameSpaceID,
                     nsIAtom* aName,
                     nsIAtom** aValues,
                     nsCaseTreatment aCaseSensitive)
{
  return FindAttrValueIn(aNameSpaceID, aName, aValues, aCaseSensitive) >= 0;
}


(adjust test depending on if FindAttrValueIn returns an index or a pointer)

Comment 8

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

Attachment: fix

I found it helpful to distinguish "attribute exists, but does not match anything in the list" from "attribute does not exist" in the result codes.

This patch converts nsBoxFrame and a couple of places in nsEventStateManager to the new API. It also adds a couple of uses of AttrValueIs in nsEventStateManager.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 208621 [details] [diff] [review]
fix

>Index: content/base/public/nsIContent.h

>+   * @return 

Say "described above" or something, I guess?

>Index: content/events/src/nsEventStateManager.cpp
>@@ -1144,27 +1144,28 @@ nsEventStateManager::CreateClickHoldTime
>+    static nsIAtom* const* const strings[] = {&nsXULAtoms::_empty, NULL};

s/NULL/nsnull/?

>@@ -1254,37 +1255,37 @@ nsEventStateManager::FireContextClick()
>+          static nsIAtom* const* const strings[] = {&nsXULAtoms::_empty, NULL};
>+          PRInt32 index = mGestureDownContent->FindAttrValueIn(kNameSpaceID_None,
>+              nsXULAtoms::container, strings, eCaseMatters);
>+          if (index == nsIContent::ATTR_VALUE_NO_MATCH) {
>             allowedToDispatch = PR_FALSE;
>+          } else {
>+            // If the toolbar button has an open menu, don't attempt to open
>+            // a second menu
>+            if (mGestureDownContent->AttrValueIs(kNameSpaceID_None, nsXULAtoms::open,
>+                                                 nsXULAtoms::_true, eCaseMatters)) {
>+              allowedToDispatch = PR_FALSE;

It seems that this might (to me at least) be more readable as:

  static nsIAtom* const* const strings[] = {&nsXULAtoms::_empty, nsnull};
  if (mGestureDownContent->FindAttrValueIn(kNameSpaceID_None,
                nsXULAtoms::container, strings, 
                eCaseMatters) == nsIContent::ATTR_VALUE_NO_MATCH ||
      mGestureDownContent->AttrValueIs(kNameSpaceID_None, nsXULAtoms::open,
                                       nsXULAtoms::_true, eCaseMatters)) {
    allowedToDispatch = PR_FALSE;
  }

or something.  That is, instead of nested ifs use a single if with ||.  Either way, though.  It's hard to tell how readable stuff is in this bugzilla textfield.  ;)

>@@ -4906,27 +4907,26 @@ nsEventStateManager::MoveFocusToCaret(PR
>       *aIsSelectionWithFocus = testContent->HasAttr(kNameSpaceID_XLink, nsHTMLAtoms::href);
>       if (*aIsSelectionWithFocus) {
>-        nsAutoString xlinkType;
>-        testContent->GetAttr(kNameSpaceID_XLink, nsHTMLAtoms::type, xlinkType);
>-        if (!xlinkType.EqualsLiteral("simple")) {
>+        if (!testContent->AttrValueIs(kNameSpaceID_XLink, nsHTMLAtoms::type,
>+                                      nsHTMLAtoms::simple, eCaseMatters)) {
>           *aIsSelectionWithFocus = PR_FALSE;  // Xlink must be type="simple"

I think should should definitely be:

  *aIsSelectionWithFocus = 
    testContent->HasAttr(kNameSpaceID_XLink, nsHTMLAtoms::href) &&
    testContent->AttrValueIs(kNameSpaceID_XLink, nsHTMLAtoms::type,
                             nsHTMLAtoms::simple, eCaseMatters);

Makes it easier to translate the code into English.  ;)

>Index: content/xul/content/src/nsXULAtomList.h

I assume what you did here is alphabetized the list and added _empty?  If not, what other changes?  This part is pretty much impossible to actually review...

For what it's worth, I thought there was some benefit in having the atoms grouped by what actually used them; is there a reason we really prefer alphabetizing?

>Index: content/xul/content/src/nsXULElement.cpp
>Index: layout/xul/base/src/nsBoxFrame.cpp
> nsBoxFrame::GetInitialHAlignment(nsBoxFrame::Halignment& aHalign)

>+  static const Halignment values[] =
>+    {hAlign_Left, hAlign_Left, hAlign_Center, hAlign_Right};

Comment that the first entry is unused?  Or use [index-1] to index into this array and just remove that first entry?

> nsBoxFrame::GetInitialVAlignment(nsBoxFrame::Valignment& aValign)

This code screams to me of attribute mapping.  :(  We should really get that set up for XUL...

>+  static nsIAtom* const* const strings[] =
>+    {&nsXULAtoms::_empty, &nsXULAtoms::start, &nsXULAtoms::center,
>+     &nsXULAtoms::baseline, &nsXULAtoms::end, NULL};

Same comment as for GetInitialHAlignment.

sr=bzbarsky with those nits.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 208621 [details] [diff] [review]
fix

Oh, and bump the nsIContent IID, please.

Comment 11

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

(In reply to comment #9)
> >Index: content/xul/content/src/nsXULAtomList.h
> 
> I assume what you did here is alphabetized the list and added _empty?  If
> not, what other changes?  This part is pretty much impossible to actually
> review...

Er yeah, sorry. I added _empty and a number of other atoms used by the new code.

> For what it's worth, I thought there was some benefit in having the atoms
> grouped by what actually used them; is there a reason we really prefer
> alphabetizing?

It makes it a lot easier to see whether your atom already exists. Also, it wasn't clear where I should be adding the new atoms; the existing groups were not labelled in most cases.

> >Index: content/xul/content/src/nsXULElement.cpp
> >Index: layout/xul/base/src/nsBoxFrame.cpp
> > nsBoxFrame::GetInitialHAlignment(nsBoxFrame::Halignment& aHalign)
> 
> >+  static const Halignment values[] =
> >+    {hAlign_Left, hAlign_Left, hAlign_Center, hAlign_Right};
> 
> Comment that the first entry is unused?  Or use [index-1] to index into this
> array and just remove that first entry?

I'll do the former.

> > nsBoxFrame::GetInitialVAlignment(nsBoxFrame::Valignment& aValign)
> 
> This code screams to me of attribute mapping.  :(  We should really get that
> set up for XUL...

Yes, although in some places here attributes override the corresponding CSS property :-(.

Agree with all your other comments.

Comment 12

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

Comment on attachment 208621 [details] [diff] [review]
fix

>Index: content/base/public/nsIContent.h
>+  enum {
>+    ATTR_VALUE_MISSING = -1,
>+    ATTR_VALUE_NO_MATCH = -2
>+  };

These names are a bit too similar IMHO. How about renaming the first ATTR_MISSING or ATTR_NOT_SET? And add a comment next to each value describing what they mean. Like "Attribute  does not exist" and "Attribute exists but value didn't match"

>+  virtual PRInt32 FindAttrValueIn(PRInt32 aNameSpaceID,
>+                                  nsIAtom* aName,
>+                                  nsIAtom* const* const* aValues,

Would it be possible to get a typedef for this that can be used on each callsite?

>Index: content/events/src/nsEventStateManager.cpp
>@@ -1144,27 +1144,28 @@ nsEventStateManager::CreateClickHoldTime
...
>-    nsAutoString popup;
>-    mGestureDownContent->GetAttr(kNameSpaceID_None, nsXULAtoms::popup, popup);
>-    if (!popup.IsEmpty())
>+    static nsIAtom* const* const strings[] = {&nsXULAtoms::_empty, NULL};
>+    PRInt32 index = mGestureDownContent->FindAttrValueIn(kNameSpaceID_None,
>+        nsXULAtoms::popup, strings, eCaseMatters);
>+    if (index == nsIContent::ATTR_VALUE_NO_MATCH)
>       return;

Neat trick :), wouldn't mind a comment though. Actually, it might be worth adding a function to nsContentUtils for this, like
nsContentUtils::HasNonEmptyAttr(nsIContent*, PRInt32, nsIAtom*)

You'd have two callers in this patch, and I bet there are a lot more out there.

>Index: layout/xul/base/src/nsBoxFrame.cpp
> nsBoxFrame::GetInitialDebug(PRBool& aDebug)
> {
>-  nsAutoString value;
>-
>   nsCOMPtr<nsIContent> content;
>   GetContentOf(getter_AddRefs(content));
> 
>   if (!content)
>     return PR_FALSE;
> 
>-  content->GetAttr(kNameSpaceID_None, nsXULAtoms::debug, value);
>-  if (value.EqualsLiteral("true")) {
>-      aDebug = PR_TRUE;
>-      return PR_TRUE;
>-  } else if (value.EqualsLiteral("false")) {
>-      aDebug = PR_FALSE;
>-      return PR_TRUE;
>+  static nsIAtom* const* const strings[] =
>+    {&nsXULAtoms::_false, &nsXULAtoms::_true, NULL};
>+  PRInt32 index = content->FindAttrValueIn(kNameSpaceID_None,
>+      nsXULAtoms::debug, strings, eCaseMatters);
>+  if (index >= 0) {
>+    aDebug = index;
>+    return PR_TRUE;

ewww! Relying on the values of bools! I wouldn't mind:
aDebug = index == 1;

or

aDebug = index != 0;

> nsBoxFrame::GetInitialHAlignment(nsBoxFrame::Halignment& aHalign)
...
>-  if (IsHorizontal())
>-    content->GetAttr(kNameSpaceID_None, nsXULAtoms::pack, value);
>-  else
>-    content->GetAttr(kNameSpaceID_None, nsHTMLAtoms::align, value);
>-
>-  if (!value.IsEmpty()) {
>-    if (value.EqualsLiteral("start")) {
>-        aHalign = nsBoxFrame::hAlign_Left;
>-        return PR_TRUE;
>-    } else if (value.EqualsLiteral("center")) {
>-        aHalign = nsBoxFrame::hAlign_Center;
>-        return PR_TRUE;
>-    } else if (value.EqualsLiteral("end")) {
>-        aHalign = nsBoxFrame::hAlign_Right;
>-        return PR_TRUE;
>-    }
>+  nsIAtom* attrName = IsHorizontal() ? nsXULAtoms::pack : nsHTMLAtoms::align;
>+  static nsIAtom* const* const strings[] =
>+    {&nsXULAtoms::_empty, &nsXULAtoms::start, &nsXULAtoms::center, &nsXULAtoms::end, NULL};
>+  static const Halignment values[] =
>+    {hAlign_Left, hAlign_Left, hAlign_Center, hAlign_Right};
>+  index = content->FindAttrValueIn(kNameSpaceID_None, attrName,
>+      strings, eCaseMatters);

How about placing _empty last in the list and make the below test |index >= 0 && index < 3|
Same in GetInitialVAlignment.

> nsBoxFrame::GetInitialOrientation(PRBool& aIsHorizontal)
...
>-  content->GetAttr(kNameSpaceID_None, nsXULAtoms::orient, value);
>-  if (value.EqualsLiteral("vertical"))
>-    aIsHorizontal = PR_FALSE;
>-  else if (value.EqualsLiteral("horizontal"))
>-    aIsHorizontal = PR_TRUE;
>+  static nsIAtom* const* const strings[] =
>+    {&nsXULAtoms::vertical, &nsXULAtoms::horizontal, NULL};
>+  PRInt32 index = content->FindAttrValueIn(kNameSpaceID_None, nsXULAtoms::orient,
>+      strings, eCaseMatters);
>+  if (index >= 0) {
>+    aIsHorizontal = index;
>+  }

aIsHorizontal = index == 1;


> nsBoxFrame::GetInitialDirection(PRBool& aIsNormal)
...
>+  static nsIAtom* const* const strings[] =
>+    {&nsXULAtoms::reverse, &nsXULAtoms::ltr, &nsXULAtoms::rtl, NULL};
>+  PRInt32 index = content->FindAttrValueIn(kNameSpaceID_None, nsXULAtoms::dir,
>+      strings, eCaseMatters);
>+  if (index >= 0) {
>+    PRPackedBool values[] = {!aIsNormal, PR_TRUE, PR_FALSE};

What does this compile to? I guess the same thing as setting three variables so it's not too bad.

r=me with that and bzs comments.

fwiw, i do prefer atoms sorted by name too. Generally when I add an atom there is no group that matches my new atom, it fits in more then one group, or I can't figure out what the different groups are.

Comment 13

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

(In reply to comment #12)
> These names are a bit too similar IMHO. How about renaming the first
> ATTR_MISSING or ATTR_NOT_SET?

ATTR_MISSING it is.

> >+  virtual PRInt32 FindAttrValueIn(PRInt32 aNameSpaceID,
> >+                                  nsIAtom* aName,
> >+                                  nsIAtom* const* const* aValues,
> 
> Would it be possible to get a typedef for this that can be used on each
> callsite?

OK

> Neat trick :), wouldn't mind a comment though. Actually, it might be worth
> adding a function to nsContentUtils for this, like
> nsContentUtils::HasNonEmptyAttr(nsIContent*, PRInt32, nsIAtom*)

Yes, I'll do that.

> >Index: layout/xul/base/src/nsBoxFrame.cpp
> > nsBoxFrame::GetInitialDebug(PRBool& aDebug)
> > {
> >-  nsAutoString value;
> >-
> >   nsCOMPtr<nsIContent> content;
> >   GetContentOf(getter_AddRefs(content));
> > 
> >   if (!content)
> >     return PR_FALSE;
> > 
> >-  content->GetAttr(kNameSpaceID_None, nsXULAtoms::debug, value);
> >-  if (value.EqualsLiteral("true")) {
> >-      aDebug = PR_TRUE;
> >-      return PR_TRUE;
> >-  } else if (value.EqualsLiteral("false")) {
> >-      aDebug = PR_FALSE;
> >-      return PR_TRUE;
> >+  static nsIAtom* const* const strings[] =
> >+    {&nsXULAtoms::_false, &nsXULAtoms::_true, NULL};
> >+  PRInt32 index = content->FindAttrValueIn(kNameSpaceID_None,
> >+      nsXULAtoms::debug, strings, eCaseMatters);
> >+  if (index >= 0) {
> >+    aDebug = index;
> >+    return PR_TRUE;
> 
> ewww! Relying on the values of bools! I wouldn't mind:
> aDebug = index == 1;

All it really relies on is that 1 means 'true' and 0 means 'false'. We really rely on that all over the place. I'll do as you suggest, though.

> > nsBoxFrame::GetInitialDirection(PRBool& aIsNormal)
> ...
> >+  static nsIAtom* const* const strings[] =
> >+    {&nsXULAtoms::reverse, &nsXULAtoms::ltr, &nsXULAtoms::rtl, NULL};
> >+  PRInt32 index = content->FindAttrValueIn(kNameSpaceID_None, nsXULAtoms::dir,
> >+      strings, eCaseMatters);
> >+  if (index >= 0) {
> >+    PRPackedBool values[] = {!aIsNormal, PR_TRUE, PR_FALSE};
> 
> What does this compile to? I guess the same thing as setting three variables
> so it's not too bad.

I suspect it gives better code than doing ifs or a 3-way switch. It certainly looks nicer because the values line up with the atoms.

Comment 14

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

checked in.

Comment 15

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

You didn't make the change from comment 9 to
>@@ -1254,37 +1255,37 @@ nsEventStateManager::FireContextClick()

Dunno if it was intentional or not?

Comment 16

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

It was unintentional. However IMHO the readability gain is slight, in particular because the context has a bunch of other 'if' statements setting allowedToDispatch to PR_FALSE.