Closed Bug 754140 Opened 13 years ago Closed 13 years ago

Set mozapp status on iframe by sync message on the parent instead of injecting a script

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file)

Attached patch Patch v1Splinter Review
This should include a review comment not applied in bug 753978.
Attachment #622998 - Flags: review?(justin.lebar+bug)
Blocks: 754141
Summary: Set mozapp status on iframe by sync message on the parent inside of injecting a script → Set mozapp status on iframe by sync message on the parent instead of injecting a script
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.
Attachment #622998 - Flags: review?(justin.lebar+bug) → review+
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.
Target Milestone: --- → mozilla15
Attachment #622998 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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.
> 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.
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: