Closed Bug 114169 Opened 21 years ago Closed 17 years ago

Fix boolean attr checks to use hasAttribute

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: bugzilla, Unassigned)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

 
Keywords: perf
Attached patch patch (obsolete) — Splinter Review
Comment on attachment 60918 [details] [diff] [review]
patch

sr=hewitt
Attachment #60918 - Flags: superreview+
QA Contact: sairuh → jrgm
Attachment #60918 - Attachment is obsolete: true
The logic in nsHTMLButtonElement::GetAttribute seems strange.  Instead of 
actually returning the attribute of the button, it checks for its existence and 
then returns "true" or "false".  I guess this forced validation is for a reason 
so I left it alone.  Can I get some reviews on this?
- The nsCSSFrameConstructor.cpp changes seem unrelated to this bug, no?

- In nsBoxFrame.cpp, before the changes aEqualSize was only true if the
attribute had the value "always", now it'll be true as long as there's an
"equalsize" attribute on the element, indentionally so?

- Likewisr in nsMenuFrame.cpp, |sizetopopup="false"| will now be equal to
|sizetopopup="true"|, seems wrong to me.

... and nsOutlinerBodyFrame.cpp

- In nsOutlinerContentView.cpp, are you sure HasAttr + GetAttr() is faster than
simply GetAttr() even if you can hide a nsAutoString constructor inside the
HasAttr() check?

That's where I stopped, doesn't look too good to me yet...
- Yes, the css frame constructor change is unrelated, ignore that.

- No, the changes that make foo="false" equal to foo="true" are correct.  The 
xul attribute model has always been that the existence of the attribute is true 
and the lack of it is false, but now we're beginning to enforce it.  Some of it 
has already been done (see last patch here), the rest will come soon, but I can 
already change these attributes over as they're only used in a few places, and 
I know they're used correctly.

- Yes, I'm pretty sure it will be faster; dbaron also commented so in the other 
bug.  In this case I'm especially sure because GetCellProperties is called 
frequently (e.g. when painting an outliner cell).
Well, great.  hewitt says at a late night denny's run, hyatt insisted that we 
explicitly check for true. I didn't realize they sold booze at denny's.  at 
least a year and a half ago, he agreed on (no, explained to me) this model ;)
I have never believed in this model.  I do believe that as much as possible the
absence of the attribute should imply that the value is false, but I've always
maintained that boolean attributes can be explicitly set to false.  

This is necessary in order to be consistent with other XML grammars and for
schema validation, where attributes are given types that associate them with a
range of values.  An attribute like "disabled" in the XUL schema is a boolean
attribute, which means it automatically has as its two acceptable values "true"
and "false".  Schemas also have the concept of the default value that should be
chosen when the attribute is absent, and that's the model we're following.  We
strive to pick "false" as the default boolean value for the attribute when it is
absent, but a XUL author should be allowed to specify this default explicitly.

With this change we'd essentially be saying "the value of this attribute doesn't
matter", and therefore it wouldn't be a boolean type any more.  I don't want to
go down that route, especially since performance would be just fine if we
implemented a hasAttrWithValue method.
In other words, for a given attribute, 

(1) disabled="true" means TRUE
(2) disabled="false" means FALSE
(3) disabled="XXX" is undefined, but we typically choose FALSE, because it's
easiest to just check for TRUE and assume FALSE otherwise
(4) absence of a value means choose the default value for that attribute, which
is what you've heard me go on about.  For XUL attributes we should strive to
make this FALSE always.

For boolean getters/setters in XBL, I also advocate removing attributes rather
than setting them to false, as well as doing that in C++ and JS code.  Setting
an attribute to false explicitly is not wrong, just less performant (since you
could save an attribute in the content model), and you can still patch all such
uses in our XUL.

Just don't change the tests for true to check only for the presence of the attr,
since you'll miss the explicit false value if it's there.

Target Milestone: --- → mozilla0.9.9
Target Milestone: mozilla0.9.9 → mozilla1.0.1
retargeting
Target Milestone: mozilla1.0.1 → Future
Reassigning obsolete bugs to their respective Seamonkey owners (i.e. nobody). 
If you want this fixed for Firefox, change the Product and Component accordingly
and reassign back to me.
Assignee: firefox → guifeatures
Target Milestone: Future → ---
Product: Core → Mozilla Application Suite
*** Bug 287771 has been marked as a duplicate of this bug. ***
WONTFIX per comment 9.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WONTFIX
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.