Bug 20561

Summary: "isFoo()" methods on CSSValue and subclasses should all be const
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Simon Fraser (smfr) <simon.fraser>
Severity: Normal CC: darin, hyatt, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Description Flags
Patch, changelog hyatt: review+

Description Simon Fraser (smfr) 2008-08-28 12:03:11 PDT
CSSValue implements:

    virtual bool isValueList() { return false; }

and CSSValueList has both:

    virtual bool isValueList() const { return true; } 
    virtual bool isValueList() { return true; }

Much hilarity ensues.
Comment 1 Darin Adler 2008-08-28 12:11:46 PDT
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.
Comment 2 Simon Fraser (smfr) 2008-08-28 14:20:35 PDT
Created attachment 23059 [details]
Patch, changelog
Comment 3 Dave Hyatt 2008-08-28 14:26:53 PDT
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?

Comment 4 Dave Hyatt 2008-08-28 14:27:07 PDT
Unless it compiles, in which case never mind. :)

Comment 5 Simon Fraser (smfr) 2008-08-28 14:29:51 PDT
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)
Comment 6 Darin Adler 2008-08-28 14:45:16 PDT
(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.
Comment 7 Dave Hyatt 2008-08-28 15:29:39 PDT
Yeah I figured that out after commenting in the bug.   Thanks for confirming my suspicions.