113580 - switch uses of getAttribute to hasAttributeValue whenever possible

Status: RESOLVED FIXED

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

Description

[Joe Hewitt (gone)]

Since I am adding HasAttr and HasAttrValue to nsIContent and hasAttributeValue
to nsIDOMXULElement (bug 113562) I should scan through as many c++/js/xbl files
as I can while keeping my sanity and fix their usage.  This should save on a ton
of wasteful string copying.

Comment 1

[Blake Ross]

-> me

Comment 2

[Vidar Haarr (not reading bugmail)]

Attachment: optimizes a bit by doing HasAttr before GetAttr

I just really need some general comments here. Am I way off with what I've done
here, or am I on the right track?

Comment 3

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

Comment on attachment 169127 [details] [diff] [review]
optimizes a bit by doing HasAttr before GetAttr

The nsHTMLContentSerializer change looks good.

The nsPlainTextSerializer change changes the code in a way that looks incorrect
(the if has an else, so you can't just add to it with &&).

All but the nsHTMLContentSerializer changes seem like premature optimization
(that could even hurt if the attribute is common).  I don't think it's worth
doing HasAttr before GetAttr except maybe in very performance-sensitive cases
where the attribute is unlikely to be present.	It is, however, worthwhile when
the caller doesn't care about the attribute's value (e.g., the
nsHTMLContentSerializer case).

Comment 4

[Vidar Haarr (not reading bugmail)]

(In reply to comment #3)
> The nsPlainTextSerializer change changes the code in a way that looks incorrect
> (the if has an else, so you can't just add to it with &&).

Yes, I thought so at first, too. But the if..else if.. doesn't end in an else,
it ends with else if (aNode) and returns at the bottom.

> All but the nsHTMLContentSerializer changes seem like premature optimization
> (that could even hurt if the attribute is common).  I don't think it's worth
> doing HasAttr before GetAttr except maybe in very performance-sensitive cases
> where the attribute is unlikely to be present.

I understand, but I don't see the actual harm in doing it.
In nsObjectFrame.cpp, I just move the check out one level. And in
nsTableCellFrame.cpp, we save one string allocation for every table.

Keep in mind that I'm not exactly an experienced C coder, so my above statements
can very well be wrong - in that case, I'd very much appreciate an explanation
as to why they are. Thanks!

Comment 5

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

(In reply to comment #4)
> (In reply to comment #3)
> > The nsPlainTextSerializer change changes the code in a way that looks
> > incorrect (the if has an else, so you can't just add to it with &&).
> 
> Yes, I thought so at first, too. But the if..else if.. doesn't end in an else,
> it ends with else if (aNode) and returns at the bottom.

You're changing what happens if mContent is non-null, aNode is non-null, and
mContent does *not* have the attribute.

> In nsObjectFrame.cpp, I just move the check out one level. And in

And cause us to go through the list of attributes twice just to save a string
copy, as well as increasing codesize for what's probably no measurable
performance gain.

> nsTableCellFrame.cpp, we save one string allocation for every table.

Constructing and destroying an unused nsAutoString should be cheap, and there's
no allocation involved.

Comment 6

[Vidar Haarr (not reading bugmail)]

(In reply to comment #5)
> You're changing what happens if mContent is non-null, aNode is non-null, and
> mContent does *not* have the attribute.

Please bare with me in discussing this, I'm just trying to understand the code
(even if the change doesn't really provide any optimization), not be a pain.
<http://lxr.mozilla.org/seamonkey/source/content/base/src/nsPlainTextSerializer.cpp#1810>

If mContent is not null, it will never check for aNode. If mContent is null, it
will check aNode, and if aNode is null, it will return NS_ERROR_NOT_AVAILABLE.
Then, if the attribute is there, it will set the value, and return NS_OK.
If the attribute is not there, it will skip the aNode check, and return
NS_ERROR_NOT_AVAILABLE.

Is that correct? If not, which part of the statements are false?
I can't see that the patch has changed the logic :/

> > In nsObjectFrame.cpp, I just move the check out one level. And in
> 
> And cause us to go through the list of attributes twice just to save a string
> copy, as well as increasing codesize for what's probably no measurable
> performance gain.

Ah, yes, true. Thanks.

> > nsTableCellFrame.cpp, we save one string allocation for every table.
> 
> Constructing and destroying an unused nsAutoString should be cheap, and there's
> no allocation involved.

Ah, so GetAttr doesn't allocate anything if the attribute doesn't exist? I can't
actually seem to find the implementation of GetAttr, so I couldn't check ;)

Thanks!

Comment 7

[Blake Ross]

Vidur,

Your patch changes the logic because if mContent is non-NULL but it doesn't have
the attribute in question, the else case will be evaluated. In the current code,
if mContent is non-NULL then the else case will never be evaluated even if the
attribute doesn't exist. The behavior is different.

Also, GetAttr won't do anything with the string if the attribute doesn't exist,
it will just return NS_CONTENT_ATTR_NOT_THERE.

I agree with David that this is only useful the case where the caller doesn't
care about the attribute, as in nsHTMLContentSerializer. David said everything
else was premature optimization, but it looks as if nsTableCellFrame doesn't
care about the attr value either--correct me if I'm wrong.

Vidar, could you create a new patch fixing only those call sites? Thanks.

Comment 8

[Vidar Haarr (not reading bugmail)]

(In reply to comment #7)
> Your patch changes the logic because if mContent is non-NULL but it doesn't have
> the attribute in question, the else case will be evaluated. In the current code,
> if mContent is non-NULL then the else case will never be evaluated even if the
> attribute doesn't exist. The behavior is different.

I can't believe I didn't see that. Thanks alot!

> Also, GetAttr won't do anything with the string if the attribute doesn't exist,
> it will just return NS_CONTENT_ATTR_NOT_THERE.

Ah! Good. Anything else would've been silly, I suppose, but I couldn't find the
implementation (GetAttr is referenced too many places), so I couldn't check.

> I agree with David that this is only useful the case where the caller doesn't
> care about the attribute, as in nsHTMLContentSerializer. David said everything
> else was premature optimization, but it looks as if nsTableCellFrame doesn't
> care about the attr value either--correct me if I'm wrong.

It seems you are correct - nsTableCellFrame doesn't even need to call GetAttr,
so I'll just remove that.

> Vidar, could you create a new patch fixing only those call sites? Thanks.

I'll do that tonight.

Comment 9

[Vidar Haarr (not reading bugmail)]

Attachment: updated to comments on changed behavior

I removed the changed behavior. The patch only touches nsHTMLContentSerializer
and nsTableCellFrame now. I removed the attribute getting from nsTableCellFrame
all together, since it didn't actually use the returned value for anything.

Now that I know what to look for and how to proceed, I think I'll look for more
occurances of GetAttr that can be changed. Sounds good?

Comment 10

[Vidar Haarr (not reading bugmail)]

Comment on attachment 169237 [details] [diff] [review]
updated to comments on changed behavior

Do you want to check this in, or should I patch a few more locations first?

Comment 11

[Vidar Haarr (not reading bugmail)]

Comment on attachment 169237 [details] [diff] [review]
updated to comments on changed behavior

bz: can you sr= this?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 169237 [details] [diff] [review]
updated to comments on changed behavior

sr=bzbarsky

Comment 13

[Ian Neal]

Comment on attachment 169237 [details] [diff] [review]
updated to comments on changed behavior

Checking in content/base/src/nsHTMLContentSerializer.cpp;
/cvsroot/mozilla/content/base/src/nsHTMLContentSerializer.cpp,v  <-- 
nsHTMLContentSerializer.cpp
new revision: 1.92; previous revision: 1.91
done
Checking in content/base/src/nsHTMLContentSerializer.h;
/cvsroot/mozilla/content/base/src/nsHTMLContentSerializer.h,v  <-- 
nsHTMLContentSerializer.h
new revision: 1.25; previous revision: 1.24
done
Checking in layout/tables/nsTableCellFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableCellFrame.cpp,v  <-- 
nsTableCellFrame.cpp
new revision: 3.354; previous revision: 3.353
done

Comment 14

[Vidar Haarr (not reading bugmail)]

Thanks, Ian!

I'll start searching for places where this would help, and make a new patch when
I've found some. If anyone knows any good places to look, please feel free to
patch or direct my attention :)

Blake: since you actually changed this bug to ASSIGNED once I submitted the
first patch, does that mean you'll be working on this?

Comment 15

[Boris Zbarsky [:bzbarsky]]

I suspect blake was trying to reassign...

Comment 16

[David Gardiner]

I think
http://lxr.mozilla.org/seamonkey/source/layout/forms/nsTextControlFrame.cpp#2004
might be another candidate?

-dave

Comment 17

[Vidar Haarr (not reading bugmail)]

Yes, thanks. I got a rather lengthy list from gandalf as well, which I'll
address soon (tm).

Comment 18

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #17)
> Yes, thanks. I got a rather lengthy list from gandalf as well, which I'll
> address soon (tm).

Soon indeed. This bug hasn't be touched in four years, and I'm curious as to whether it's still a valid concern. The reporter is "gone" and the assignee is "not reading bugmail".

I'm marking this as "closeme" for a month from now. Surely that should be plenty of time for someone to analyze whether this is still an issue. (If it is, simply remove the tag and be on your merry way.)

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to comment #18)
> I'm marking this as "closeme" for a month from now. Surely that should be
> plenty of time for someone to analyze whether this is still an issue. (If it
> is, simply remove the tag and be on your merry way.)

In fact it is still an issue and we still have a lot of places in our code where we misuse those two.

Places like this:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js#992

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/update.js#434

It's pity I don't remember what list did I send to Vidar four years ago, since I'm pretty sure I came up with a good regexp pattern there :)

Comment 20

[Luyun Xie]

Attachment: replace getAttribute with hasAttribute when necessary

I did a little search in mozilla-central and made a few changes. They are all .js files because it seems that this bug had been eliminated already from .cpp files. This is my first attempt to submit a patch to firefox and I'm glad I made the first step. Please let me know if anything is inappropriate.

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 379195 [details] [diff] [review]
replace getAttribute with hasAttribute when necessary

Great!

I'm happy to review this if no one else plans to.

Comment 22

[Arpad Borsos [:Swatinem]]

I'm also glad you made the first step ;)
Try searching for uppercase GetAttribute. Scriptable methods are always uppercase in C++ and also use outparameters.
See the list in the URL field.

This here: http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#1198
> rv = windowElement->GetAttribute(NS_LITERAL_STRING("sizemode"), stateString);
...
>  if (stateString.Equals(SIZEMODE_MAXIMIZED)) {

looks like a very good candidate to rewrite to HasAttributeValue.

Maybe you can produce a second patch for the C++ cases?

Comment 23

[Luyun Xie]

I was searching for GetAttr in .cpp files. I read through the patch for bug 113562 and found nothing about HasAttributeValue method.(although Joe did mentioned this method) HasAttrValue is not implemented according to bug 114538, I wonder if HasAttributeValue has been implemented yet. I found nothing about "HasAttrValue" or "HasAttributeValue" with mxr.

But I guess some "GetAttribute" could be replaced by "HasAttribute". I'll work on that.

Comment 24

[Luyun Xie]

I ran through a long list of "GetAttribute" occurrence in mozilla-central and found nothing irregular. Maybe I missed something cause that's really long and I excluded some most unlikely entries. I guess the patch I attached previously is all we need.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: switch uses of getAttribute to hasAttribute whenever possible

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 500502 [details] [diff] [review]
switch uses of getAttribute to hasAttribute whenever possible

It turns out only the microformats code is dead enough not to rot.

Comment 27

[Shawn Wilsher :sdwilsh]

Comment on attachment 500502 [details] [diff] [review]
switch uses of getAttribute to hasAttribute whenever possible

r=sdwilsh

Comment 28

[Shawn Wilsher :sdwilsh]

Comment on attachment 500502 [details] [diff] [review]
switch uses of getAttribute to hasAttribute whenever possible

Review of attachment 500502 [details] [diff] [review]:

::: browser/base/content/browser.js
@@ +5120,5 @@
     let target = linkNode.target;
     let mainTarget = !target || target == "_content" || target  == "_main";
     if (isPanelClick && mainTarget) {
       // javascript and data links should be executed in the current browser.
+      if (linkNode.hasAttribute("onclick") ||

We don't actually want to return true here if onclick is "" though, so don't do this change.

::: browser/base/content/web-panels.js
@@ +107,5 @@
 {
     var panelBrowser = getPanelBrowser();
     panelBrowser.webProgress.addProgressListener(panelProgressListener,
                                                  Ci.nsIWebProgress.NOTIFY_ALL);
+    if (panelBrowser.hasAttribute("cachedurl")) {

same here; if it's "" we don't actually want to run the next code.  You can, however, store the value in a local and then use that local variable in both places here.

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Will do, thanks.

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/de9c234bb392

I had to revert the nsBrowserGlue.js change, because it caused mochitest-other failures.

More changes of this kind can go in followup bugs.

Comment 31

[[e-mail]]

(In reply to comment #30)
> I had to revert the nsBrowserGlue.js change, because it caused
> mochitest-other failures.
It's possible for the chromehidden attribute to exist but be blank, which should be treated the same as if it is not present.

Comment 32

[Philip Chee]

> It turns out only the microformats code is dead enough not to rot.
Hmm. I thought that the microformats code was due to be removed from the tree Real Soon Now?

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
> Hmm. I thought that the microformats code was due to be removed from the tree
> Real Soon Now?
I think you are confusing that with microsummaries.

Comment 34

[Philip Chee]

> I think you are confusing that with microsummaries.
They aren't the same? I guess I'm really confused now.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

(In reply to [e-mail] from comment #31)
> (In reply to comment #30)
> > I had to revert the nsBrowserGlue.js change, because it caused
> > mochitest-other failures.
> It's possible for the chromehidden attribute to exist but be blank, which
> should be treated the same as if it is not present.

(and so the other chromehidden change in this patch was also wrong, see bug 784028)