Closed Bug 114169 Opened 21 years ago Closed 17 years ago
Fix boolean attr checks to use has
Comment on attachment 60918 [details] [diff] [review] patch sr=hewitt
Attachment #60918 - Flags: superreview+
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: 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 → ---
*** Bug 287771 has been marked as a duplicate of this bug. ***
WONTFIX per comment 9.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.