Summary: | Unresolved CSSMutableStyleDeclaration | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Norbert Leser <norbert.leser> | ||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin, hausmann, laszlo.gombos, mrowe | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | S60 Hardware | ||||||
OS: | S60 3rd edition | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 27065 | ||||||
Attachments: |
|
Description
Norbert Leser
2009-03-11 19:32:17 PDT
Created attachment 28515 [details] Proposed fix for bug 24539 Why is the forward-declaration of CSSMutableStyleDeclaration not sufficient? 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. (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 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. (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. (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. (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. 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 That comment was meant for https://bugs.webkit.org/show_bug.cgi?id=25742. |