SVGZoomAndPan constants are missing from window object Unfortunately we can't just turn on "GeneratesContructor" for SVGZoomAndPan and have everything work right. There need to be modifications to the autogen system to generate window properties which just define constants but don't actually let you construct them (because, as in this case, they're only virtual base classes in C++).
Created attachment 142874 [details] Patch
Comment on attachment 142874 [details] Patch Attachment 142874 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12727658
Comment on attachment 142874 [details] Patch Attachment 142874 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12730206
Comment on attachment 142874 [details] Patch Attachment 142874 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12726769
Comment on attachment 142874 [details] Patch Attachment 142874 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12733215
Comment on attachment 142874 [details] Patch Attachment 142874 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12724837
Comment on attachment 142874 [details] Patch Attachment 142874 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12730210
Comment on attachment 142874 [details] Patch Attachment 142874 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12734206
Created attachment 142883 [details] Patch v2
Comment on attachment 142883 [details] Patch v2 Attachment 142883 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12734235
Comment on attachment 142883 [details] Patch v2 Attachment 142883 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12734236
Created attachment 143244 [details] Patch v3
Comment on attachment 143244 [details] Patch v3 Attachment 143244 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12759029
Comment on attachment 143244 [details] Patch v3 Attachment 143244 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12733938
Comment on attachment 143244 [details] Patch v3 Attachment 143244 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12736919
Comment on attachment 143244 [details] Patch v3 Attachment 143244 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12742559
Comment on attachment 143244 [details] Patch v3 Attachment 143244 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12738784
Comment on attachment 143244 [details] Patch v3 Attachment 143244 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12749280
Created attachment 143301 [details] Patch v4
Comment on attachment 143301 [details] Patch v4 Attachment 143301 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12748457
Comment on attachment 143301 [details] Patch v4 Attachment 143301 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12761060
I think I finally found the V8 build problem, v5 should fix chromium. Speculative fix for the toJS() overload problem on win is included, let's see.
Created attachment 143311 [details] Patch v5
Comment on attachment 143311 [details] Patch v5 Attachment 143311 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12749354
The win build makes me sad :( 4>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Debug\obj\WebCore\DerivedSources\JSSVGDocument.cpp(149) : error C2668: 'WebCore::toJS' : ambiguous call to overloaded function 4> c:\cygwin\home\buildbot\webkit\webkitbuild\debug\obj\webcore\derivedsources\JSNode.h(111): could be 'JSC::JSValue WebCore::toJS(JSC::ExecState *,WebCore::JSDOMGlobalObject *,WebCore::Node *)' 4> c:\cygwin\home\buildbot\webkit\webkitbuild\debug\obj\webcore\derivedsources\JSSVGZoomAndPan.h(88): or 'JSC::JSValue WebCore::toJS(JSC::ExecState *,WebCore::JSDOMGlobalObject *,WebCore::SVGZoomAndPan *)' 4> while trying to match the argument list '(JSC::ExecState *, WebCore::JSDOMGlobalObject *, WebCore::SVGSVGElement *)' It can't figure out if it's supposed to call toJS(Node*) or toJS(SVGZoomAndPan*) for a SVGSVGElement. It doesn't clash with SVGFitToViewBox toJS, or SVGTests toJS, and I don't get that. It only complains that toJS(Node*) / toJS(SVGZoomAndPan*) clashes - why is that? :( Here's the offending code from JSSVGDocument.cpp: JSValue jsSVGDocumentRootElement(ExecState* exec, JSValue slotBase, PropertyName) { JSSVGDocument* castedThis = jsCast<JSSVGDocument*>(asObject(slotBase)); UNUSED_PARAM(exec); SVGDocument* impl = static_cast<SVGDocument*>(castedThis->impl()); JSValue result = toJS(exec, castedThis->globalObject(), WTF::getPtr(impl->rootElement())); return result; } Of course a static_cast would solve this, but I'd rather find the problem for real instead of adding work-arounds to CodeGeneratorJS.pm, just for win & SVGSVGElement.
I've had a nice discussion with Hajime on IRC and the problem is nailed down. Patch v6 should hopefully work everywhere.
Created attachment 143488 [details] Patch v6
Comment on attachment 143488 [details] Patch v6 Attachment 143488 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12769140
Oops, still a typo in CodeGeneratorV8.pm, so good that we have the EWS :-)
Created attachment 143499 [details] Patch v7
Comment on attachment 143499 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=143499&action=review > Source/WebCore/svg/SVGViewElement.h:44 > + using SVGStyledElement::deref; Do we need these? > Source/WebCore/svg/SVGViewSpec.h:35 > + public SVGFitToViewBox { Do we need this change? > Source/WebCore/svg/SVGViewSpec.h:55 > + void setZoomAndPan(unsigned short, ExceptionCode&); It looks there is no implementation of this function. > Source/WebCore/svg/SVGZoomAndPan.h:81 > + // These are never called, and thus ASSERT_NOT_REACHED. Maybe inlining help to save the code size.
(In reply to comment #31) > (From update of attachment 143499 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143499&action=review > > > Source/WebCore/svg/SVGViewElement.h:44 > > + using SVGStyledElement::deref; > > Do we need these? Yes, SVGViewElement has ref/deref via SVGStyledElement inheritance (inherits from Node). SVGZoomAndPan also has ref/deref, but it's never used (can't be removed as the generated JSSVG* code depends on the existence of ref/deref). > > Source/WebCore/svg/SVGViewSpec.h:35 > > + public SVGFitToViewBox { > > Do we need this change? Yes, it mimics the W3C IDL again - consider it a progression. > > > Source/WebCore/svg/SVGViewSpec.h:55 > > + void setZoomAndPan(unsigned short, ExceptionCode&); > > It looks there is no implementation of this function. There is: NO_RETURN_DUE_TO_ASSERT void SVGZoomAndPan::setZoomAndPan(unsigned short) It only exists so that we don't need to special case the CodeGenerators for SVGZoomAndPan. > > Source/WebCore/svg/SVGZoomAndPan.h:81 > > + // These are never called, and thus ASSERT_NOT_REACHED. > > Maybe inlining help to save the code size. Hm, does it really help? I usually don't inline without having checked it helps.
Comment on attachment 143499 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=143499&action=review Looks good! A welcome cleanup. > Source/WebCore/ChangeLog:20 > + MSVC can't decide wheteher it should call toJS(Node*) or toJS(SVGZoomAndPan*) for a SVGSVGElement. Typo: whether. > Source/WebCore/ChangeLog:26 > + Maybe reference the test section here, why you do not need to add new tests.
Committed r118353: <http://trac.webkit.org/changeset/118353>
(In reply to comment #34) > Committed r118353: <http://trac.webkit.org/changeset/118353> This broke the Mac tests: http://build.webkit.org/results/Lion%20Debug%20(Tests)/r118365%20(6929)/fast/js/global-constructors-pretty-diff.html I will check in updated results this time, but please watch the bots next time you make a change.
(In reply to comment #35) > (In reply to comment #34) > > Committed r118353: <http://trac.webkit.org/changeset/118353> > > This broke the Mac tests: > > http://build.webkit.org/results/Lion%20Debug%20(Tests)/r118365%20(6929)/fast/js/global-constructors-pretty-diff.html > > I will check in updated results this time, but please watch the bots next time you make a change. Sorry Jessie! To my defense: as you can see the cr-linux EWS, which runs all tests, is green. I didn't realize fast/js/global-constructors.html is skipped for Chromium.
(In reply to comment #36) > (In reply to comment #35) > > (In reply to comment #34) > > > Committed r118353: <http://trac.webkit.org/changeset/118353> > > > > This broke the Mac tests: > > > > http://build.webkit.org/results/Lion%20Debug%20(Tests)/r118365%20(6929)/fast/js/global-constructors-pretty-diff.html > > > > I will check in updated results this time, but please watch the bots next time you make a change. > > Sorry Jessie! To my defense: as you can see the cr-linux EWS, which runs all tests, is green. I didn't realize fast/js/global-constructors.html is skipped for Chromium. Now that we have the Mac Lion bots green (or pretty close to green at any given time), please try to look at them as well *after* you commit.