Bug 24539 - Unresolved CSSMutableStyleDeclaration
Summary: Unresolved CSSMutableStyleDeclaration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-03-11 19:32 PDT by Norbert Leser
Modified: 2009-07-13 11:35 PDT (History)
4 users (show)

See Also:


Attachments
Proposed fix for bug 24539 (989 bytes, patch)
2009-03-11 19:33 PDT, Norbert Leser
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Norbert Leser 2009-03-11 19:32:17 PDT
Symbian linker cannot resolve CSSMutableStyleDeclaration class without explicit include of CSSMutableStyleDeclaration.h

Proposed solution is to explicitly include the header file. See attached patch.
Comment 1 Norbert Leser 2009-03-11 19:33:04 PDT
Created attachment 28515 [details]
Proposed fix for bug 24539
Comment 2 Mark Rowe (bdash) 2009-03-11 19:39:03 PDT
Why is the forward-declaration of CSSMutableStyleDeclaration not sufficient? 
Comment 3 Eric Seidel (no email) 2009-03-26 11:05:39 PDT
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.
Comment 4 Darin Adler 2009-03-26 12:43:00 PDT
(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?
Comment 5 David Levin 2009-04-08 16:33:54 PDT
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.
Comment 6 Eric Seidel (no email) 2009-04-09 01:46:15 PDT
(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.
Comment 7 Darin Adler 2009-04-09 08:38:36 PDT
(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.
Comment 8 Norbert Leser 2009-04-14 16:01:49 PDT
(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.
Comment 9 Darin Adler 2009-05-11 16:54:50 PDT
http://trac.webkit.org/changeset/43523
Comment 10 Eric Seidel (no email) 2009-05-20 05:37:49 PDT
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

Comment 11 Eric Seidel (no email) 2009-05-20 05:38:41 PDT
That comment was meant for https://bugs.webkit.org/show_bug.cgi?id=25742.