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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(1 file)
5.70 KB,
patch
|
justin.lebar+bug
:
review+
mounir
:
checkin+
|
Details | Diff | Splinter Review |
This should include a review comment not applied in bug 753978.
Attachment #622998 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•13 years ago
|
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 1•13 years ago
|
||
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+
Assignee | ||
Comment 2•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Attachment #622998 -
Flags: checkin+
Comment 3•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3105e35ffa3f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
> 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.
Assignee | ||
Comment 6•13 years ago
|
||
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.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•