311827 - Make GetAttr return a bool

Status: RESOLVED FIXED

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

Description

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

The current situation where GetAttr returns an nsresult containing one of three
possible values, all success values, is less then optimal.

First we for every getattr have to check if the returned string is empty or not,
which in most cases is then just ignored. Second, nsGenericHTMLElement behaves
differently from all other elements and never returns NO_VALUE, only NOT_THERE
or HAS_VALUE, so for a random element you can't rely on the returned value anyway.

And in most places in the code it is very unclear if the author really want to
differentiate between an attribute being set to "" or the attribute being
missing, or if that was just a misstake not realizing that there are three
possible returns.

So I propose that we make GetAttr return a bool indicating whether the attribute
was set or not. This makes it easy to differentiate between missing and
emptystring, and noone should waste time on NS_ENSURE_SUCCESS checks.

I'm currently going through the tree removing by searching for NS_CONTENT_ATTR_.
So I might miss a few places where we do 
rv = GetAttr(...
NS_ENSURE_SUCCESS(rv, rv)

However code like that should behave correctly since both PR_TRUE and PR_FALSE
will be treated as success values. My plan is to make a followup patch once the
first patch is in and I can use lxr to search for the remaining.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Second, nsGenericHTMLElement behaves differently from all other elements

That's fixed in bug 309054 fwiw -- it's just been waiting on review for a few
weeks now...

> And in most places in the code it is very unclear if the author really want

Agreed, and this is the hardest part of making this change.

Generally I would OK with this, I think...  I agree that it would make things
clearer for callers.

Comment 2

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

(In reply to comment #1)
> > And in most places in the code it is very unclear if the author really want
> Agreed, and this is the hardest part of making this change.

Yes, this is a pain. I'm trying to not make any changes except in the places
where I think the code is doing the wrong thing. Once this patch has landed it
should be clearer what the code does and people can fix the areas that are wrong.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Again, bug 309054 has a bunch of places that we've already looked at, I think...
 So you probably want to use that as a basis, not the current tree.

Comment 4

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

Attachment: Patch to fix

This should do it. I've merged in the patch from bug 309054, but this one
touches a whole lot more places.

The only thing I did differently was the comments about the basetag handling in
the sinks. Why would we want to ignore the attributes if they contain the empty
string? Seems like "" is a valid value both for base-target and base-href?

Comment 5

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

Comment on attachment 199195 [details] [diff] [review]
Patch to fix

Aaron: It would be great if you could look at the accessibility changes to this
patch too. It's basically the same idea as bug 309054, but it touches more
places.

Comment 6

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

Attachment: With -w

Same as previous but with -w. Should be a lot eaiser to see what's going on
since i've merged if-statements in a few places.

Comment 7

[tor]

The SVG specification says that for requiredFeatures, requiredExtensions, and
systemLanguage that "" is equivalent to false.

    http://www.w3.org/TR/SVG11/struct.html#RequiredFeaturesAttribute
    http://www.w3.org/TR/SVG11/struct.html#RequiredExtensionsAttribute
    http://www.w3.org/TR/SVG11/struct.html#SystemLanguageAttribute

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Seems like "" is a valid value both for base-target and base-href?

Might be OK for base target.  Base href is an absolute URI, so "" will just
throw when we NS_NewURI in ProcessBaseHref.  Bailing up front might be faster,
but maybe not...

I'll try to get to this this weekend.

Comment 9

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

Comment on attachment 199195 [details] [diff] [review]
Patch to fix

Actually, i think an r/sr is good enough here. It's a lot of changes, but they
are all really simple.

Comment 10

[Aaron Leventhal]

Comment on attachment 199195 [details] [diff] [review]
Patch to fix

> * @returns PR_TRUE if the attribute was set (even when set to empty string)
Nit: s/was/is -- if someone does a removeAttribute call after that attribute was set I hope it will return false

r=aaronlev on the accessible parts.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 199195 [details] [diff] [review]
Patch to fix

There are some things I request that you fix below that might be ok not being fixed on initial checkin, since both PR_FALSE and PR_TRUE do test NS_SUCCEEDED...  Feel free to fix them either in this patch or in a followup.

>Index: accessible/src/base/nsAccessibilityService.cpp
> PRBool nsAccessibilityService::GetRole(nsIContent *aContent,
>-  return NS_CONTENT_ATTR_HAS_VALUE ==
>-         aContent->GetAttr(kNameSpaceID_XHTML2_Unofficial,
>+  return aContent->GetAttr(kNameSpaceID_XHTML2_Unofficial,

This changes the behavior, does it not?  I guess aaronl OK'ed it, so I'll just not worry about it, or the other accessibility changes.  At least too much.  ;)

aaron, please note that a lot of these changes change behavior if an attribute ise set, but set to the empty string.  I'm assuming you're ok with these behavior changes...

>Index: accessible/src/base/nsAccessible.cpp
>@@ -1289,20 +1286,18 @@ nsresult nsAccessible::AppendFlatStringF
>-      else if (NS_CONTENT_ATTR_HAS_VALUE !=
>-               aContent->GetAttr(kNameSpaceID_None,
>-                                 nsAccessibilityAtoms::tooltiptext,
>-                                 textEquivalent)) {
>+      else if (aContent->GetAttr(kNameSpaceID_None,
>+                                 nsAccessibilityAtoms::tooltiptext, textEquivalent)) {

I believe you want a ! there.  And possibly a IsEmpty() check on textEquivalent?  So

  else if (!aContent->GetAttr( ... ) || textEquivalent.IsEmpty()) {
  }

> nsresult nsAccessible::GetTextFromRelationID(nsIAtom *aIDAttrib, nsString &aName)
>   nsAutoString id;
>-  if (NS_CONTENT_ATTR_HAS_VALUE !=
>-      content->GetAttr(kNameSpaceID_WAIProperties, aIDAttrib, id)) {
>+  if (!content->GetAttr(kNameSpaceID_WAIProperties, aIDAttrib, id)) {

Definitely add "|| id.IsEmpty()" here.  We don't want to be using empty ids for anything.

>Index: accessible/src/msaa/nsAccessNodeWrap.cpp

The caller at line 206 of this file should either check whether true was returned or just unconditionally assign.  The latter is what we do now, so just do that?

>Index: accessible/src/xul/nsXULTextAccessible.cpp

In this file, nsXULLinkAccessible::GetValue returns the retval of GetAttr, and it's a NS_IMETHODIMP.  Please fix it to just return NS_OK?

>Index: content/base/public/nsIContent.h
>+// e5417db2-fc59-4b43-8c9a-edc3173a8540

Don't add that, please.  It's just one more thing to maintain every time we change the IID... ;)

>Index: content/base/src/nsObjectLoadingContent.cpp
>@@ -739,21 +738,23 @@ nsObjectLoadingContent::ObjectURIChanged
>+    hasID = thisContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::classid,
>+                                 classid);

I think we want hasID == !classid.IsEmpty() here.  That is, I think we should ignore |classid=""| on objects and render them normally in that case.  That's what IE does, at least.

Please fix nsScriptLoader::IsScriptEventHandler.  I think we want something like:

  if (!contElement->GetAttr(kNameSpaceID_None, nsHTMLAtoms::_for, str)) {
     return PR_FALSE;
  }

there.  Similar for the _event attr.

>Index: content/events/src/nsXMLEventsManager.cpp
>@@ -102,33 +102,29 @@ PRBool nsXMLEventsListener::InitXMLEvent
>+    aContent->AttrValueIs(nameSpaceID, nsHTMLAtoms::phase,
>+                          NS_LITERAL_STRING("capture"), eCaseMatters);

I almost wonder whether we should have atoms for those string... (and in fact store the attr values as atoms, if we start atomizing short attr values in general, not just for XUL).  Later bug, I guess... ;)

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>@@ -1689,18 +1689,17 @@ nsGenericHTMLElement::SetAttr(PRInt32 aN
>-      rv = GetAttr(aNamespaceID, aAttribute, oldValue);
>-      modification = rv != NS_CONTENT_ATTR_NOT_THERE;
>+      modification = GetAttr(aNamespaceID, aAttribute, oldValue);

Argh....  This will conflict with my SetAttr patch....  :(  Fun merge times...

You need to fix the (new) GetAttr caller in nsHTMLAppletElement::StartAppletLoad.

>Index: content/html/content/src/nsHTMLButtonElement.cpp

Please fix nsHTMLButtonElement::GetDefaultValue.

Also, please file a followup bug to investigate whether a button with an empty name should really be trying to submit?  Same for other form controls (eg textarea, etc).

>Index: content/html/content/src/nsHTMLFormElement.cpp

Please fix nsHTMLFormElement::GetAction also.

Please fix nsHTMLDocument::MatchNameAttribute?  It's using NS_SUCCEEDED on the retval of GetAttr.

Please fix nsEventListenerManager::CompileEventHandlerInternal.

Please fix nsSVGElement::GetId.

Please fix nsSVGScriptElement::GetType.

>Index: content/xml/content/src/nsXMLElement.cpp
>+      if (AttrValueIs(kNameSpaceID_XLink, nsLayoutAtoms::actuate,
>+                      onloadString, eCaseMatters)) {

We might also want to move to an atom here, if we start storing short attr values as atoms for XML.

>@@ -247,19 +242,18 @@ nsXMLElement::MaybeTriggerAutoLink(nsIDo
>-        rv = nsGenericElement::GetAttr(kNameSpaceID_XLink, nsHTMLAtoms::href,
>-                                       value);
>-        if (rv == NS_CONTENT_ATTR_HAS_VALUE && !value.IsEmpty()) {
>+        GetAttr(kNameSpaceID_XLink, nsHTMLAtoms::href, value);
>+        if (!value.IsEmpty()) {

I think this code was wrong -- an empty href, if set, is perfectly valid, really...  I think this should just check |if (GetAttr(...))|

>Index: content/xul/content/src/nsXULElement.cpp
>@@ -2143,31 +2142,28 @@ nsXULElement::GetRangeList() const
> nsXULElement::GetResource(nsIRDFResource** aResource)
> {
>+    nsresult rv;

I believe that's unused with your changes.  Just remove it.

>Index: content/xul/document/src/nsXULDocument.cpp

Please fix nsXULDocument::MatchAttribute.

Please fix nsXULDocument::OverlayForwardReference::Resolve.

Please fix the first and third GetAttr calls in nsXULDocument::OverlayForwardReference::Merge.

Please fix nsXULDocument::BroadcasterHookup::~BroadcasterHookup.

Please fix the remaining GetAttr calls in nsXULDocument::FindBroadcaster.

>@@ -4245,20 +4217,18 @@ nsXULDocument::InsertElement(nsIContent*
>-        rv = aChild->GetAttr(kNameSpaceID_None, nsXULAtoms::position, posStr);
>-        if (NS_FAILED(rv)) return rv;
>-
>-        if (rv == NS_CONTENT_ATTR_HAS_VALUE) {
>+        if (aChild->GetAttr(kNameSpaceID_None, nsXULAtoms::position,
>+                            posStr)) {

This changes behavior; please check for posStr.IsEmpty()?

>Index: content/xul/templates/src/nsXULContentBuilder.cpp

Please remove the setting of rv in nsXULContentBuilder::SynchronizeUsingTemplate.

Please fix nsXULContentBuilder::IsOpen (probably use AttrValueIs with an "open" atom).

>Index: content/xul/templates/src/nsXULSortService.cpp
>@@ -383,97 +381,69 @@ XULSortServiceImpl::SetSortColumnHints(n

That method should really become void, but that can happen in a separate patch.

Please fix the GetAttr() calls in the "if (sortInfo.db && sortInfo.naturalOrderSort) {" block in XULSortServiceImpl::InsertContainerNode.

>@@ -1578,34 +1538,27 @@ XULSortServiceImpl::Sort(nsIDOMNode* nod
>+      (sortDirection.Equals(*kAscendingStr) &&
>+       dbNode->AttrValueIs(kNameSpaceID_None, nsXULAtoms::sortDirection,
>+                           *kDescendingStr, eCaseMatters) ||
>+       sortDirection.Equals(*kDescendingStr) &&
>+       dbNode->AttrValueIs(kNameSpaceID_None, nsXULAtoms::sortDirection,
>+                    *kAscendingStr, eCaseMatters))) {

I think I prefer |((foo && bar) || (baz && whatever))| with the extra parens.  Remembering that && binds tighter than || is a pain, really.  ;)

>Index: content/xul/templates/src/nsXULTemplateBuilder.cpp

Please fix nsXULTemplateBuilder::ComputeContainmentProperties.

Please fix the first GetAttr call in nsXULTemplateBuilder::CompileSimpleRule.

>Index: extensions/xforms/nsXFormsUtils.cpp
>+      if(content->AttrValueIs(kNameSpaceID_None, content->GetIDAttributeName(),

Space before '(', please.

>Index: layout/base/nsCSSFrameConstructor.cpp
>@@ -7226,47 +7224,42 @@ nsCSSFrameConstructor::TestSVGConditions
>   aHasSystemLanguage = PR_TRUE;
>-  rv = aContent->GetAttr(kNameSpaceID_None, nsSVGAtoms::systemLanguage, value);
>-  if (NS_CONTENT_ATTR_HAS_VALUE == rv) {
>+  if (aContent->GetAttr(kNameSpaceID_None, nsSVGAtoms::systemLanguage,
>+                        value)) {

This is actually wrong.  We should set aHasSystemLanguage to !aContent->GetAttr() to start with, or something, then do this stuff.  That is, the right behavior is:

1)  No attr set means aHasSystemLanguage == PR_TRUE
2)  Attr set to empty string means aHasSystemLanguage == PR_FALSE
3)  Attr set to nonempty string means compare to our pref.

>Index: layout/generic/nsObjectFrame.cpp
> nsObjectFrame::GetMIMEType(nsACString& aType)

This method is gone on trunk.  ;)

>@@ -1856,17 +1854,17 @@ nsObjectFrame::Instantiate(const char* a
>   if (mContent->Tag() == nsHTMLAtoms::object &&
>-      NS_CONTENT_ATTR_HAS_VALUE == mContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::classid, classid)) {
>+      mContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::classid, classid)) {

Just make sure this check ends up matching the check in nsObjectLoadingContent::ObjectURIChanged, ok?

>Index: layout/xul/base/src/nsBoxFrame.cpp

>@@ -628,22 +625,21 @@ nsBoxFrame::GetInitialEqualSize(PRBool& 
>+  if (content->AttrValueIs(kNameSpaceID_None, nsXULAtoms::equalsize,
>+                           NS_LITERAL_STRING("always"), eCaseMatters)) {

We should have an nsXULAtom for that and use it here.

>Index: layout/xul/base/src/nsGrippyFrame.cpp
>@@ -129,23 +128,19 @@ nsGrippyFrame::MouseClicked(nsPresContex
>+    PRBool before =
>+      !content->AttrValueIs(kNameSpaceID_None, nsXULAtoms::collapse,
>+                            NS_LITERAL_STRING("after"), eCaseMatters);

Again, there should be an atom for that.

Please fix nsComposeTxtSrvFilter::Skip (in editor).

>Index: widget/src/xpwidgets/nsNativeTheme.cpp
>@@ -130,24 +130,21 @@ nsNativeTheme::CheckBooleanAttr(nsIFrame
>+  return content->AttrValueIs(kNameSpaceID_None, aAtom,
>+                              NS_LITERAL_STRING("true"), eCaseMatters);

This might be worth making an atom.

r+sr=bzbarsky with those fixed or followups filed appropriately.

Comment 12

[Aaron Leventhal]

Boris, I looked at what's being changed in the accessibility module and have no problem with it. In some cases it really makes no difference and in others it's arguably better now.

Comment 13

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

(In reply to comment #11)
> There are some things I request that you fix below that might be ok not being
> fixed on initial checkin, since both PR_FALSE and PR_TRUE do test
> NS_SUCCEEDED...  Feel free to fix them either in this patch or in a followup.
> 

> I believe you want a ! there.  And possibly a IsEmpty() check on
> textEquivalent?  So
> 
>   else if (!aContent->GetAttr( ... ) || textEquivalent.IsEmpty()) {
>   }

Good catch! I'm not sure if the IsEmpty check should be there or not, but i'll put it back. Aaron, let me know if you don't want the IsEmpty check there.

> Definitely add "|| id.IsEmpty()" here.  We don't want to be using empty ids for
> anything.

Actually, if this attribute is empty the code returns NS_OK, and i'd rather not change that. Note that this isn't the the id attribute that is being retrived, but an attribute that contains an id.

> >+// e5417db2-fc59-4b43-8c9a-edc3173a8540
> Don't add that, please.  It's just one more thing to maintain every time we
> change the IID... ;)

Actually, i sort of like this since it allows you to search the tree for an IID. I'll give in this time though :)

> I almost wonder whether we should have atoms for those string... (and in fact
> store the attr values as atoms, if we start atomizing short attr values in
> general, not just for XUL).  Later bug, I guess... ;)

I changed to using an attribute rather then a string. But making us store atoms for short values is a different bug, though something we should do.

> Also, please file a followup bug to investigate whether a button with an empty
> name should really be trying to submit?  Same for other form controls (eg
> textarea, etc).

Filed bug 314117.

> >@@ -4245,20 +4217,18 @@ nsXULDocument::InsertElement(nsIContent*
> This changes behavior; please check for posStr.IsEmpty()?

Actually, the other way amounts to the same thing, but an IsEmpty check is slightly faster.

> Please fix nsXULContentBuilder::IsOpen (probably use AttrValueIs with an
> "open" atom).

I already had :)

> I think I prefer |((foo && bar) || (baz && whatever))| with the extra parens. 
> Remembering that && binds tighter than || is a pain, really.  ;)

bah! Boolean algerbra is only logical that way ;)

> >Index: layout/xul/base/src/nsGrippyFrame.cpp
> >@@ -129,23 +128,19 @@ nsGrippyFrame::MouseClicked(nsPresContex
> >+    PRBool before =
> >+      !content->AttrValueIs(kNameSpaceID_None, nsXULAtoms::collapse,
> >+                            NS_LITERAL_STRING("after"), eCaseMatters);
> 
> Again, there should be an atom for that.

I didn't do this one since this code is commented out.

> >Index: widget/src/xpwidgets/nsNativeTheme.cpp
> >@@ -130,24 +130,21 @@ nsNativeTheme::CheckBooleanAttr(nsIFrame
> >+  return content->AttrValueIs(kNameSpaceID_None, aAtom,
> >+                              NS_LITERAL_STRING("true"), eCaseMatters);
> 
> This might be worth making an atom.

There is no nsFooAtoms class around here so i'll let this be as is.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Just to clarify, some of those "this should be an atom" are fodder for followup bugs, imo.

Comment 15

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

Too late :)

It's something we should do eventually anyway so i just went with it.

Comment 16

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

Checked in. I'll attach the patch that was checked in when I get home.

Once lxr is updated i'll look through the tree to make sure all callers of GetAttr have been fixed and file a new bug if needed.

Comment 17

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

Attachment: Patch that was checked in

Patch that was checked in.

I also looked over all GetAttr callers and it looks like they all should be fine.

Comment 18

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

Attachment: Minor fix

I found one remaining place where the returnvalue was treated as an nsresult (though never used).

Comment 19

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

checked in followup patch.