1429486 - expose GetClientInfo() and GetController() on nsIGlobalObject

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

In order to make other refactoring easier I'd like to expose GetClientInfo(), GetClientState(), and GetController() on nsIGlobalObject.  These would return Maybe<> objects.  For window and worker globals they will get the values from the underlying ClientSource.  For other globals, like sandboxes, etc, these will always return a nothing Maybe<>.

I previously spoke with Boris about this in IRC and he was not opposed.

One of the goals here is to reduce the amount of code doing "if window else worker" sprinkled all over the place.  Instead we can just operate on the global each DOM object is owned by.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Expose GetClientInfo(), GetClientState(), and GetController() on nsIGlobalObject. r=asuth

Andrew, this exposes GetClientInfo(), GetClientState(), and GetController() on nsIGlobalObject.  This will be useful as we refactor service workers and move towards exposing more on workers.

This bug doesn't have any code that uses the change, but I plan to shortly.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8941532 [details] [diff] [review]
Expose GetClientInfo(), GetClientState(), and GetController() on nsIGlobalObject. r=asuth

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

::: dom/base/nsIGlobalObject.h
@@ +17,5 @@
>  { 0x11afa8be, 0xd997, 0x4e07, \
>  { 0xa6, 0xa3, 0x6f, 0x87, 0x2e, 0xc3, 0xee, 0x7f } }
>  
> +namespace mozilla {
> +template<typename T> class Maybe;

Do you really want to forward-declare this?  The actual declaration has the annotations `MOZ_NON_PARAM MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS` that seem useful, and it almost seems certain that Maybe.h is already included by something else.  If the answer is yes, maybe discuss with :nfroyd and create an mfbtFwd.h like we have nsStringFwd.h?

nsJSEnvironment.h seems to be the only existing place that tries to do a forward of this, at https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/dom/base/nsJSEnvironment.h#31

::: dom/workers/WorkerScope.h
@@ +229,5 @@
> +
> +  Maybe<ClientState>
> +  GetClientState() const override;
> +
> +  Maybe<ServiceWorkerDescriptor>

(restating: does not need forward because Workers.h is explicitly included by WorkerScope.h, and Workers.h includes ServiceWorkerDescriptor.h.)

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941532 [details] [diff] [review]
Expose GetClientInfo(), GetClientState(), and GetController() on nsIGlobalObject. r=asuth

Nathan, do you have any problem with this forward declaration of Maybe<>?  Since nsIGlobalObject.h is included in a lot of files I was trying to avoid adding any additional headers here.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8941532 [details] [diff] [review]
Expose GetClientInfo(), GetClientState(), and GetController() on nsIGlobalObject. r=asuth

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

asuth brings up a good point.  My interpretation of asuth's point is, "given that we forward-declared Maybe<T>, do the static analyses triggered by the tags on the declaration of Maybe<T> always apply even when we only saw the forward declaration for some code?"

I think the answer to this question is "yes": for code which passes around (possibly cv-qualified) Maybe<T>& or Maybe<T>*, that's fine.  But eventually, code passing in Maybe<T>&/Maybe<T>* will need to declare concrete Maybe<T> values, and mozilla/Maybe.h will need to be included to declare those values correctly (e.g. knowing the size of Maybe<T>).  Those concrete instances of Maybe<T> are the places where the static analysis applies, and I *think* the tags on the actual declaration will take precedence over the non-existence of tags on the forward declaration.  I am not 100% certain on that, though.

FWIW, Google's style guide frowns on forward declarations:

https://google.github.io/styleguide/cppguide.html#Forward_Declarations

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> FWIW, Google's style guide frowns on forward declarations:
> 
> https://google.github.io/styleguide/cppguide.html#Forward_Declarations

Given that google doesn't care a wit about compile speed, I don't find this compelling.  Have you tried to compile their tree without google's compute infrastructure?  It took around ~4 hours last time I tried on my 2016 macbook pro.

Anyway, since we're unsure, I'll just use the #include for now here.  I think in general, though, we should be able to forward declare things.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Expose GetClientInfo(), GetClientState(), and GetController() on nsIGlobalObject. r=asuth

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Inbound is closed, so lets do a try compilation push to make sure I didn't break anything:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a014321f71906edf84866e0d38858409def28982

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Ugh, I can't include the full Maybe.h here without also pulling in the full ClientInfo.h, ClientState.h, and ServiceWorkerDescriptor.h.  This is really not good for such a widespread header where only limited code is going to call these methods.

Comment 9

[Nathan Froyd [:froydnj]]

I didn't even look at the patch; I'm a little surprised you can get away with:

template<class> class Maybe;

const Maybe<X> MyVirtualMethod();

in the first place because the size of X determines how Maybe<X> values are returned.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Expose GetClientInfo() and GetController() on nsIGlobalObject. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0408dc11f2fbe1ff376815b013fcbfc316f0405c

This includes Maybe.h, ClientInfo.h, and ServiceWorkerDescriptor.h.  I had to punt on exposing GetClientState() for now, though, since ClientState.h has a dependency on nsIGlobalObject.h currently.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdccad454192
Expose GetClientInfo() and GetController() on nsIGlobalObject. r=asuth

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/fdccad454192