114169 - Fix boolean attr checks to use hasAttribute

Status: RESOLVED WONTFIX

(SeaMonkey :: UI Design, defect)

Description

[Blake Ross]

Comment 1

[Blake Ross]

Attachment: patch

Comment 2

[Joe Hewitt (gone)]

Comment on attachment 60918 [details] [diff] [review]
patch

sr=hewitt

Comment 3

[Blake Ross]

Attachment: patch for flagrant c++ callers

Comment 4

[Blake Ross]

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?

Comment 5

[Johnny Stenback (:jst)]

- 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...

Comment 6

[Blake Ross]

- 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).

Comment 7

[Blake Ross]

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 ;)

Comment 8

[David Hyatt]

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.

Comment 9

[David Hyatt]

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.

Comment 10

[Vedran Miletic]

retargeting

Comment 11

[Blake Ross]

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.

Comment 12

[Alessio Marchetti]

*** Bug 287771 has been marked as a duplicate of this bug. ***

Comment 13

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

WONTFIX per comment 9.