Last Comment Bug 881128 - Remove nsIDOMGetSVGDocument
: Remove nsIDOMGetSVGDocument
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-09 20:43 PDT by David Zbarsky (:dzbarsky)
Modified: 2013-07-15 07:27 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (17.99 KB, patch)
2013-06-09 20:43 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review+
Details | Diff | Splinter Review
Fix tests (2.40 KB, patch)
2013-06-11 14:51 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2013-06-09 20:43:57 PDT
Created attachment 760273 [details] [diff] [review]
Patch
Comment 1 Boris Zbarsky [:bz] 2013-06-10 11:53:19 PDT
Comment on attachment 760273 [details] [diff] [review]
Patch

r=me
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2013-06-10 11:54:37 PDT
This removes window.GetSVGDocument? If so, please fix test_interfaces.html
Comment 3 David Zbarsky (:dzbarsky) 2013-06-10 17:49:55 PDT
(In reply to :Ms2ger from comment #2)
> This removes window.GetSVGDocument? If so, please fix test_interfaces.html

Right, didn't realize we don't actually run that test...
Comment 4 David Zbarsky (:dzbarsky) 2013-06-10 17:51:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/efdf2d801664
Comment 5 Boris Zbarsky [:bz] 2013-06-10 18:41:51 PDT
Er, what do you mean we don't actually run it?
Comment 6 Phil Ringnalda (:philor) 2013-06-10 19:29:04 PDT
I think he means "after philor backs me out in https://hg.mozilla.org/integration/mozilla-inbound/rev/81b227f1a522 for causing test_interfaces.html to fail, then I'm going to know deep down in my bones that we do in fact run it, and that it will in fact fail when I land this patch."
Comment 7 Phil Ringnalda (:philor) 2013-06-10 19:58:33 PDT
Also seems to have busted a whole bunch of devtools browser-chrome tests.
Comment 8 David Zbarsky (:dzbarsky) 2013-06-10 22:08:15 PDT
(In reply to Boris Zbarsky (:bz) from comment #5)
> Er, what do you mean we don't actually run it?

Looks like we don't run it on linux64.
Comment 9 Masatoshi Kimura [:emk] 2013-06-10 22:09:32 PDT
Why *removing* an interface caused a failure of test_interfaces.html? Perhaps clobber was needed.
But anyway, we also need to remove use of window.GetSVGDocument from devtools.
Comment 10 Masatoshi Kimura [:emk] 2013-06-10 22:21:37 PDT
BTW do we really want to remove GetSVGDocument? Even the latest editor's draft defines the interface.
https://dvcs.w3.org/hg/svg2/raw-file/tip/master/struct.html#InterfaceGetSVGDocument
We should convert it to WebIDL instead, no?
Comment 11 Cameron McCormack (:heycam) 2013-06-10 22:30:26 PDT
I don't think we want to remove getSVGDocument from HTMLObjectElement.prototype, etc., since it looks like people use it.  I think the interface in the spec should become [NoInterfaceObject], however.  Although Chrome does implement the interface, it doesn't expose it on the window.
Comment 12 Cameron McCormack (:heycam) 2013-06-10 22:30:44 PDT
(Better view of the SVG 2 draft here: https://svgwg.org/svg2-draft/struct.html#InterfaceGetSVGDocument)
Comment 13 Masatoshi Kimura [:emk] 2013-06-11 04:28:55 PDT
(In reply to Cameron McCormack (:heycam) from comment #11)
> Although Chrome does implement the interface, it doesn't expose it on the
> window.

It's the same with IE 10 and Opera 12.15. Looks like we are the only browser exposing GetSVGDocument on the window.
Comment 14 Boris Zbarsky [:bz] 2013-06-11 07:13:37 PDT
> Looks like we don't run it on linux64.

Sure looks to me like we ran it on linux64 on your inbound push...

I do wonder why it failed, though.  It's possible that removing a .idl file needs a clobber or something dumb like that (e.g. if we don't properly remove .xpt files?)
Comment 15 Gregory Szorc [:gps] 2013-06-11 08:25:11 PDT
There are a number of open build config bugs around bad dependencies in XPIDL land. Hopefully these will all magically go away with bug 850380.
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2013-06-11 09:31:41 PDT
(In reply to David Zbarsky (:dzbarsky) from comment #3)
> (In reply to :Ms2ger from comment #2)
> > This removes window.GetSVGDocument? If so, please fix test_interfaces.html
> 
> Right, didn't realize we don't actually run that test...

We do, but it doesn't usually catch *removed* interfaces.
Comment 17 David Zbarsky (:dzbarsky) 2013-06-11 14:51:33 PDT
Created attachment 761171 [details] [diff] [review]
Fix tests
Comment 18 Boris Zbarsky [:bz] 2013-06-11 14:58:50 PDT
Comment on attachment 761171 [details] [diff] [review]
Fix tests

>+      // then the node is a frame

No, you need to keep null-checking the return value of getSVGDocument().  Think <object> or <embed> showing an image or a plug-in...

r=me with that fixed.
Comment 19 David Zbarsky (:dzbarsky) 2013-06-11 21:57:44 PDT
(In reply to Boris Zbarsky (:bz) from comment #18)
> Comment on attachment 761171 [details] [diff] [review]
> Fix tests
> 
> >+      // then the node is a frame
> 
> No, you need to keep null-checking the return value of getSVGDocument(). 
> Think <object> or <embed> showing an image or a plug-in...
> 
> r=me with that fixed.

Ah, yes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/51369eb28a8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c813a569658

Note You need to log in before you can comment on or make changes to this bug.