expose GetClientInfo() and GetController() on nsIGlobalObject

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
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.
Attachment #8941532 - Flags: review?(bugmail)
Blocks: 1429200
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.)
Attachment #8941532 - Flags: review?(bugmail) → review+
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.
Attachment #8941532 - Flags: feedback?(nfroyd)
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
Attachment #8941532 - Flags: feedback?(nfroyd)
(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.
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
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.
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.
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.
Attachment #8941888 - Attachment is obsolete: true
Attachment #8942026 - Flags: review+
Summary: expose GetClientInfo(), GetClientState(), and GetController() on nsIGlobalObject → expose GetClientInfo() and GetController() on nsIGlobalObject

Comment 11

Last year
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdccad454192
Expose GetClientInfo() and GetController() on nsIGlobalObject. r=asuth

Comment 12

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/fdccad454192
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.