Closed Bug 1426253 Opened 2 years ago Closed 2 years ago

Expose GetClientInfo() and GetController() on nsIDocument

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(4 files, 1 obsolete file)

To make it easier to replace nsIDocument in service worker code I want to add a few helper methods.  Specifically I want to add:

  Maybe<ClientInfo> GetClientInfo() const;

And:

  Maybe<ServiceWorkerDescriptor> GetController() const;

These will just forward to the document's inner window, if there is one.
I should probably add an assertion in ClientSource::SetController() that the principal does not have the private browsing origin attribute flag.  I will figure that out tomorrow since it requires extracting it from the PrincipalInfo OMT.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d941fb872f640fdd6945bfd6996ea5702922d375
Comment on attachment 8937856 [details] [diff] [review]
P1 Expose nsIDocument GetClientInfo(), GetClientState(), and GetController(). r=asuth

Andrew, this patch pipes the GetClientInfo(), GetClientState(), and GetController() methods from the inner window through to the document.  This is a convenience to save a bunch of GetInnerWindow() and nullptr checks everywhere.
Attachment #8937856 - Flags: review?(bugmail)
Comment on attachment 8937880 [details] [diff] [review]
P2 Use nsIDocument::GetClientInfo() where possible. r=asuth

There are a few call sites already that can use the GetClientInfo() and GetClientState() helpers.
Attachment #8937880 - Flags: review?(bugmail)
Comment on attachment 8937881 [details] [diff] [review]
P3 Use the window/document GetController() method. r=asuth

While looking for call sites where we can use the GetController() helper I realized we can actually remove more code.  There is a ServiceWorkerManager::IsControlled() method that we can completely remove.  Also an nsContentUtils method.

Note, these methods had nsContentUtils::IsInPrivateBrowsing() checks that we don't do any more.  I believe these are from before the private browsing id being added to the principal origin attributes.  Now that the principal is keyed on the private browsing id, I don't think we should ever match a private browsing window with a service worker registration.

I will add an assertion for this private browsing invariant in the next patch.
Attachment #8937881 - Flags: review?(bugmail)
Comment on attachment 8937898 [details] [diff] [review]
P4 Assert that ClientSource::SetController() is never called on a client in private browsing mode. r=asuth

This patch adds a ClientInfo::IsPrivateBrowsing() helper method that looks at the client's PrincipalInfo.  We then use this to assert that ClientSource::SetController() is never called in private browsing mode.
Attachment #8937898 - Flags: review?(bugmail)
Blocks: 1425975
Attachment #8937856 - Flags: review?(bugmail) → review+
Comment on attachment 8937880 [details] [diff] [review]
P2 Use nsIDocument::GetClientInfo() where possible. r=asuth

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

Delightful fallout from part 1!
Attachment #8937880 - Flags: review?(bugmail) → review+
Comment on attachment 8937881 [details] [diff] [review]
P3 Use the window/document GetController() method. r=asuth

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

Agreed that OriginAttributes and SWM's partitioning by origin covers the private browsing case.

Also, nice additional cleanup!
Attachment #8937881 - Flags: review?(bugmail) → review+
Attachment #8937898 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7fb331fc914
P1 Expose nsIDocument GetClientInfo(), GetClientState(), and GetController(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b676a0c9383
P2 Use nsIDocument::GetClientInfo() where possible. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/83c725dad7a9
P3 Use the window/document GetController() method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/661c1ca56b09
P4 Assert that ClientSource::SetController() is never called on a client in private browsing mode. r=asuth
Priority: -- → P2
Depends on: 1426732
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.