nsISecureBrowserUI::Init should take a docshell instead of a content window

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

The only thing the nsSecureBrowserUIImpl constructor does with the window is get the docshell.

Instead, we could just pass the docshell directly. This would reduce code size and avoid constructing an about:blank content window in the docshell when constructing the security UI / info object.

This seems a pretty simple change so, assuming that doesn't turn out to be an illusion, I'd like to do that in this bug, to fix the immediate problem I'm seeing where the securityUI initialization forces initialization of the content window - plus tidying up some code that currently gets a content window reference for no reason other than to construct this object.

Longer-term though, I wonder why consumers are responsible for the construction of this object. I guess there's a performance impact when constructing it for docshells that don't need it (ie have no security UI). On the other hand, perhaps we could rework the implementation so it can be fetched "just in time". That is, make docshell construct one if someone requests it and it doesn't exist yet, and make the securityInfo object populate its mState member if it's 0/uninitialized, and someone asks it for the security state, using the docshell's document channel (after which it'll then be updated the usual way).
Summary: nsISecureBrowserUI should either be constructed by docshell itself or at least take a docshell instead of a content window → nsISecureBrowserUI::Init take a docshell instead of a content window
Summary: nsISecureBrowserUI::Init take a docshell instead of a content window → nsISecureBrowserUI::Init should take a docshell instead of a content window
(In reply to :Gijs (he/him) from comment #0)
> Longer-term though, I wonder why consumers are responsible for the
> construction of this object. I guess there's a performance impact when
> constructing it for docshells that don't need it (ie have no security UI).
> On the other hand, perhaps we could rework the implementation so it can be
> fetched "just in time". That is, make docshell construct one if someone
> requests it and it doesn't exist yet, and make the securityInfo object
> populate its mState member if it's 0/uninitialized, and someone asks it for
> the security state, using the docshell's document channel (after which it'll
> then be updated the usual way).

Hm. I looked at this a bit, but I don't see a good way of making this lazy today - it looks and feels as if consumers explicitly construct this today because they want the onSecurityChange() notifications, and they don't otherwise update anything, so if we push responsibility into docshell we'd have to create it eagerly, or we'd miss the notifications. That's definitely possible, but seems like a bit of work that we don't necessarily need to do here, also because we'd want to avoid doing it for chrome type docshells etc. etc.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Depends on: 1494009
Depends on: 1494029
This also removes the (afaict, unused) stub implementation from TabParent. The netwerk header
inclusions were necessary because those files included TabParent.h and through it,
nsISecureBrowserUI, but now TabParent.h no longer does that.
Comment on attachment 9011917 [details]
Bug 1493655 - make nsISecureBrowserUI initialize from a docshell instead of a window, r?keeler,nika

Dana Keeler [:keeler] (she/her) (use needinfo) has approved the revision.
Attachment #9011917 - Flags: review+
Attachment #9011917 - Attachment description: Bug 1493655 - make nsISecureBrowserUI initialize from a docshell instead of a window, r?keeler → Bug 1493655 - make nsISecureBrowserUI initialize from a docshell instead of a window, r?keeler,nika
Comment on attachment 9011917 [details]
Bug 1493655 - make nsISecureBrowserUI initialize from a docshell instead of a window, r?keeler,nika

:Nika Layzell has approved the revision.
Attachment #9011917 - Flags: review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d86f6268d191
make nsISecureBrowserUI initialize from a docshell instead of a window, r=keeler,nika
https://hg.mozilla.org/mozilla-central/rev/d86f6268d191
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.