770532 - Make new nsIPrincipal and nsIDocShell attributes work in content process

Status: RESOLVED FIXED

(Core :: General, defect)

Comment 2

[Mounir Lamouri (:mounir)]

Getting the appId and the browserParentURI is currintly done by traversing the parent chain. This will not work with e10s. The information will have to be carried another way.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Boilerplate and App ID being carried over

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Carry browser parent URI

Comment 5

[Justin Lebar (not reading bugmail)]

As before, one problem is that the browserParentURI (browserEmbedderURI is sounding better and better) can change.  And you now have different behavior wrt in- and out-of-process <iframe mozbrowser> -- with in-process, the getBrowserParentURI will reflect the embedder's current URI, and with out-of-process, it will reflect the URI of the embedder when the embedder created us.

Can you just rip out the parent walking and always use a cached copy of the embedder's origin?

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 639131 [details] [diff] [review]
Part 1 - Boilerplate and App ID being carried over

># HG changeset patch
># Parent 444e5726cf2ff0287411bc50499debf293a4002d
># User Mounir Lamouri <[e-mail]>
># Date 1341420727 -7200

The big change I think is necessary here is moving from embedder URL to embedder origin.

Also, whatever we decide to call browserParentURI (browserEmbedderOrigin?), I'd appreciate if we were consistent with variable names (i.e., no cheating with "browserURI").

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -1924,20 +1924,43 @@ nsFrameLoader::TryRemoteBrowser()
>   nsCOMPtr<nsIXULWindow> window(do_GetInterface(parentOwner));
>   if (!window) {
>     return false;
>   }
>   if (NS_FAILED(window->GetChromeFlags(&chromeFlags))) {
>     return false;
>   }
> 
>+  PRUint32 appId = 0;

NO_APP_ID!!

>+  nsCOMPtr<nsIURI> browserURI;
>+
>+  if (OwnerIsBrowserFrame()) {
>+    if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozapp)) {
>+      nsAutoString manifest;
>+      mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::mozapp, manifest);
>+
>+      nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
>+      if (!appsService) {
>+        NS_ERROR("Apps Service is not available!");
>+        return NS_ERROR_FAILURE;
>+      }
>+
>+      appsService->GetAppLocalIdByManifestURL(manifest, &appId);
>+    }
>+
>+    if (appId == nsIDocShell::NO_APP_ID) {
>+      parentAsWebNav->GetCurrentURI(getter_AddRefs(browserURI));
>+    }
>+  }

Please explain what's going on here: An <iframe mozbrowser> has /either/ an appId or a browserEmbedderURI.

>   ContentParent* parent = ContentParent::GetNewOrUsed();
>   NS_ASSERTION(parent->IsAlive(), "Process parent should be alive; something is very wrong!");
>   mRemoteBrowser = parent->CreateTab(chromeFlags,
>-                                     /* aIsBrowserFrame = */ OwnerIsBrowserFrame());
>+                                     /* aBrowserURI = */ browserURI,
>+                                     /* aAppId = */ appId);

You don't need those comments there; it's totally clear what the params mean.

>diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
>--- a/dom/ipc/ContentParent.h
>+++ b/dom/ipc/ContentParent.h
>@@ -49,20 +49,20 @@ public:
>     NS_DECL_ISUPPORTS
>     NS_DECL_NSIOBSERVER
>     NS_DECL_NSITHREADOBSERVER
>     NS_DECL_NSIDOMGEOPOSITIONCALLBACK
> 
>     /**
>      * Create a new tab.
>      *
>-     * |aIsBrowserFrame| indicates whether this tab is part of an
>-     * <iframe mozbrowser>.
>+     * |aBrowserURI| indicates the URI of the browser containing the tab.
>+     * |aAppId| indicates in which app the tab is inside.
>      */
>-    TabParent* CreateTab(PRUint32 aChromeFlags, bool aIsBrowserFrame);
>+    TabParent* CreateTab(PRUint32 aChromeFlags, nsIURI* aBrowserURI, PRUint32 aAppId);

Please explain this API better, because it's really tricky: If aAppID is
NO_APP_ID, then we're not an app.  If we are an app, aBrowserURI must be null.
If aBrowserURI (shouldn't this be "browser{Parent/Embedder}URI"?) is null, then
we're not a browser; if we are a browser, aAppId must be NO_APP_ID.

>-TabChild::TabChild(PRUint32 aChromeFlags, bool aIsBrowserFrame)
>+TabChild::TabChild(PRUint32 aChromeFlags, const URI& aBrowserURI,
>+                   PRUint32 aAppId)
>   : mRemoteFrame(nsnull)
>   , mTabChildGlobal(nsnull)
>   , mChromeFlags(aChromeFlags)
>   , mOuterRect(0, 0, 0, 0)
>   , mLastBackgroundColor(NS_RGB(255, 255, 255))
>   , mDidFakeShow(false)
>-  , mIsBrowserFrame(aIsBrowserFrame)
>+  , mBrowserURI(aBrowserURI)
>+  , mAppId(aAppId)
> {
>     printf("creating %d!\n", NS_IsMainThread());
> }

Please assert that we don't have both a browserURI (browserEmbedderURI) and an appId.

>@@ -329,27 +331,20 @@ TabChild::ProvideWindow(nsIDOMWindow* aP
>                         const nsACString& aFeatures, bool* aWindowIsNew,
>                         nsIDOMWindow** aReturn)
> {
>     *aReturn = nsnull;
> 
>     // If aParent is inside an <iframe mozbrowser> and this isn't a request to
>     // open a modal-type window, we're going to create a new <iframe mozbrowser>
>     // and return its window here.
>-    nsCOMPtr<nsIDocShell> docshell = do_GetInterface(aParent);
>-    bool inBrowserFrame = false;
>-    if (docshell) {
>-      docshell->GetContainedInBrowserFrame(&inBrowserFrame);
>-    }

Can we remove nsDocShell::GetContainedInBrowserFrame and supporting code?

>-    if (inBrowserFrame &&
>+    if ((mBrowserURI || mAppId != 0) ||

NO_APP_ID

>         !(aChromeFlags & (nsIWebBrowserChrome::CHROME_MODAL |
>                           nsIWebBrowserChrome::CHROME_OPENAS_DIALOG |
>                           nsIWebBrowserChrome::CHROME_OPENAS_CHROME))) {
>-
>       // Note that BrowserFrameProvideWindow may return NS_ERROR_ABORT if the
>       // open window call was canceled.  It's important that we pass this error
>       // code back to our caller.
>       return BrowserFrameProvideWindow(aParent, aURI, aName, aFeatures,
>                                        aWindowIsNew, aReturn);
>     }
> 
>     // Otherwise, create a new top-level window.
>@@ -373,17 +368,18 @@ TabChild::BrowserFrameProvideWindow(nsID
>                                     bool* aWindowIsNew,
>                                     nsIDOMWindow** aReturn)
> {
>   *aReturn = nsnull;
> 
>   nsRefPtr<TabChild> newChild =
>     static_cast<TabChild*>(Manager()->SendPBrowserConstructor(
>       /* aChromeFlags = */ 0,
>-      /* aIsBrowserFrame = */ true));
>+      /* aBrowserURI = */ IPC::URI(mBrowserURI),
>+      /* aAppId = */ mAppId));

Comments aren't necessary.

>@@ -570,16 +566,25 @@ TabChild::RecvShow(const nsIntSize& size
>         // into the parent-process's layer system.  That's not a fatal
>         // error.
>         return true;
>     }
> 
>     baseWindow->InitWindow(0, mWidget,
>                            0, 0, size.width, size.height);
>     baseWindow->Create();
>+
>+    nsCOMPtr<nsIWebBrowser> webBrowser = do_QueryInterface(mWebNav);
>+    MOZ_ASSERT(webBrowser);
>+
>+    if (webBrowser) {
>+      webBrowser->SetAppId(mAppId);
>+      webBrowser->SetBrowserURI(mBrowserURI);
>+    }
>+
>     baseWindow->SetVisibility(true);
> 
>     // IPC uses a WebBrowser object for which DNS prefetching is turned off
>     // by default. But here we really want it, so enable it explicitly
>     nsCOMPtr<nsIWebBrowserSetup> webBrowserSetup = do_QueryInterface(baseWindow);
>     if (webBrowserSetup) {
>       webBrowserSetup->SetProperty(nsIWebBrowserSetup::SETUP_ALLOW_DNS_PREFETCH,
>                                    true);
>@@ -946,17 +951,17 @@ TabChild::InitTabChildGlobal()
> 
>   scope->Init();
> 
>   nsCOMPtr<nsPIWindowRoot> root = do_QueryInterface(chromeHandler);
>   NS_ENSURE_TRUE(root, false);
>   root->SetParentTarget(scope);
> 
>   // Initialize the child side of the browser element machinery, if appropriate.
>-  if (mIsBrowserFrame) {
>+  if (mBrowserURI || mAppId != 0) {

NO_APP_ID

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h
>--- a/dom/ipc/TabChild.h
>+++ b/dom/ipc/TabChild.h
>@@ -138,20 +138,21 @@ class TabChild : public PBrowserChild,
>                  public nsITabChild
> {
>     typedef mozilla::layout::RenderFrameChild RenderFrameChild;
> 
> public:
>     /**
>      * Create a new TabChild object.
>      *
>-     * |aIsBrowserFrame| indicates whether the TabChild is inside an
>-     * <iframe mozbrowser>.
>+     * |aBrowserURI| indicates the URI of the browser, ie the frame containing
>+     * the <iframe mozbrowser>.
>+     * |aAppId| indicates the app id of the app containing this tab.
>      */
>-    TabChild(PRUint32 aChromeFlags, bool aIsBrowserFrame);
>+    TabChild(PRUint32 aChromeFlags, const URI& aBrowserURI, PRUint32 aAppId);

Similarly update comment here.

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 639143 [details] [diff] [review]
Part 2 - Carry browser parent URI

This patch is fine, but I'd like to look at the whole queue again.

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #5)
> As before, one problem is that the browserParentURI (browserEmbedderURI is
> sounding better and better) can change.  And you now have different behavior
> wrt in- and out-of-process <iframe mozbrowser> -- with in-process, the
> getBrowserParentURI will reflect the embedder's current URI, and with
> out-of-process, it will reflect the URI of the embedder when the embedder
> created us.
> 
> Can you just rip out the parent walking and always use a cached copy of the
> embedder's origin?

That is something I was expecting to do as a follow-up. We have indeed kind of two paths but both are working. We could write tests based on that and then refactor.
Does that plan sounds good to you?

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #6)
> >diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
> >--- a/content/base/src/nsFrameLoader.cpp
> >+++ b/content/base/src/nsFrameLoader.cpp
> >@@ -1924,20 +1924,43 @@ nsFrameLoader::TryRemoteBrowser()
> >   nsCOMPtr<nsIXULWindow> window(do_GetInterface(parentOwner));
> >   if (!window) {
> >     return false;
> >   }
> >   if (NS_FAILED(window->GetChromeFlags(&chromeFlags))) {
> >     return false;
> >   }
> > 
> >+  PRUint32 appId = 0;
> 
> NO_APP_ID!!

Actually, it feels weird that NO_APP_ID is defined in nsIDocShell and nsISecurityManager. I wonder if we should define that in a WebApp-related interface. But, that would make docshell and securitymanager to depend on it.

> >+  nsCOMPtr<nsIURI> browserURI;
> >+
> >+  if (OwnerIsBrowserFrame()) {
> >+    if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozapp)) {
> >+      nsAutoString manifest;
> >+      mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::mozapp, manifest);
> >+
> >+      nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
> >+      if (!appsService) {
> >+        NS_ERROR("Apps Service is not available!");
> >+        return NS_ERROR_FAILURE;
> >+      }
> >+
> >+      appsService->GetAppLocalIdByManifestURL(manifest, &appId);
> >+    }
> >+
> >+    if (appId == nsIDocShell::NO_APP_ID) {
> >+      parentAsWebNav->GetCurrentURI(getter_AddRefs(browserURI));
> >+    }
> >+  }
> 
> Please explain what's going on here: An <iframe mozbrowser> has /either/ an
> appId or a browserEmbedderURI.

You mean that should contain |else|? Indeed.

> >diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
> >--- a/dom/ipc/ContentParent.h
> >+++ b/dom/ipc/ContentParent.h
> >@@ -49,20 +49,20 @@ public:
> >     NS_DECL_ISUPPORTS
> >     NS_DECL_NSIOBSERVER
> >     NS_DECL_NSITHREADOBSERVER
> >     NS_DECL_NSIDOMGEOPOSITIONCALLBACK
> > 
> >     /**
> >      * Create a new tab.
> >      *
> >-     * |aIsBrowserFrame| indicates whether this tab is part of an
> >-     * <iframe mozbrowser>.
> >+     * |aBrowserURI| indicates the URI of the browser containing the tab.
> >+     * |aAppId| indicates in which app the tab is inside.
> >      */
> >-    TabParent* CreateTab(PRUint32 aChromeFlags, bool aIsBrowserFrame);
> >+    TabParent* CreateTab(PRUint32 aChromeFlags, nsIURI* aBrowserURI, PRUint32 aAppId);
> 
> Please explain this API better, because it's really tricky: If aAppID is
> NO_APP_ID, then we're not an app.  If we are an app, aBrowserURI must be
> null.
> If aBrowserURI (shouldn't this be "browser{Parent/Embedder}URI"?) is null,
> then
> we're not a browser; if we are a browser, aAppId must be NO_APP_ID.

Hmmm, actually, if you are a browser tab, you should get an AppID. Otherwise, browser tabs from different apps will share the same data. Will double-check that.

> >@@ -329,27 +331,20 @@ TabChild::ProvideWindow(nsIDOMWindow* aP
> >                         const nsACString& aFeatures, bool* aWindowIsNew,
> >                         nsIDOMWindow** aReturn)
> > {
> >     *aReturn = nsnull;
> > 
> >     // If aParent is inside an <iframe mozbrowser> and this isn't a request to
> >     // open a modal-type window, we're going to create a new <iframe mozbrowser>
> >     // and return its window here.
> >-    nsCOMPtr<nsIDocShell> docshell = do_GetInterface(aParent);
> >-    bool inBrowserFrame = false;
> >-    if (docshell) {
> >-      docshell->GetContainedInBrowserFrame(&inBrowserFrame);
> >-    }
> 
> Can we remove nsDocShell::GetContainedInBrowserFrame and supporting code?

I was planning to do that after we have tests.

Comment 10

[Justin Lebar (not reading bugmail)]

> Actually, it feels weird that NO_APP_ID is defined in nsIDocShell and nsISecurityManager. I wonder if we should define that in a WebApp-related interface. But, that would make docshell and securitymanager to depend on it.

That would be fine with me.  Or we could just use 0 everywhere.

> > >+  nsCOMPtr<nsIURI> browserURI;
> > >+
> > >+  if (OwnerIsBrowserFrame()) {
> > >+    if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozapp)) {
> > >+      nsAutoString manifest;
> > >+      mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::mozapp, manifest);
> > >+
> > >+      nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
> > >+      if (!appsService) {
> > >+        NS_ERROR("Apps Service is not available!");
> > >+        return NS_ERROR_FAILURE;
> > >+      }
> > >+
> > >+      appsService->GetAppLocalIdByManifestURL(manifest, &appId);
> > >+    }
> > >+
> > >+    if (appId == nsIDocShell::NO_APP_ID) {
> > >+      parentAsWebNav->GetCurrentURI(getter_AddRefs(browserURI));
> > >+    }
> > >+  }
> > 
> > Please explain what's going on here: An <iframe mozbrowser> has /either/ an
> > appId or a browserEmbedderURI.
> 
> You mean that should contain |else|? Indeed.

Well, if I have <iframe mozbrowser mozapp="http://this.is.not.an.app">, should that be considered an <iframe mozbrowser>, or a regular <iframe>?  |else| would imply the latter.  I'm not sure what the right thing is, but we need to be consistent with in- and out-of-process.

> > Please explain this API better, because it's really tricky: If aAppID is
> > NO_APP_ID, then we're not an app.  If we are an app, aBrowserURI must be
> > null.
> > If aBrowserURI (shouldn't this be "browser{Parent/Embedder}URI"?) is null,
> > then
> > we're not a browser; if we are a browser, aAppId must be NO_APP_ID.
> 
> Hmmm, actually, if you are a browser tab, you should get an AppID. Otherwise, browser tabs from different apps will share the same data. Will double-check that.

Ah, you're right.  I think the code works as advertised.

> > Can we remove nsDocShell::GetContainedInBrowserFrame and supporting code?
> 
> I was planning to do that after we have tests.

Okay.  I still am not really comfortable r+'ing a patch with pending code removal when it's not clear that the code removal will not require more changes to the new interface.  But if you're confident that the current interface is right, then I'll go with it this time.

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 12

[Justin Lebar (not reading bugmail)]

># HG changeset patch
># Parent 55b726ed033839ad7c3512bcc879c5550d1e9c96
># User Mounir Lamouri <[e-mail]>
># Date 1342815076 25200
>
>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -1966,25 +1966,48 @@ nsFrameLoader::TryRemoteBrowser()
>   nsCOMPtr<nsIXULWindow> window(do_GetInterface(parentOwner));
>   if (!window) {
>     return false;
>   }
>   if (NS_FAILED(window->GetChromeFlags(&chromeFlags))) {
>     return false;
>   }
> 
>+  PRUint32 appId = 0;
>+  bool isBrowserElement = false;
>+
>+  if (OwnerIsBrowserFrame()) {
>+    isBrowserElement = true;
>+
>+    if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozapp)) {
>+      nsAutoString manifest;
>+      mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::mozapp, manifest);
>+
>+      nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
>+      if (!appsService) {
>+        NS_ERROR("Apps Service is not available!");
>+        return NS_ERROR_FAILURE;
>+      }
>+
>+      appsService->GetAppLocalIdByManifestURL(manifest, &appId);
>+
>+      if (appId == nsIScriptSecurityManager::NO_APP_ID) {
>+        isBrowserElement = false;
>+      }

Please put a comment explaining what you're doing.  I'd personally just leave
this out, but whatever, we're going to change it all anyway.

>+    }
>+  }

We still need to clean this up so that OwnerIsBrowserFrame follows our new set
of conventions, but..later, I guess.  :)

>diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
>--- a/dom/ipc/ContentParent.h
>+++ b/dom/ipc/ContentParent.h
>@@ -64,20 +64,21 @@ public:
>     NS_DECL_ISUPPORTS
>     NS_DECL_NSIOBSERVER
>     NS_DECL_NSITHREADOBSERVER
>     NS_DECL_NSIDOMGEOPOSITIONCALLBACK
> 
>     /**
>      * Create a new tab.
>      *
>-     * |aIsBrowserFrame| indicates whether this tab is part of an
>+     * |aIsBrowserElement| indicates whether this tab is part of an
>      * <iframe mozbrowser>.
>+     * |aAppId| indicates in which app the tab is inside.

Should be either "in which the app is", or (much better) "which app the tab is inside," or even "which app the tab belongs to".

(The "in which X the Y is" construction is designed to avoid ending the sentence with a preposition, but, as Churchill is believed to have said, that rule "is the sort of bloody nonsense up with which I will not put.")

Also, make this consistent with the same comment you have later in this patch.  :)

>-    TabParent* CreateTab(PRUint32 aChromeFlags, bool aIsBrowserFrame);
>+    TabParent* CreateTab(PRUint32 aChromeFlags, bool aIsBrowserElement, PRUint32 aAppId);

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>--- a/dom/ipc/TabChild.cpp
>+++ b/dom/ipc/TabChild.cpp
>@@ -83,24 +83,26 @@ ContentListener::HandleEvent(nsIDOMEvent
> class ContentDialogChild : public PContentDialogChild
> {
> public:
>   virtual bool Recv__delete__(const InfallibleTArray<int>& aIntParams,
>                               const InfallibleTArray<nsString>& aStringParams);
> };
> 
> 
>-TabChild::TabChild(PRUint32 aChromeFlags, bool aIsBrowserFrame)
>+TabChild::TabChild(PRUint32 aChromeFlags, const bool& aIsBrowserElement,
>+                   PRUint32 aAppId)

Nit: Why is it |const bool&|, anyway?  Can we just make it a normal bool, like
the PRUint32 you're adding?

>   : mRemoteFrame(nsnull)
>   , mTabChildGlobal(nsnull)
>   , mChromeFlags(aChromeFlags)
>   , mOuterRect(0, 0, 0, 0)
>   , mLastBackgroundColor(NS_RGB(255, 255, 255))
>   , mDidFakeShow(false)
>-  , mIsBrowserFrame(aIsBrowserFrame)
>+  , mIsBrowserElement(aIsBrowserElement)
>+  , mAppId(aAppId)
> {
>     printf("creating %d!\n", NS_IsMainThread());
> }
> 
> nsresult
> TabChild::Init()
> {
>   nsCOMPtr<nsIWebBrowser> webBrowser = do_CreateInstance(NS_WEBBROWSER_CONTRACTID);
>@@ -579,16 +580,27 @@ TabChild::RecvShow(const nsIntSize& size
>         // into the parent-process's layer system.  That's not a fatal
>         // error.
>         return true;
>     }
> 
>     baseWindow->InitWindow(0, mWidget,
>                            0, 0, size.width, size.height);
>     baseWindow->Create();
>+
>+    nsCOMPtr<nsIWebBrowser> webBrowser = do_QueryInterface(mWebNav);
>+    MOZ_ASSERT(webBrowser);
>+
>+    if (webBrowser) {
>+      webBrowser->SetAppId(mAppId);
>+      if (mIsBrowserElement) {
>+        webBrowser->SetIsBrowserElement();
>+      }
>+    }

When we create the BrowserElementChild, we set isBrowser and the app manifest.

You should decide whether you want this to be in C++ or in JS.  From our IRC conversation, it sounds like you want to do it in JS, in which case this change (and also the webbrowser IDL changes?) isn't necessary.

r- until we can clarify this issue.

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h
>--- a/dom/ipc/TabChild.h
>+++ b/dom/ipc/TabChild.h
>@@ -142,20 +142,20 @@ class TabChild : public PBrowserChild,
>                  public nsITabChild
> {
>     typedef mozilla::layout::RenderFrameChild RenderFrameChild;
> 
> public:
>     /**
>      * Create a new TabChild object.
>      *
>-     * |aIsBrowserFrame| indicates whether the TabChild is inside an
>-     * <iframe mozbrowser>.
>+     * |aIsBrowserElement| indicates if the tab is inside an <iframe mozbrowser>.
>+     * |aAppId| indicates the app id of the app containing this tab.

"indicates if" is a bit awkward to my ear ("indicates whether" is more
idiomatic), but it's fine if you prefer it.

"indicates the app id", however, is too far afield; it should be "is the app
id", or even just "is the id".  Also, please specify how to indicate that the
TabChild is not in an app.

>-    TabChild(PRUint32 aChromeFlags, bool aIsBrowserFrame);
>+    TabChild(PRUint32 aChromeFlags, const bool& aIsBrowserElement, PRUint32 aAppId);

>diff --git a/embedding/browser/webBrowser/nsIWebBrowser.idl b/embedding/browser/webBrowser/nsIWebBrowser.idl
>--- a/embedding/browser/webBrowser/nsIWebBrowser.idl
>+++ b/embedding/browser/webBrowser/nsIWebBrowser.idl
>-[scriptable, uuid(33e9d001-caab-4ba9-8961-54902f197202)]
>+[scriptable, uuid(60e7e4a3-234e-408e-8508-41ebe880dec5)]
> interface nsIWebBrowser : nsISupports
> {
>     /**
>      * Registers a listener of the type specified by the iid to receive
>      * callbacks. The browser stores a weak reference to the listener
>      * to avoid any circular dependencies.
>      * Typically this method will be called to register an object
>      * to receive <CODE>nsIWebProgressListener</CODE> or 
>@@ -138,9 +139,12 @@ interface nsIWebBrowser : nsISupports
>      * enough that we want to avoid certain optimizations like discarding
>      * decoded image data and throttling the refresh driver. In Firefox,
>      * this corresponds to the visible tab.
>      *
>      * Defaults to true. For optimal performance, set it to false when
>      * appropriate.
>      */
>     attribute boolean isActive;
>+
>+    [noscript] void setAppId(in unsigned long appId);
>+    [noscript] void setIsBrowserElement();

Could you please comment that these do the same things as their corresponding nsIDocShell methods?

It's called setIsBrowser in nsIDocShell, so it should be the same here.  Feel free to change either; setIsBrowserElement is probably better, because it's already nsIDocShell::isBrowserElement.

Neither of these should be noscript, IMO, and we should change docshell to match.  But at the very least, we should be consistent and make setIsBrowserElement callable from script.

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 644436 [details] [diff] [review]
Patch

er, that's r-, per the comments.

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #12)
> >   : mRemoteFrame(nsnull)
> >   , mTabChildGlobal(nsnull)
> >   , mChromeFlags(aChromeFlags)
> >   , mOuterRect(0, 0, 0, 0)
> >   , mLastBackgroundColor(NS_RGB(255, 255, 255))
> >   , mDidFakeShow(false)
> >-  , mIsBrowserFrame(aIsBrowserFrame)
> >+  , mIsBrowserElement(aIsBrowserElement)
> >+  , mAppId(aAppId)
> > {
> >     printf("creating %d!\n", NS_IsMainThread());
> > }
> > 
> > nsresult
> > TabChild::Init()
> > {
> >   nsCOMPtr<nsIWebBrowser> webBrowser = do_CreateInstance(NS_WEBBROWSER_CONTRACTID);
> >@@ -579,16 +580,27 @@ TabChild::RecvShow(const nsIntSize& size
> >         // into the parent-process's layer system.  That's not a fatal
> >         // error.
> >         return true;
> >     }
> > 
> >     baseWindow->InitWindow(0, mWidget,
> >                            0, 0, size.width, size.height);
> >     baseWindow->Create();
> >+
> >+    nsCOMPtr<nsIWebBrowser> webBrowser = do_QueryInterface(mWebNav);
> >+    MOZ_ASSERT(webBrowser);
> >+
> >+    if (webBrowser) {
> >+      webBrowser->SetAppId(mAppId);
> >+      if (mIsBrowserElement) {
> >+        webBrowser->SetIsBrowserElement();
> >+      }
> >+    }
> 
> When we create the BrowserElementChild, we set isBrowser and the app
> manifest.
> 
> You should decide whether you want this to be in C++ or in JS.  From our IRC
> conversation, it sounds like you want to do it in JS, in which case this
> change (and also the webbrowser IDL changes?) isn't necessary.
> 
> r- until we can clarify this issue.

Actually, I thought about this again and I would prefer an early set. Also, we can't get to the webapps service from the child so we can't have setAppId() there.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 16

[Justin Lebar (not reading bugmail)]

> Actually, I thought about this again and I would prefer an early set.

Me too, for what little it's worth.  :)

Comment 17

[Justin Lebar (not reading bugmail)]

+      // If the frame is actually an app, we should not mark it as a browser.
+      if (appId == nsIScriptSecurityManager::NO_APP_ID) {
+        isBrowserElement = false;
+      }

This comment does not match the code.  What are you trying to do?

Comment 18

[Justin Lebar (not reading bugmail)]

What about changes to BrowserElementChild so it doesn't set the appID and the isBrowserElement flag?

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #18)
> What about changes to BrowserElementChild so it doesn't set the appID and
> the isBrowserElement flag?

For isBrowserElement, this is going to be a follow-up. Simple, just need to be tested.
For appId, it's harder because we will have to change the callers of the nsGlobalWindow::IsPartOfApp. It's not yet a high priority.

Comment 20

[Justin Lebar (not reading bugmail)]

+    nsCOMPtr<nsIWebBrowser> webBrowser = do_QueryInterface(mWebNav);
+    MOZ_ASSERT(webBrowser);
+
+    if (webBrowser) {
+      webBrowser->SetAppId(mAppId);
+      if (mIsBrowserElement) {
+        webBrowser->SetIsBrowserElement();
+      }
+    }

If we're going to keep this in C++, can we just getInterface to nsIDocShell?

Comment 21

[Justin Lebar (not reading bugmail)]

Comment on attachment 644507 [details] [diff] [review]
Patch

r=me, but can we clarify comment 17 before you land, please?  I think basically any behavior is fine for now, although the behavior in the comment is better than the behavior in the code, if that works.

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #17)
> +      // If the frame is actually an app, we should not mark it as a
> browser.
> +      if (appId == nsIScriptSecurityManager::NO_APP_ID) {
> +        isBrowserElement = false;
> +      }
> 
> This comment does not match the code.  What are you trying to do?

What the comment says ;)

(In reply to Justin Lebar [:jlebar] from comment #20)
> +    nsCOMPtr<nsIWebBrowser> webBrowser = do_QueryInterface(mWebNav);
> +    MOZ_ASSERT(webBrowser);
> +
> +    if (webBrowser) {
> +      webBrowser->SetAppId(mAppId);
> +      if (mIsBrowserElement) {
> +        webBrowser->SetIsBrowserElement();
> +      }
> +    }
> 
> If we're going to keep this in C++, can we just getInterface to nsIDocShell?

It's working. Cool :)

Comment 23

[Justin Lebar (not reading bugmail)]

>> This comment does not match the code.  What are you trying to do?
>
> What the comment says ;)

Cool, that's great then!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c9aa4834f0f