273740 - GradientFrame should use HasAttr rather than HasAttribute

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Scooter Morris]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041019
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041019

The Gradient code currently use HasAttribute to determine if this element has a
particular attribute.  It must do this to determine if it needs to look for a
possible linked element that provides the attribute.  A better approach would be
to use HasAttr insted, which is more efficient (and called by HasAttribute in
the end, anywyas).


Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Scooter Morris]

Attachment: Patch to use HasAttr

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 168219 [details] [diff] [review]
Patch to use HasAttr

A few comments after scanning this. You might want to add:

#include "nsSVGAtoms.h"

for the SVG atoms. Also the following change is wrong.

>@@ -359,22 +357,21 @@ nsSVGGradientFrame::GetNextGradient(nsIS
>   } else {
>     *aNextGrad = nsnull;
>     return NS_ERROR_FAILURE;
>   }
> }
> 
> // Private (helper) methods
> PRBool 
>-nsSVGGradientFrame::checkURITarget(const nsAString& attrStr) {
>+nsSVGGradientFrame::checkURITarget(nsIAtom *attr) {
>   nsCOMPtr<nsIDOMSVGGradientElement> aGrad = do_QueryInterface(mContent);
> 
>   // Was the attribute explicitly set?
>-  PRBool attr;
>-  aGrad->HasAttribute(attrStr, &attr);
>+  mContent->HasAttr(kNameSpaceID_None, attr);
>   if (attr) {
>     // Yes, just return
>     return PR_FALSE;
>   }
>   return checkURITarget();
> }
> 
> PRBool

Just make this:

  if (mContent->HasAttribute(attrStr, &attr)) {
    // Yes, just return
    return PR_FALSE;
  }

You need the QI or a local PRBool. If you want to check that mContent
implements nsIDOMSVGGradientElement then you should be checking aGrad isn't
nsnull. Also maybe change 'attr' to 'aAttr'? If you want a proper review from
me just request it with the flags.

Comment 3

[Jonathan Watt [:jwatt]]

I meant you *don't* need the QI or local PRBool, of course.

Comment 4

[Jonathan Watt [:jwatt]]

*sigh* and make that:

  if (mContent->HasAttr(kNameSpaceID_None, aAttr)) {
    // Yes, just return
    return PR_FALSE;
  }

Comment 5

[Scooter Morris]

Attachment: Fixed patch to use HasAttr

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 169455 [details] [diff] [review]
Fixed patch to use HasAttr

r=jwatt, but remove aGrad from checkURITarget. You don't use it, or check if
the do_QI succeeded. I don't think it would be desirable to do that check there
anyway.

Comment 7

[Scooter Morris]

Attachment: HasAttr patch, removed unnecessary call to get gradient element

Nice catch, Jonathan.  Fixed.

Comment 8

[tor]

Checked in.

Checking in nsSVGGradientFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGGradientFrame.cpp,v  <-- 
nsSVGGradientFrame.cpp
new revision: 1.5; previous revision: 1.4
done