1150120 - Pass outerWindowID of frame up with contextmenu message

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

Split out from Mossop's work in bug 1146454 - in order to avoid passing CPOWs around for contentWindow's and contentDocument's, we should make it possible to pass outerWindowIDs around instead.

Comment 1

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

Attachment: Pass outerWindowID up when sending contextmenu message up from content process. r=?

This will hopefully allow us to avoid using CPOWs for contentDocument and contentWindow
when doing things like printing or viewing the source of subframes.

Comment 2

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8586874 [details] [diff] [review]
Pass outerWindowID up when sending contextmenu message up from content process. r=?

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

Looks good to me!

::: browser/base/content/nsContextMenu.js
@@ +616,5 @@
>        this.principal = this.target.ownerDocument.nodePrincipal;
> +      this.frameOuterWindowID = this.target.ownerDocument.defaultView
> +                                  .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                  .getInterface(Ci.nsIDOMWindowUtils)
> +                                  .outerWindowID;

Nit: this.target.ownerDocument is used a few times in this section and below... shouldn't we add a var/let for it?

::: browser/base/content/tabbrowser.xml
@@ +3773,5 @@
>                                            docLocation: aMessage.data.docLocation,
>                                            charSet: aMessage.data.charSet,
>                                            referrer: aMessage.data.referrer,
>                                            referrerPolicy: aMessage.data.referrerPolicy,
> +                                          frameOuterWindowID: aMessage.data.frameOuterWindowID,

Nit: extra |,| at the end

Comment 3

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> Comment on attachment 8586874 [details] [diff] [review]
> Pass outerWindowID up when sending contextmenu message up from content
> process. r=?
> 
> Review of attachment 8586874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me!
> 

Thanks for the fast review, gabor!

> ::: browser/base/content/nsContextMenu.js
> @@ +616,5 @@
> >        this.principal = this.target.ownerDocument.nodePrincipal;
> > +      this.frameOuterWindowID = this.target.ownerDocument.defaultView
> > +                                  .QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                  .getInterface(Ci.nsIDOMWindowUtils)
> > +                                  .outerWindowID;
> 
> Nit: this.target.ownerDocument is used a few times in this section and
> below... shouldn't we add a var/let for it?
> 

Good idea - done.

> ::: browser/base/content/tabbrowser.xml
> @@ +3773,5 @@
> >                                            docLocation: aMessage.data.docLocation,
> >                                            charSet: aMessage.data.charSet,
> >                                            referrer: aMessage.data.referrer,
> >                                            referrerPolicy: aMessage.data.referrerPolicy,
> > +                                          frameOuterWindowID: aMessage.data.frameOuterWindowID,
> 
> Nit: extra |,| at the end

In IRC, gabor and I talked about this, and we think we'll keep the trailing comma so as not to require future coders to change the blame on the final item when adding new items just by adding a comma.

Landed on fx-team as:

remote:   https://hg.mozilla.org/integration/fx-team/rev/26113aaa2a74

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/26113aaa2a74