Bug 109926 - Fix build when css3-conditional-rules feature flag is enabled
Summary: Fix build when css3-conditional-rules feature flag is enabled
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 92868
  Show dependency treegraph
 
Reported: 2013-02-15 04:56 PST by Lamarque V. Souza
Modified: 2013-02-15 18:51 PST (History)
12 users (show)

See Also:


Attachments
Patch (1.36 KB, patch)
2013-02-15 05:04 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (EWS only version) (15.82 KB, patch)
2013-02-15 05:07 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (1.43 KB, patch)
2013-02-15 05:10 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (1.58 KB, patch)
2013-02-15 08:02 PST, Lamarque V. Souza
abarth: review-
Details | Formatted Diff | Diff
Patch (1.63 KB, patch)
2013-02-15 15:36 PST, Lamarque V. Souza
abarth: review-
Details | Formatted Diff | Diff
Patch (1.60 KB, patch)
2013-02-15 15:56 PST, Lamarque V. Souza
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lamarque V. Souza 2013-02-15 04:56:09 PST
WebKit does not compile after commit 05ccd1f76464bc0adc15d7fc256078bd497271c5 landed and if css3-conditional-rules is enabled. That commit added file css/DOMWindowCSS.h, which miss a virtual destructor. I will a patch to fix the problem.
Comment 1 Lamarque V. Souza 2013-02-15 05:04:32 PST
Created attachment 188539 [details]
Patch

Proposed patch
Comment 2 Lamarque V. Souza 2013-02-15 05:07:50 PST
Created attachment 188541 [details]
Patch (EWS only version)

Path with css3-conditional-rules enabled
Comment 3 Lamarque V. Souza 2013-02-15 05:10:42 PST
Created attachment 188544 [details]
Patch

Update ChangeLog
Comment 4 Thiago Marcos P. Santos 2013-02-15 06:15:23 PST
Comment on attachment 188544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188544&action=review

> Source/WebCore/ChangeLog:9
> +        05ccd1f76464bc0adc15d7fc256078bd497271c5 if css3-conditional-rules is

Use SVN revision (i.e. r12345) instead of git hash.

> Source/WebCore/css/DOMWindowCSS.h:43
> +    virtual ~DOMWindowCSS() { }

I don't see why the default destructor is not enough. AFAIK no other class is inheriting from this one.
Comment 5 Lamarque V. Souza 2013-02-15 07:50:12 PST
(In reply to comment #4)
> (From update of attachment 188544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188544&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        05ccd1f76464bc0adc15d7fc256078bd497271c5 if css3-conditional-rules is
> 
> Use SVN revision (i.e. r12345) instead of git hash.

Ok, I will update the patch.
 
> > Source/WebCore/css/DOMWindowCSS.h:43
> > +    virtual ~DOMWindowCSS() { }
> 
> I don't see why the default destructor is not enough. AFAIK no other class is inheriting from this one.

The problem is that Source/WebCore/bindings/scripts/CodeGeneratorV8.pm generates code that tries to access DOMWindowCSS's vtable, which does not seem to be created unless there is at least one virtual method in the class.
Comment 6 Lamarque V. Souza 2013-02-15 08:02:21 PST
Created attachment 188574 [details]
Patch

Update ChangeLog with more additional info about the problem.
Comment 7 Ryosuke Niwa 2013-02-15 14:58:58 PST
Comment on attachment 188574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188574&action=review

> Source/WebCore/css/DOMWindowCSS.h:44
> +    virtual ~DOMWindowCSS() { }
> +

Please wrap this in if-def v8 or chromium.
Comment 8 Adam Barth 2013-02-15 14:59:54 PST
Comment on attachment 188574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188574&action=review

>> Source/WebCore/css/DOMWindowCSS.h:44
>> +    virtual ~DOMWindowCSS() { }
>> +
> 
> Please wrap this in if-def v8 or chromium.

Why?
Comment 9 Adam Barth 2013-02-15 15:01:08 PST
Comment on attachment 188574 [details]
Patch

The bots seem to be compiling.  It doesn't look like this is a supported configuration.  If it were a supported configuration, this fix does not appear to be correct.
Comment 10 Lamarque V. Souza 2013-02-15 15:36:12 PST
Created attachment 188653 [details]
Patch

Declare virtual destructor only if V8_BINDING is enabled.
Comment 11 Lamarque V. Souza 2013-02-15 15:40:59 PST
(In reply to comment #10)
> Created an attachment (id=188653) [details]
> Patch
> 
> Declare virtual destructor only if V8_BINDING is enabled.

This does not seem to work, now it does not build with the same error message as before:

../../Source/WebKit/chromium/third_party/gold/gold64: obj/Source/WebCore/WebCore.gyp/libwebcore_bindings.a(obj/Source/WebCore/WebCore.gyp/gen/webkit/bindings/webcore_bindings.V8DerivedSources17.o): in function WebCore::checkTypeOrDieTrying(WebCore::DOMWindowCSS*):gen/webcore/bindings/V8DOMWindowCSS.cpp:52: error: undefined reference to 'vtable for WebCore::DOMWindowCSS'
collect2: ld returned 1 exit status
Comment 12 Adam Barth 2013-02-15 15:52:44 PST
Comment on attachment 188653 [details]
Patch

See comment #9.
Comment 13 Lamarque V. Souza 2013-02-15 15:56:42 PST
Created attachment 188657 [details]
Patch

Declare virtual destructor only if V8 is enabled.
Comment 14 Adam Barth 2013-02-15 15:58:50 PST
Comment on attachment 188657 [details]
Patch

See comment #9.

Please stop spamming this bug with patches without addressing earlier review comments.
Comment 15 Lamarque V. Souza 2013-02-15 16:16:24 PST
(In reply to comment #14)
> (From update of attachment 188657 [details])
> See comment #9.
> 
> Please stop spamming this bug with patches without addressing earlier review comments.

Sorry, I did not see your comments here since I was talking to the guys on irc. This problem only happens if css3-conditional-rules is enabled (see DOMWindowCSS.h) and that feature flag is not enabled by default in any buildbot. I usually use a patch to enable that feature flag before sending my patches with css3 code to buildbots so they do really test the patch [1]. Without such a trick the bots just give a green for the patch when it in fact did not test the patch.

[1] https://bugs.webkit.org/show_bug.cgi?id=106819#c7
Comment 16 Adam Barth 2013-02-15 16:47:29 PST
What are css3-conditional-rules?  My question in comment #9 is whether that's a supported configuration.
Comment 17 Lamarque V. Souza 2013-02-15 17:16:15 PST
(In reply to comment #16)
> What are css3-conditional-rules?  My question in comment #9 is whether that's a supported configuration.

Yes, it is a supported configuration, see https://bugs.webkit.org/show_bug.cgi?id=86146
Comment 18 Adam Barth 2013-02-15 17:19:16 PST
I skimmed that thread, but I don't see a statement that this configuration is supported by Chromium.  Generally speaking, if there isn't any bot coverage of a configuration, that means it's not supported.
Comment 19 Lamarque V. Souza 2013-02-15 17:38:53 PST
(In reply to comment #18)
> I skimmed that thread, but I don't see a statement that this configuration is supported by Chromium.  Generally speaking, if there isn't any bot coverage of a configuration, that means it's not supported.

I stumbled across this problem [1] when I was testing my patch implementing wavy stroke [2] for regressions. My local lucid64 chroot I set up to compile chromium also gives that same build error. Since css3-conditional-rules is a supported feature I think it should compile without errors, don't you agree? 

[1] http://queues.webkit.org/results/16527657
[2] https://bugs.webkit.org/show_bug.cgi?id=92868
Comment 20 Adam Barth 2013-02-15 17:42:01 PST
> Since css3-conditional-rules is a supported feature I think it should compile without errors, don't you agree? 

You haven't provided any evidence that css3-conditional-rules is a supported feature of the Chromium port.  I guess that means I disagree with the premise of your question.
Comment 21 Lamarque V. Souza 2013-02-15 17:56:17 PST
(In reply to comment #20)
> > Since css3-conditional-rules is a supported feature I think it should compile without errors, don't you agree? 
> 
> You haven't provided any evidence that css3-conditional-rules is a supported feature of the Chromium port.  I guess that means I disagree with the premise of your question.

What are the conditions for a feature to be considered a supported feature of the Chromium port?
Comment 22 Adam Barth 2013-02-15 18:07:06 PST
> What are the conditions for a feature to be considered a supported feature of the Chromium port?

The maintainers of the Chromium port decide to support the feature.
Comment 23 Lamarque V. Souza 2013-02-15 18:49:02 PST
(In reply to comment #22)
> > What are the conditions for a feature to be considered a supported feature of the Chromium port?
> 
> The maintainers of the Chromium port decide to support the feature.

Ok, once Chromium supports this feature this change will be necessary, you can keep this bug open if you wish. I will update my test patches to incorporate this change so I can keep on using the buildbots to test for regressions when css3-conditional-rules is enabled.