Bug 15494 – SVGZoomAndPan constants are missing from window object

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

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

[Nikolas Zimmermann]

Created attachment 142874 [details]
Patch

[Early Warning System Bot]

Comment on attachment 142874 [details]
Patch

Attachment 142874 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12727658

[Early Warning System Bot]

Comment on attachment 142874 [details]
Patch

Attachment 142874 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12730206

[Gyuyoung Kim]

Comment on attachment 142874 [details]
Patch

Attachment 142874 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12726769

[Build Bot]

Comment on attachment 142874 [details]
Patch

Attachment 142874 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12733215

[WebKit Review Bot]

Comment on attachment 142874 [details]
Patch

Attachment 142874 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12724837

[Build Bot]

Comment on attachment 142874 [details]
Patch

Attachment 142874 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12730210

[Build Bot]

Comment on attachment 142874 [details]
Patch

Attachment 142874 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12734206

[Nikolas Zimmermann]

Created attachment 142883 [details]
Patch v2

[WebKit Review Bot]

Comment on attachment 142883 [details]
Patch v2

Attachment 142883 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12734235

[Build Bot]

Comment on attachment 142883 [details]
Patch v2

Attachment 142883 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12734236

[Nikolas Zimmermann]

Created attachment 143244 [details]
Patch v3

[Build Bot]

Comment on attachment 143244 [details]
Patch v3

Attachment 143244 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12759029

[Early Warning System Bot]

Comment on attachment 143244 [details]
Patch v3

Attachment 143244 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12733938

[Early Warning System Bot]

Comment on attachment 143244 [details]
Patch v3

Attachment 143244 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12736919

[Build Bot]

Comment on attachment 143244 [details]
Patch v3

Attachment 143244 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12742559

[WebKit Review Bot]

Comment on attachment 143244 [details]
Patch v3

Attachment 143244 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12738784

[Build Bot]

Comment on attachment 143244 [details]
Patch v3

Attachment 143244 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12749280

[Nikolas Zimmermann]

Created attachment 143301 [details]
Patch v4

[Build Bot]

Comment on attachment 143301 [details]
Patch v4

Attachment 143301 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12748457

[WebKit Review Bot]

Comment on attachment 143301 [details]
Patch v4

Attachment 143301 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12761060

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

Created attachment 143311 [details]
Patch v5

[Build Bot]

Comment on attachment 143311 [details]
Patch v5

Attachment 143311 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12749354

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

I've had a nice discussion with Hajime on IRC and the problem is nailed down. Patch v6 should hopefully work everywhere.

[Nikolas Zimmermann]

Created attachment 143488 [details]
Patch v6

[WebKit Review Bot]

Comment on attachment 143488 [details]
Patch v6

Attachment 143488 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12769140

[Nikolas Zimmermann]

Oops, still a typo in CodeGeneratorV8.pm, so good that we have the EWS :-)

[Nikolas Zimmermann]

Created attachment 143499 [details]
Patch v7

[Hajime Morrita]

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.

[Nikolas Zimmermann]

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

[Rob Buis]

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.

[Nikolas Zimmermann]

Committed r118353: <http://trac.webkit.org/changeset/118353>

[Jessie Berlin]

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

[Nikolas Zimmermann]

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

[Jessie Berlin]

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