1507540 - Use notxpcom attributes more in xpidl, now that we can

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Fwiw, the component was correct.  This is going to have changes all over the tree, once I finish ironing out the  Android build issues.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Use more notxpcom attributes in caps/

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use more notxpcom attributes in docshell/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Use more notxpcom attributes in netwerk/

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove unused nsIProfiler::GetStartParams

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Use more notxpcom attributes in widget/

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Make nsIVariant's "type" a notxpcom attribute

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Use notxpcom more in dom/

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 9025740 [details] [diff] [review]
part 6.   Make nsIVariant's "type" a notxpcom attribute

Review of attachment 9025740 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/ds/nsIVariant.idl
@@ +64,4 @@
>  [scriptable, uuid(81e4c2de-acac-4ad6-901a-b5fb1b851a0d)]
>  interface nsIVariant : nsISupports
>  {
> +    [notxpcom,nostdcall] readonly attribute uint16_t     dataType;

Two questions:

1) Did we really want to delete `noscript` here?
2) Why are we changing this to `nostdcall` at the same time?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> 1) Did we really want to delete `noscript` here?

notxpcom implies noscript.  I could put the noscript back, but it would just be redundant.

Style around this is inconsistent in the tree, and I went with the shorter thing, but I can see the argument for doing the more explicit thing.  Up to you what you want to see in your module.

> 2) Why are we changing this to `nostdcall` at the same time?

To cut down on the ugly in the C++, basically.  I could leave off the nostdcall, and then the impls would have to have NS_IMETHODIMP_(uint16_t) as the return type instead of just uint16_t.

Frankly, I think we should have noscript imply nostdcall, but doing that as a single change is a bit hard.  ;)  We can get there incrementally if we just add nostdcall whenever we can, then at some point it's added to all noscript things and we can just make noscript imply it. :)

Is there a reason to _not_ add nostdcall?

Comment 11

[Nathan Froyd [:froydnj]]

Thanks for the explanation!

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #10)
> > 1) Did we really want to delete `noscript` here?
> 
> notxpcom implies noscript.  I could put the noscript back, but it would just
> be redundant.
>
> Style around this is inconsistent in the tree, and I went with the shorter
> thing, but I can see the argument for doing the more explicit thing.

Ah yes.  We should probably get to the point where we're just specifying notxpcom instead of notxpcom,noscript.

> > 2) Why are we changing this to `nostdcall` at the same time?
> 
> To cut down on the ugly in the C++, basically.  I could leave off the
> nostdcall, and then the impls would have to have NS_IMETHODIMP_(uint16_t) as
> the return type instead of just uint16_t.

This seems very reasonable.  Having noscript imply nostdcall also seems like a reasonable goal to aspire to.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9025738 [details] [diff] [review]
part 4.  Remove unused nsIProfiler::GetStartParams

Review of attachment 9025738 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think usage of this attribute went by the wayside during the work in bug 1328363 - looks like we just forgot to remove it. Thanks!

Comment 14

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

(I need to look at carefully whether returning Foo* leads to code where strong pointer isn't used when needed.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

> (I need to look at carefully whether returning Foo* leads to code where strong pointer isn't used when needed.)

Makes sense.  I generally kept using strong pointers for that reason, fwiw, so it should not be too hard to check.

Comment 16

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

I mainly meant that in the sense of future-proof-ness.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fair.

I wonder whether it's worth adding an annotation to xpidl for changing the return type to already_AddRefed<>.

Comment 18

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

That would be quite nice I think.

Comment 19

[Andrew McCreight [:mccr8]]

Comment on attachment 9025741 [details] [diff] [review]
part 7.  Use notxpcom more in dom/

Review of attachment 9025741 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsObjectLoadingContent.cpp
@@ +3721,4 @@
>  {
>    // We don't resolve anything; we just try to make sure we're instantiated.
>    // This purposefully does not fire for chrome/xray resolves, see bug 967694
> +  ScriptRequestPluginInstance(aCx);

Unused << please

@@ +3742,4 @@
>    // Just like DoResolve, just make sure we're instantiated.  That will do
>    // the work our Enumerate hook needs to do.  This purposefully does not fire
>    // for xray resolves, see bug 967694
> +  ScriptRequestPluginInstance(aCx);

Unused << please

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edff28b27f20
part 1.  Use more notxpcom attributes in caps/.  r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e826769668
part 2.  Use more notxpcom attributes in docshell/.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/52d07c32fa7c
part 3.  Use more notxpcom attributes in netwerk/.  r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cdbce9b4a51
part 4.  Remove unused nsIProfiler::GetStartParams.  r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/f50e3a884aa8
part 5.  Use more notxpcom attributes in widget/.   r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b60870ec6f8
part 6.   Make nsIVariant's "type" a notxpcom attribute.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/d583bf98a4b2
part 7.  Use notxpcom more in dom/.  r=mccr8

Comment 21

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/edff28b27f20
https://hg.mozilla.org/mozilla-central/rev/a1e826769668
https://hg.mozilla.org/mozilla-central/rev/52d07c32fa7c
https://hg.mozilla.org/mozilla-central/rev/5cdbce9b4a51
https://hg.mozilla.org/mozilla-central/rev/f50e3a884aa8
https://hg.mozilla.org/mozilla-central/rev/6b60870ec6f8
https://hg.mozilla.org/mozilla-central/rev/d583bf98a4b2