Bug 20561 - "isFoo()" methods on CSSValue and subclasses should all be const
Summary: "isFoo()" methods on CSSValue and subclasses should all be const
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-08-28 12:03 PDT by Simon Fraser (smfr)
Modified: 2008-08-28 15:29 PDT (History)
3 users (show)

See Also:


Attachments
Patch, changelog (3.15 KB, patch)
2008-08-28 14:20 PDT, Simon Fraser (smfr)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2008-08-28 12:03:11 PDT
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.
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?

r=me
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.