Bug 109926

Created attachment 188539 [details] Patch Proposed patch

Created attachment 188541 [details] Patch (EWS only version) Path with css3-conditional-rules enabled

Created attachment 188544 [details] Patch Update ChangeLog

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.

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

Created attachment 188574 [details] Patch Update ChangeLog with more additional info about the problem.

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

Created attachment 188653 [details] Patch Declare virtual destructor only if V8_BINDING is enabled.

(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 on attachment 188653 [details] Patch See comment #9.

Created attachment 188657 [details] Patch Declare virtual destructor only if V8 is enabled.

Comment on attachment 188657 [details] Patch See comment #9. Please stop spamming this bug with patches without addressing earlier review comments.

(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

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

(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

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.

(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

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

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

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

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