Summary: | SVGZoomAndPan constants are missing from window object | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||||
Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, gustavo, haraken, japhet, jberlin, ojan, philn, rakuco, rwlbuis, sam, vestbo, webkit.review.bot, xan.lopez, zimmermann | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||
Bug Depends on: | 20430 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2007-10-13 21:54:47 PDT
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. |