Comment 1 Justin Lebar (not reading bugmail) 2012-05-10 23:43:32 PDT

Comment on attachment 622998 [details] [diff] [review]
Patch v1

>@@ -43,19 +47,21 @@ BrowserElementChild.prototype = {
> 
>     // A mozbrowser iframe contained inside a mozapp iframe should return false
>     // for nsWindowUtils::IsPartOfApp (unless the mozbrowser iframe is itself
>     // also mozapp).  That is, mozapp is transitive down to its children, but
>     // mozbrowser serves as a barrier.
>     //
>     // This is because mozapp iframes have some privileges which we don't want
>     // to extend to untrusted mozbrowser content.
>+    //
>+    // Set the app state of the window by requesting it to the parent.
>     content.QueryInterface(Ci.nsIInterfaceRequestor)
>            .getInterface(Components.interfaces.nsIDOMWindowUtils)
>-           .setIsApp(false);
>+           .setIsApp(sendSyncMsg('get-mozapp')[0]);

requesting it /from/ the parent.  But maybe "Set the window's isApp state by
asking our parent if our iframe has the 'mozapp' attribute."

>-  _observeInProcessBrowserFrameShown: function(frameLoader, isMozApp) {
>+  _observeInProcessBrowserFrameShown: function(frameLoader, data) {
>     debug("In-process browser frame shown " + frameLoader);
>-    this._setUpMessageManagerListeners(frameLoader, isMozApp);
>+    this._setUpMessageManagerListeners(frameLoader, data);
>   },
> 
>-  _observeRemoteBrowserFrameShown: function(frameLoader, isMozApp) {
>+  _observeRemoteBrowserFrameShown: function(frameLoader, data) {
>     debug("Remote browser frame shown " + frameLoader);
>-    this._setUpMessageManagerListeners(frameLoader, isMozApp);
>+    this._setUpMessageManagerListeners(frameLoader, data);
>   },
> 
>-  _setUpMessageManagerListeners: function(frameLoader, isMozApp) {
>+  _setUpMessageManagerListeners: function(frameLoader, data) {

You don't need any of these data params, right?

>     let frameElement = frameLoader.QueryInterface(Ci.nsIFrameLoader).ownerElement;
>     if (!frameElement) {
>       debug("No frame element?");
>       return;
>     }
> 
>     let mm = frameLoader.messageManager;
> 
>     // Messages we receive are handled by functions with parameters
>     // (frameElement, data), where |data| is the message manager's data object.
> 
>+    let self = this;
>+
>     function addMessageListener(msg, handler) {

Nit: No linebreak.

>-      mm.addMessageListener('browser-element-api:' + msg, handler.bind(this, frameElement));
>+      mm.addMessageListener('browser-element-api:' + msg, handler.bind(self, frameElement));
>     }
> 
>     addMessageListener("hello", this._recvHello);
>     addMessageListener("locationchange", this._fireEventFromMsg);
>     addMessageListener("loadstart", this._fireEventFromMsg);
>     addMessageListener("loadend", this._fireEventFromMsg);
>     addMessageListener("titlechange", this._fireEventFromMsg);
>+    addMessageListener("get-mozapp", this._sendAppState);

Nit: Maybe the message should be 'get-is-mozapp', and the function should be called the same thing?

>@@ -133,31 +130,35 @@ BrowserElementParent.prototype = {
>     }
>     else {
>       evt = new win.Event('mozbrowser' + evtName);
>     }
> 
>     frameElement.dispatchEvent(evt);
>   },
> 
>+  _sendAppState: function(frameElement, data) {
>+    return frameElement.hasAttribute('mozapp');
>+  },
>+
>   observe: function(subject, topic, data) {
>     switch(topic) {
>     case 'app-startup':
>       this._init();
>       break;
>     case NS_PREFBRANCH_PREFCHANGE_TOPIC_ID:
>       if (data == BROWSER_FRAMES_ENABLED_PREF) {
>         this._init();
>       }
>       break;
>     case 'remote-browser-frame-shown':
>-      this._observeRemoteBrowserFrameShown(subject, data == "is-moz-app:true");
>+      this._observeRemoteBrowserFrameShown(subject, data);
>       break;
>     case 'in-process-browser-frame-shown':
>-      this._observeInProcessBrowserFrameShown(subject, data == "is-moz-app:true");
>+      this._observeInProcessBrowserFrameShown(subject, data);

Don't need data here either.

Comment 2 Mounir Lamouri (:mounir) 2012-05-11 13:09:13 PDT

So I haven't changed the |data| because that was here before I changed them to |isMozApp| and I haven't changed the callback name because I have another patch in my queue doing that.

Comment 3 Matt Brubeck (:mbrubeck) 2012-05-12 09:02:40 PDT

https://hg.mozilla.org/mozilla-central/rev/3105e35ffa3f

Comment 4 Justin Lebar (not reading bugmail) 2012-05-15 12:46:59 PDT

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> So I haven't changed the |data| because that was here before I changed them
> to |isMozApp| and I haven't changed the callback name because I have another
> patch in my queue doing that.

In the future, please don't do this unless you're going to land all at once.  I (along with others) am hacking on this file extensively, and it's confusing when you land patches with incorrect names.

Comment 5 Justin Lebar (not reading bugmail) 2012-05-15 12:59:53 PDT

> So I haven't changed the |data| because that was here before I changed them
> to |isMozApp|

You actually added the |data| parameter in an earlier patch, but it doesn't really matter; I didn't r+ a patch that didn't remove the parameter.

I'd like you to remove that, please.

Comment 6 Mounir Lamouri (:mounir) 2012-05-15 14:29:28 PDT

Sorry, I really thought I didn't add the parameters so I thought your comment wasn't applying but I was actually wrong. I pushed in m-i a follow-up to fix that.

Comment 7 Ed Morley [:emorley] 2012-05-16 03:34:03 PDT

https://hg.mozilla.org/mozilla-central/rev/606f80ee71a4