Bug 24539 – Unresolved CSSMutableStyleDeclaration

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Norbert Leser]

Symbian linker cannot resolve CSSMutableStyleDeclaration class without explicit include of CSSMutableStyleDeclaration.h

Proposed solution is to explicitly include the header file. See attached patch.

[Norbert Leser]

Created attachment 28515 [details]
Proposed fix for bug 24539

[Mark Rowe (bdash)]

Why is the forward-declaration of CSSMutableStyleDeclaration not sufficient? 

[Eric Seidel (no email)]

Comment on attachment 28515 [details]
Proposed fix for bug 24539

This looks fine.  Because CSSMutableStyleDeclaration is used in a RefPtr<CSSMutableStyleDeclaration> other ports must require this header too.  They're likely only not failing due to RemoveCSSPropertyCommand.halways being included after CSSMutableStyleDeclaration in the same file.

[Darin Adler]

(In reply to comment #3)
> This looks fine.  Because CSSMutableStyleDeclaration is used in a
> RefPtr<CSSMutableStyleDeclaration> other ports must require this header too. 
> They're likely only not failing due to RemoveCSSPropertyCommand.halways being
> included after CSSMutableStyleDeclaration in the same file.

Eric, I think that's wrong. Normally a forward declaration is sufficient even if the file does define a RefPtr, as long as the header doesn't contain any code that actually needs to manipulate the RefPtr. Are you sure this is a good change?

[David Levin]

Comment on attachment 28515 [details]
Proposed fix for bug 24539

I'm clearing the r+ flag to remove this from the commit queue due to Drain's comment.

It is waiting for a comment from Eric before it can be committed and he could add the r+ when he adds a comment.

[Eric Seidel (no email)]

(In reply to comment #4)
> Eric, I think that's wrong. Normally a forward declaration is sufficient even
> if the file does define a RefPtr, as long as the header doesn't contain any
> code that actually needs to manipulate the RefPtr. Are you sure this is a good
> change?

Again, this is one of those c++ questions where I'm gonna trust your (lengthy) c++ experience over my own... but in this case, I think the instantiation of PassRefPtr<CSSMutableStyleDeclaration> via:

6	    static PassRefPtr<RemoveCSSPropertyCommand> create(Document* document, PassRefPtr<CSSMutableStyleDeclaration> style, CSSPropertyID property)
37	    {
38	        return adoptRef(new RemoveCSSPropertyCommand(document, style, property));
39	    }

is going to require actually including CSSMutableStyleDeclaration.h so that PassRefPtr can see that CSSMutableStyleDeclaration actually has a ref() method, no?

I'm still confused by why the Symbian build would fail here and no other port would, but I don't see including this header as harmful, if PassRefPtr<CSSMutableStyleDeclaration> instantiation does really require the full class definition.

[Darin Adler]

(In reply to comment #6)

Yes, the create function does require the CSSMutableStyleDeclaration class definition. So either that function needs to be moved to the .cpp file and made non-inline, or the include needs to be added.

[Norbert Leser]

(In reply to comment #7)
> (In reply to comment #6)
> 
> Yes, the create function does require the CSSMutableStyleDeclaration class
> definition. So either that function needs to be moved to the .cpp file and made
> non-inline, or the include needs to be added.
> 

I will leave it up to you guys to decide. Either of the proposed solutions seems to work in our (symbian linker) case.

[Darin Adler]

http://trac.webkit.org/changeset/43523

[Eric Seidel (no email)]

I missed a couple files when landing:

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	LayoutTests/platform/mac/fast/forms/search-styled-expected.checksum
	A	LayoutTests/platform/mac/fast/forms/search-styled-expected.png
Committed r43904

[Eric Seidel (no email)]

That comment was meant for https://bugs.webkit.org/show_bug.cgi?id=25742.