Fix build when css3-conditional-rules feature flag is enabled

• Component: CSS
• Status: RESOLVED INVALID

[Lamarque V. Souza]

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.

[Lamarque V. Souza]

Created attachment 188539 [details]
Patch

Proposed patch

[Lamarque V. Souza]

Created attachment 188541 [details]
Patch (EWS only version)

Path with css3-conditional-rules enabled

[Lamarque V. Souza]

Created attachment 188544 [details]
Patch

Update ChangeLog

[Thiago Marcos P. Santos]

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.

[Lamarque V. Souza]

(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.

[Lamarque V. Souza]

Created attachment 188574 [details]
Patch

Update ChangeLog with more additional info about the problem.

[Ryosuke Niwa]

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.

[Adam Barth]

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?

[Adam Barth]

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.

[Lamarque V. Souza]

Created attachment 188653 [details]
Patch

Declare virtual destructor only if V8_BINDING is enabled.

[Lamarque V. Souza]

(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

[Adam Barth]

Comment on attachment 188653 [details]
Patch

See comment #9.

[Lamarque V. Souza]

Created attachment 188657 [details]
Patch

Declare virtual destructor only if V8 is enabled.

[Adam Barth]

Comment on attachment 188657 [details]
Patch

See comment #9.

Please stop spamming this bug with patches without addressing earlier review comments.

[Lamarque V. Souza]

(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

[Adam Barth]

What are css3-conditional-rules?  My question in comment #9 is whether that's a supported configuration.

[Lamarque V. Souza]

(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

[Adam Barth]

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.

[Lamarque V. Souza]

(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

[Adam Barth]

> 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.

[Lamarque V. Souza]

(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?

[Adam Barth]

> 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.

[Lamarque V. Souza]

(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.