1216885 - warning: interface 'nsISelectionPrivate' is scriptable but derives from non-scriptable 'nsISelection', ../../../dist/idl/nsISelectionPrivate.idl

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

Just ran across this build warning:
{
warning: interface 'nsISelectionPrivate' is scriptable but derives from non-scriptable 'nsISelection', ../../../dist/idl/nsISelectionPrivate.idl line 31:0
interface nsISelectionPrivate : nsISelection
^
}

(The warning is present in TreeHerder logs, too -- it's not just my local machine.  e.g. this one I grabbed from recent a mozilla-central run:
http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-debug/1445386861/mozilla-central-linux64-debug-bm72-build1-build111.txt.gz
)

I'm guessing we've been spamming this warning since mid-2014 when we made nsISelection non-scriptable in bug 994964:
http://hg.mozilla.org/mozilla-central/diff/c590fcb23c3c/content/base/public/nsISelection.idl

ehsan, should we have dropped "scriptable" from nsISelectionPrivate there as well?

Comment 1

[(no longer active)]

We can't.  See <http://mxr.mozilla.org/mozilla-central/search?string=nsISelectionPrivate&find=\.js%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central>.

Comment 2

[Daniel Holbert [:dholbert]]

OK -- any suggestions on how to silence / address this warning, then?

(I think this may be our only instance of an "is scriptable but derives from non-scriptable" warning, BTW; it's the only instance in the TreeHerder log linked in comment 0, at least.)

Comment 3

[(no longer active)]

Kyle, what purpose does this warning serve?

Comment 4

[Daniel Holbert [:dholbert]]

bz may be able to offer some insights, too.

He said in bug 337723 comment 4 (10 years ago):
> In general, imo xpidl should error, not warn, when a scriptable interface inherits from a non-scriptable one.

That makes it sound like this warning is worth worrying about. But I don't really know.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well if you actually care about the IDL inheritance, you should care about it from script too, no?  And if you don't, just break the inheritance chain.

Comment 6

[Boris Zbarsky [:bzbarsky]]

What Kyle said.  The inheritance chain either makes sense or doesn't.  If it makes sense, then it needs to actually make sense; if not, there's no reason for the interfaces to inherit from each other.

The problem is C++ developers who don't realize they're creating something nonsensical from a JS point of view.

Comment 7

[(no longer active)]

So I guess in this case we want to make nsISelectionPrivate not inherit from nsISelection.  That may end up being a good chunk of work, I'm not going to do that myself, but I'd r+ a patch.  :-)

Comment 8

[:hellosct1]

hi,
I'm trying to compile and I get the same error.
I can not find the patch?
:Ehsan can you tell me where the patch is located
because I want to make demos of upcoming events where Mozilla has a stand
thank you

Christophe (:hellosct1)

Comment 9

[(no longer active)]

Sorry for the late response, Christophe, I was out on vacation/sick leave for a while.

As far as I know, no patch has been written for this bug.  What I was trying to say in comment 7 was that if someone else writes a patch, I'd be happy to review it. :-)

Comment 10

[Daniel Holbert [:dholbert]]

In addition to what ehsan said:
(In reply to :hellosct1 from comment #8)
> I'm trying to compile and I get the same error.

This build warning should not actually cause your build to fail - this is just a build warning, not an error.  (It's just noise during the build process.)

If your build was failing and you noticed this warning in the output, then it's likely there's another build error hiding somewhere else in the output, and that's the real source of your trouble.

Comment 11

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I happened across this error. It just seems like implementing Ehsan's suggestion from comment 7 just works, at least assuming we can trust try: try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88269702e9f26e562231728f520a04509cf2fa5b

I'll need to read through some code a bit more to convince myself.

Comment 12

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I looked through all of the places that use nsISelectionPrivate. Most of them QI to it, then only call methods from Private on it, so that works just fine.

layout/base/tests/test_scroll_selection_into_view.html calls methods from both nsISelection and nsISelectionPrivate:

  var selection = SpecialPowers.wrap(win.getSelection())
                               .QueryInterface(SpecialPowers.Ci.nsISelectionPrivate);
  ...
  if (target.contentDocument) {
    selection = SpecialPowers.wrap(target.contentWindow.getSelection())
                             .QueryInterface(SpecialPowers.Ci.nsISelectionPrivate);
    ...
  }
  selection.collapse(target.parentNode, 0); // method on nsISelection
  ...
  selection.scrollIntoView(FOCUS, true, vPercent, 0); // method on nsISelectionPrivate

This may work okay because we initially have an object that implements nsISelection, so when we QI it to the Private interface, we end up with an object that implements both. I'm not entirely sure. But the test still passes.

Comment 13

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: Bug 1216885 - Make nsISelectionPrivate not inherit from nsISelection.

nsISelectionPrivate is accessible to script, while nsISelection is
not, so making the former inherit from the latter means script doesn't
have a complete view of the inheritance chain so the XPIDL compiler
produces an error.

It turns out that nothing in script relies on this inheritance, which
makes sense because I'm not sure how it would even work, so just
remove it.

Review commit: https://reviewboard.mozilla.org/r/178972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178972/

Comment 14

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I guess it is axiomatic that no script depends on this, because the reason for the warning is that script can't see the inheritance...

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8907307 [details]
Bug 1216885 - Make nsISelectionPrivate not inherit from nsISelection.

https://reviewboard.mozilla.org/r/178972/#review184100

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ca25d95e1aa
Make nsISelectionPrivate not inherit from nsISelection. r=smaug

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1ca25d95e1aa