Bug 20561 – "isFoo()" methods on CSSValue and subclasses should all be const

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

CSSValue implements:

    virtual bool isValueList() { return false; }

and CSSValueList has both:

public:
    virtual bool isValueList() const { return true; } 
and
private:
    virtual bool isValueList() { return true; }

Much hilarity ensues.

[Darin Adler]

Making all be const or all be non-const would be equally good -- obviously a mix is bad!

I don't think it's all that useful to have const member functions on objects that are in tree-like structures like the DOM classes.

[Simon Fraser (smfr)]

Created attachment 23059 [details]
Patch, changelog

[Dave Hyatt]

Comment on attachment 23059 [details]
Patch, changelog

I don't think the derived class isValueList() should be private.  Put it where the old non-const one was instead?

r=me

[Dave Hyatt]

Unless it compiles, in which case never mind. :)

[Simon Fraser (smfr)]

Committed r35978
	M	WebCore/ChangeLog
	M	WebCore/css/FontValue.h
	M	WebCore/css/CSSValueList.h
	M	WebCore/css/CSSValue.h
	M	WebCore/css/CSSTimingFunctionValue.h
r35978 = 28bad1a29d4699bd6fdaf11e87b2361af4dd856a (trunk)

[Darin Adler]

(In reply to comment #3)
> I don't think the derived class isValueList() should be private.  Put it where
> the old non-const one was instead?

There's a reason to make it private. If you already have a CSSValueList and you call isValueList() it'd be great if you got a compile time error, so then you'd realize it was unnecessary to check.

That's why I usually like to have these functions private once they're in a class where it's a "dumb thing to ask". Overriding the base class still works fine, because overriding ignores public vs. private.

[Dave Hyatt]

Yeah I figured that out after commenting in the bug.   Thanks for confirming my suspicions.