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
|
||
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.
Comment 7•13 years ago
|
||
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
•