Let 3rd-party APP has ability to embed "OPEN WEB APPs” in mozAPP frames. Such as homescreem, lockscreen....etc.

As specified on the dev-webapi mailing list, one of the only thing I can think of that will be an issue is the code at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#228 One issue with just exposing it as if is that apps basically also needs the webapps-manage permission in order to iterate over app manifests to retrieve the list of widgets and serve the right widget based on the locale. An other issue would be that embedding apps may get some informations from the iframe url by reading the src, and some apps may leak some private informations.

Jonas's idea of a new "embed-widgets" permission[1] gets most people's feedback+ on dev-webapi mailing list, so I'd like to modify the title of this bug to reflect the discussion has moved to the new "embed-widgets" permission instead of "embed-apps". Gary will summarize our discussion there and update on this bug, as well as create a wiki page and write down more details, like: widget use cases, and the usage of combining browser and homescreen-webapps-manager permissions ... etc. [1] https://groups.google.com/d/msg/mozilla.dev.webapi/mSkywh1P7L8/Z9_8qQN9pkwJ

I'll try to let Gecko support "embed-widgets" permission and relative function by following steps: Part1: Add a permission "embed-widgets" and an HTMLIFrameElement attribute "mozWidget" Part2: Implementation of checking if a window is able to embed a specific widget. Part3: Enable to embed a widget if preconditions hold Part4: Only limited browser API are available to a widget

Is there any reason for not granting this to all apps by default? It seems that the 'Safer' Browser API is no danger than normal iframe.

WIP patch Part 1.1: Load a widget as an app if the |src| is in the |widgetPages| * * * 1. Add permission |embed-widgets| and Element attribute |mozwidget| 2. Add |hasWidgetPage| in /mozIApplication.idl 3. Check permission |embed-widgets| and the |src| is in the |widgetPages| when |GetAppManifest| * * * Todo List: 1. Test Case 2. |entry_points| in communications app (dialer/contacts...) is not handled. 3. Some behaviour needs to be clairfied. Hello Fabrice, would you be able to feedback the patch? Thanks.

There's an ambiguous behaviour need to be clairfied. In the WIP patch, if the |src| is not listed in the |widgetPages| list, the iframe will be a general iframe, not a widget/app. Is the behaviour reasonable? Or it should be a blank page.

Follow by comment 5 Fixed some symbol error

Comment on attachment 8450898 [details] [diff] [review] Part 1.1: Load a widget as an app if the |src| is in the |widgetPages| Review of attachment 8450898 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsGenericHTMLFrameElement.cpp @@ +361,5 @@ > + } > + > + manifestURL.Assign(appManifestURL); > + isApp = true; > + } while(0); do / while(0) is a bit ugly. Why no do: { ... if (!appManifestURL.IsEmpty()) { manifestURL.Assign(appManifestURL); isApp = true; } @@ +385,5 @@ > + isWidget = true; > + } while(0); > + > + if( (!isApp && !isWidget) /* No valid case */ > + || (isApp && isWidget) /* Conflicted */) { I would log something here. @@ +404,5 @@ > + nsAutoString src; > + if (isWidget) { > + GetAttr(kNameSpaceID_None, nsGkAtoms::src, src); > + > + app->HasWidgetPage(src, &hasWidgetPage); check that HasWidget() succeeds. ::: dom/apps/src/AppsUtils.jsm @@ +58,5 @@ > }, > > + hasWidgetPage: function(aPage) { > + let originURI = Services.io.newURI(this.origin, null, null); > + for(let id in this.widgetPages) { nit: for (...) @@ +688,5 @@ > + get widgetPages() { > + if (this._manifest.widgetPages) { > + return this._manifest.widgetPages; > + } > + return []; hm... Ithink should we support localized widgets. We support that for launch_path, and widgets are similar. ::: dom/apps/src/Webapps.jsm @@ +332,5 @@ > } > let app = this.webapps[aResult.id]; > app.csp = aResult.manifest.csp || ""; > app.role = aResult.manifest.role || ""; > + app.widgetPages = aResult.widgetPages || []; Here and in the other places you create the array: I would rather store the full uris of all the recognized widgets. Not sure how we want to support localization either... That needs to be discussed. ::: dom/interfaces/apps/mozIApplication.idl @@ +18,5 @@ > boolean hasPermission(in string permission); > > + /* Return true if this app can be a widget and > + its |widgetPages| contains |page| */ > + boolean hasWidgetPage(in DOMString page); nit: s/page/pageURL

All, I had tried to migrate the stingray homescreen to adapt this WIP patch of widget API. The following branch is my current work on it: https://github.com/huchengtw-moz/gaia/tree/stingray-widget-api

Since the scope of this WIP patch, I had done the following tests: 1. create a widget iframe with mozwidget attribute works as expected 2. create a widget iframe without mozwidget attribute the iframe becomes normal iframe, as expected. 3. remove the embed-widgets permission from homescreen manifest the iframe becomes normal iframe, as expected. 4. request embed-apps and embed-widgets permission both in homescreen manifest works as expected 5. add both mozwidget and mozapp attributes to iframe the iframe becomes normal iframe, as expected. 6. the src of widget iframe is not in widgetPages the iframe becomes normal iframe, as expected.

BTW, the expected behavior of item 6 of comment 10 is based on current implementation.

I had made a case to test widgetPages, in https://github.com/huchengtw-moz/gaia/tree/stingray-widget-api-iframe-navigation The behavior looks wrong: 1. create a widget whose src is not in widgetPages and navigate to page who is in widgetPages Result: all pages didn't get permissions. 2. create a widget whose src is in widgetPages and navigate to page who is not in widgetPages Result: all pages got permissions.

(In reply to Fabrice Desré [:fabrice] from comment #8) > Comment on attachment 8450898 [details] [diff] [review] > Part 1.1: Load a widget as an app if the |src| is in the |widgetPages| > > Review of attachment 8450898 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/html/content/src/nsGenericHTMLFrameElement.cpp > @@ +361,5 @@ > > + } > > + > > + manifestURL.Assign(appManifestURL); > > + isApp = true; > > + } while(0); > > do / while(0) is a bit ugly. Why no do: > { > ... > if (!appManifestURL.IsEmpty()) { > manifestURL.Assign(appManifestURL); > isApp = true; > } > The reason of using |do / while(0)| is multiple |breaks|. i.e. |isApp=true| if multiple conditions hold. AFAIK, alternative choices are 1. nested if blocks: hard to trace 2. goto: infrequently used in mozilla 3. check all the conditions first; finally check if all conditions hold. ==> performance issue I am wondering which one is prefered. > ::: dom/apps/src/Webapps.jsm > @@ +332,5 @@ > > } > > let app = this.webapps[aResult.id]; > > app.csp = aResult.manifest.csp || ""; > > app.role = aResult.manifest.role || ""; > > + app.widgetPages = aResult.widgetPages || []; > > Here and in the other places you create the array: I would rather store the > full uris of all the recognized widgets. Not sure how we want to support > localization either... That needs to be discussed. > 1. The main reason of the memory-consuming mechanism is that |getManifestFor| returns a Promise, which is un-synchronizable. http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsService.js#43 Therefore, it's necessary to store all the |widgetPages| as load/update/clone. (Otherwise we need to make Promise sync in cpp) 2. We have discuss a little bit about L10n, which is in the following link. https://wiki.mozilla.org/WebAPI/WidgetAPI#extend_manifest.webapp L10n has not been tested in this patch.

(In reply to Junior Hsu [:juniorhsu] from comment #13) > (In reply to Fabrice Desré [:fabrice] from comment #8) > > Comment on attachment 8450898 [details] [diff] [review] > > Part 1.1: Load a widget as an app if the |src| is in the |widgetPages| > > > > Review of attachment 8450898 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: content/html/content/src/nsGenericHTMLFrameElement.cpp > > @@ +361,5 @@ > > > + } > > > + > > > + manifestURL.Assign(appManifestURL); > > > + isApp = true; > > > + } while(0); > > > > do / while(0) is a bit ugly. Why no do: > > { > > ... > > if (!appManifestURL.IsEmpty()) { > > manifestURL.Assign(appManifestURL); > > isApp = true; > > } > > > The reason of using |do / while(0)| is multiple |breaks|. > i.e. |isApp=true| if multiple conditions hold. > AFAIK, alternative choices are > 1. nested if blocks: hard to trace > 2. goto: infrequently used in mozilla > 3. check all the conditions first; finally check if all conditions hold. > ==> performance issue Or 4. don't inline these blocks, but turn them into functions that will return a boolean assigned to isApp and isWidget. > 1. > The main reason of the memory-consuming mechanism is that |getManifestFor| > returns a Promise, which is un-synchronizable. > http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsService.js#43 > Therefore, it's necessary to store all the |widgetPages| as > load/update/clone. > (Otherwise we need to make Promise sync in cpp) I don't follow you here. You have the manifest already (it's aResult.manifest). And you could just resolve the relative urls against the manifestURL. > 2. > We have discuss a little bit about L10n, which is in the following link. > https://wiki.mozilla.org/WebAPI/WidgetAPI#extend_manifest.webapp > L10n has not been tested in this patch. I'd like that to be figured out before we land this patch.

WIP patch Modifications: 1. Add test case 2. Update according to feedback comment Todo list: 1. |entry_points| in communications app (dialer/contacts...) is not handled 2. Some behaviour should be determined (L10n, src not in widgetPages) 3. Changes in Webapps.jsm should be investigated

Comment on attachment 8454349 [details] [diff] [review] Part 1: Load a widget as an app if the |src| is in the |widgetPages| Review of attachment 8454349 [details] [diff] [review]: ----------------------------------------------------------------- much better! ::: content/html/content/src/nsGenericHTMLFrameElement.cpp @@ +325,5 @@ > > +/* @param AppType: nsGkAtoms::mozapp or nsGkAtoms::mozwidget > + * */ > +void nsGenericHTMLFrameElement::GetManifestURLByType(nsIAtom *aAppType, > + bool *aIsOfAppType, the parameter names are a bit confusing I think. aOut should be aManifestURL @@ +340,5 @@ > + nsCOMPtr<nsIPermissionManager> permMgr = > + services::GetPermissionManager(); > + NS_ENSURE_TRUE_VOID(permMgr); > + nsIPrincipal *principal = NodePrincipal(); > + const char* aPermissionType = (aAppType==nsGkAtoms::mozapp) ? nit: spaces around == ::: dom/apps/src/AppsUtils.jsm @@ +60,5 @@ > + hasWidgetPage: function(aPageURL) { > + let originURI = Services.io.newURI(this.origin, null, null); > + for (let id in this.widgetPages) { > + let widgetPage = this.widgetPages[id]; > + if(widgetPage && originURI.resolve(widgetPage) === aPageURL) { That is is the critical path of nsGenericHTMLFrameElement::GetAppManifestURL() and you could save the uri resolution if you stored the resolved path (they don't change) during execution. ::: dom/apps/tests/test_widget.html @@ +79,5 @@ > + // Preferences > + function() { > + SpecialPowers.pushPrefEnv({"set": [["dom.datastore.enabled", true], > + ["dom.testing.ignore_ipc_principal", true], > + ["dom.testing.datastore_enabled_for_hosted_apps", true]]}, runTest); why do we need these datastore prefs?

WIP patch Update according to feedback comment 14, mostly in WebApps.jsm Todo list: Some behaviour should be determined (L10n, src not in widgetPages)

Hi Fabrice, I'm thinking if we can separate l10n support into a follow-up bug? Gaia developers and partners want a platform for their widget development as early as possible.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #18) > Hi Fabrice, I'm thinking if we can separate l10n support into a follow-up > bug? Gaia developers and partners want a platform for their widget > development as early as possible. This should not be hard to decide right? We already have a l10n system in manifests. You can reuse that, or if you don't want tell me why.

Comment on attachment 8456080 [details] [diff] [review] Part 1: Load a widget as an app if the |src| is in the |widgetPages| Review of attachment 8456080 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsGenericHTMLFrameElement.cpp @@ +341,5 @@ > + services::GetPermissionManager(); > + NS_ENSURE_TRUE_VOID(permMgr); > + nsIPrincipal *principal = NodePrincipal(); > + const char* aPermissionType = (aAppType == nsGkAtoms::mozapp) ? > + "embed-apps" : "embed-widgets"; nit: check the style guide or other uses of the ternary operator in this file. My preference is usually to align '?' and ':' like: const char* aPermissionType = (aAppType == nsGkAtoms::mozapp) ? "embed-apps" : "embed-widgets"; ::: dom/apps/src/Webapps.jsm @@ +308,5 @@ > } > return res.length > 0 ? res : null; > }, > > + _saveFullPathOfWidgetPages: function(aOrigin, aManifest, aDestApp) { Please resolve the uris against the manifest url instead of the origin. Also, rename the method to _saveWidgetsFullPath() @@ +309,5 @@ > return res.length > 0 ? res : null; > }, > > + _saveFullPathOfWidgetPages: function(aOrigin, aManifest, aDestApp) { > + var originURI = Services.io.newURI(aOrigin, null, null); s/var/let @@ +340,5 @@ > } > let app = this.webapps[aResult.id]; > app.csp = aResult.manifest.csp || ""; > app.role = aResult.manifest.role || ""; > + this._saveFullPathOfWidgetPages(app.origin, aResult.manifest, app); the manifest here is not wrapped in a ManifestHelper(), so you won't get locale processing. @@ +968,5 @@ > > app.name = manifest.name; > app.csp = manifest.csp || ""; > app.role = localeManifest.role; > + this._saveFullPathOfWidgetPages(app.origin, manifest, app); you need to use localeManifest here. @@ +2385,5 @@ > appObject.basePath = OS.Path.dirname(this.appsFile); > appObject.name = aManifest.name; > appObject.csp = aLocaleManifest.csp || ""; > appObject.role = aLocaleManifest.role; > + appObject.widgetPages = aManifest.widgetPages || []; s/aManifest/aLocaleManifest. But is that even correct to not use _saveFullPathOfWidgetPages here ? @@ +2421,5 @@ > app.name = aManifest.name; > > app.csp = aManifest.csp || ""; > > + app.widgetPages = aManifest.widgetPages || []; same here

(In reply to Fabrice Desré [:fabrice] from comment #19) > This should not be hard to decide right? We already have a l10n system in > manifests. You can reuse that, or if you don't want tell me why. Hi Fabrice, The benefits for spliting some efforts into follow up bugs are 1. To land the main core of widget support as early as possible but be disabled by preference. Then partner can help us to do the test based on their use cases. 2. We can find extra resources to fix these follow up bugs in pararell based on core patches here. 3. Junior can more focus on implemeting main core of widget support. This is the suggestion only and respect for reviewer's decision. Thanks.

(In reply to Marco Chen [:mchen] from comment #21) > The benefits for spliting some efforts into follow up bugs are > 1. To land the main core of widget support as early as possible but be > disabled by preference. > Then partner can help us to do the test based on their use cases. > 2. We can find extra resources to fix these follow up bugs in pararell > based on core patches here. > 3. Junior can more focus on implemeting main core of widget support. Anyway all is fine since the latest patch does the right thing to get the localized widgets from manifests.

1. Add permission |embed-widgets| and Element attribute |mozwidget| 2. Add |hasWidgetPage| in mozIApplication.idl 3. Check permission |embed-widgets| and the |src| is in the |widgetPages| when |GetAppManifest| 4. Add test case 5. Should enable preference |dom.enable_widgets| Note. 1. In this patch, if the src is not in the |widgetPages| specified in manifest, the widget is without priviledged/certificated webapi. 2. To enable the widget functionality, preference |dom.enable_widgets| should be enabled.

(In reply to Fabrice Desré [:fabrice] from comment #20) > @@ +2385,5 @@ > > appObject.basePath = OS.Path.dirname(this.appsFile); > > appObject.name = aManifest.name; > > appObject.csp = aLocaleManifest.csp || ""; > > appObject.role = aLocaleManifest.role; > > + appObject.widgetPages = aManifest.widgetPages || []; > > s/aManifest/aLocaleManifest. But is that even correct to not use > _saveFullPathOfWidgetPages here ? > The code snippet is in _cloneApp, which is only called by ConfirmInstall. Originally, it makes more sense to use saveFullPathOfWidgetPages in |ConfirmInstall| IMO. Now I prefer to put saveFullPathOfWidgetPages in _cloneApp since it's error-free to reuse it, just matching your suggestion. To explain more on Comment 23, the behaviour of Note 1 is not determined yet, thus causing me to implement an intuitive one first.

Comment on attachment 8458427 [details] [diff] [review] Part 1: Load a widget as an app if the |src| is in the |widgetPages| Review of attachment 8458427 [details] [diff] [review]: ----------------------------------------------------------------- Very close, but I want to take a last look before r+. Can you also push your patch to the try server? thanks! ::: content/html/content/src/nsGenericHTMLFrameElement.cpp @@ +325,5 @@ > return NS_OK; > } > > +/* @param AppType: nsGkAtoms::mozapp or nsGkAtoms::mozwidget > + * */ nit: strange comment formatting here. @@ +333,5 @@ > +{ > + *aIsOfAppType = false; > + aManifestURL.Truncate(); > + > + if(aAppType != nsGkAtoms::mozapp && aAppType != nsGkAtoms::mozwidget) { nit: here and in many other cases: |if (...)| @@ +381,4 @@ > > + GetManifestURLByType(nsGkAtoms::mozapp, &isApp, appManifestURL); > + > + //Need Pref for Widget Enable nit: // Check if the widget preference is enabled. @@ +381,5 @@ > > + GetManifestURLByType(nsGkAtoms::mozapp, &isApp, appManifestURL); > + > + //Need Pref for Widget Enable > + bool isWidgetEnable = false; s/isWidgetEnable/isWidgetEnabled @@ +390,5 @@ > + > + if (branch) { > + branch->GetBoolPref("dom.enable_widgets", &isWidgetEnable); > + } > + } Instead of using the preference service like that, please use Preferences::GetBool() @@ +397,5 @@ > + GetManifestURLByType(nsGkAtoms::mozwidget, &isWidget, widgetManifestURL); > + } > + > + if(!isApp && !isWidget) { > + /* No valid case to get manifest*/ nit: // No valid case to get manifest. @@ +429,5 @@ > + if (isWidget) { > + GetAttr(kNameSpaceID_None, nsGkAtoms::src, src); > + nsresult rv = app->HasWidgetPage(src, &hasWidgetPage); > + > + if(NS_SUCCEEDED(rv) && !hasWidgetPage) { nit: if (...)

Update according to feedback comment 25 Try Result: https://tbpl.mozilla.org/?tree=Try&rev=e32d60ee9bdc

Comment on attachment 8460156 [details] [diff] [review] Part 1: Load a widget as an app if the |src| is in the |widgetPages| Review of attachment 8460156 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Let's wait for Jonas before landing. Also, did you file followups for the navigation issues? ::: content/html/content/src/nsGenericHTMLFrameElement.cpp @@ +338,5 @@ > + } > + > + // Check permission. > + nsCOMPtr<nsIPermissionManager> permMgr = > + services::GetPermissionManager(); nit: that should fit on a single line. ::: dom/interfaces/apps/mozIApplication.idl @@ +17,5 @@ > /* Return true if this app has |permission|. */ > boolean hasPermission(in string permission); > > + /* Return true if this app can be a widget and > + its |widgetPages| contains |page| */ Nit: Multi line comments are formatted this way: /** * Line 1 * Line 2 */

(In reply to Fabrice Desré [:fabrice] from comment #27) > Comment on attachment 8460156 [details] [diff] [review] > Part 1: Load a widget as an app if the |src| is in the |widgetPages| > > Review of attachment 8460156 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me. Let's wait for Jonas before landing. Also, did you file > followups for the navigation issues? > I just create the bug 1043110. And I will implement the limited BrowserAPI feature first: only limited browser APIs are available to a widget. https://wiki.mozilla.org/WebAPI/WidgetAPI#Limited_Browser_API > ::: content/html/content/src/nsGenericHTMLFrameElement.cpp > @@ +338,5 @@ > > + } > > + > > + // Check permission. > > + nsCOMPtr<nsIPermissionManager> permMgr = > > + services::GetPermissionManager(); > > nit: that should fit on a single line. > > ::: dom/interfaces/apps/mozIApplication.idl > @@ +17,5 @@ > > /* Return true if this app has |permission|. */ > > boolean hasPermission(in string permission); > > > > + /* Return true if this app can be a widget and > > + its |widgetPages| contains |page| */ > > Nit: Multi line comments are formatted this way: > /** > * Line 1 > * Line 2 > */

Hi Jonas, According to one week passed, set needinfo flag to you. And please help to give superreivew on mozWidget. Very thanks.

Sorry for an additional patch modification. The modification is for: 1. consistency signature style of GetManifestURLByType and GetAppManifestURL 2. make GetManifestURLByType more senses to me: move all checking detail to it, leave GetAppManifestURL for high level checks All these changes are in the two mentioned functions in nsGenericHTMLFrameElement. Due to a little logic modification，I set review? again. I also rebase and update according to comment 27

Try result https://tbpl.mozilla.org/?tree=Try&rev=ba3598bb9937 I enable preference dom.enable_widgets in this try test.

a WIP patch I have hidden the methods of Browser API to a widget in the WIP patch, but hiding mozbrowser events is not done yet.

a WIP patch Hide security-sensitive mozbrowser events except: * mozbrowserusernameandpasswordrequired * mozbrowseropenwindow * mozbrowserasyncscroll

Comment on attachment 8464552 [details] [diff] [review] Part 1: Load a widget as an app if the |src| is in the |widgetPages| Review of attachment 8464552 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for not catching that earlier, but I'd like to take a last look at the Webapps.jsm changes. ::: content/html/content/src/nsGenericHTMLFrameElement.cpp @@ +353,5 @@ > + nsAutoString manifestURL; > + GetAttr(kNameSpaceID_None, aAppType, manifestURL); > + if (manifestURL.IsEmpty()) { > + return; > + } you could move this check earlier in the function (before the permission check). ::: dom/apps/src/Webapps.jsm @@ +322,5 @@ > > + _saveWidgetsFullPath: function(aManifest, aDestApp) { > + let manifestURI = Services.io.newURI(aDestApp.manifestURL, null, null); > + if (aManifest.widgetPages) { > + aDestApp.widgetPages = aManifest.widgetPages.map(manifestURI.resolve); that's not correct. You need to build a ManifestHelper object and use its resolveFromOrigin (soon to be resolveURL once bug 1042881 lands) method. @@ +353,5 @@ > let app = this.webapps[aResult.id]; > app.csp = aResult.manifest.csp || ""; > app.role = aResult.manifest.role || ""; > + > + let localeManifest = new ManifestHelper(aResult.manifest, app.origin); Make sure you update that after bug 1042881 (you need to add app.manifestURL as a 3rd parameter). @@ +2505,5 @@ > app.name = aManifest.name; > > app.csp = aManifest.csp || ""; > > + let aLocaleManifest = new ManifestHelper(aManifest, app.origin); here too.

Rebase and update according to comment 34 Try result: https://tbpl.mozilla.org/?tree=Try&rev=f14d29e69c54

Comment on attachment 8466939 [details] [diff] [review] Part 1: Load a widget as an app if the |src| is in the |widgetPages| Review of attachment 8466939 [details] [diff] [review]: ----------------------------------------------------------------- No need to review again, but fix the last nit before landing, thanks! ::: dom/apps/src/Webapps.jsm @@ +325,5 @@ > return res.length > 0 ? res : null; > }, > > + _saveWidgetsFullPath: function(aManifest, aDestApp) { > + let manifestURI = Services.io.newURI(aDestApp.manifestURL, null, null); Remove this as it's unused.

Rebase and fix nit according to comment 36, carry r+

1. Add |ownerIsWidget| in nsIFrameLoader.idl 2. Add |GetReallyIsWidget| in nsIMozBrowserFrame.idl 3. Hide the methods of browser API of a widget 4. Hide security-sensitive mozbrowser events of a widget * * * Todo List: 1. Test Case 2. Modify the test of Part1 since |mozbrowsershowmodalprompt| is not available after this patch

Modify a build time error

Comment on attachment 8469938 [details] [diff] [review] Part 2: Only limited browser API are available to a widget Review of attachment 8469938 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting to Kanru that owns the mozbrowser api.

Modify test: 1. Use message manager instead of alert owing to no mozbrowsershowmodalprompt events. 2. Add testing limited browser api

tbpl result: https://tbpl.mozilla.org/?tree=Try&rev=12d556ee3bc8

Comment on attachment 8471468 [details] [diff] [review] Part 2(v1): Only limited browser API are available to a widget Review of attachment 8471468 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/nsIFrameLoader.idl @@ +286,5 @@ > + > + /** > + * Find out whether the owner content really is a widget > + */ > + readonly attribute boolean ownerIsWidget; How does ownerIsWidget interact with owerIsBrowserOrAppFrame? Could they both be true? Document this. ::: dom/apps/tests/test_widget.html @@ +81,5 @@ > + function ok(p, msg) { \ > + if (p) \ > + sendAsyncMessage("OK", msg); \ > + else \ > + sendAsyncMessage("KO", msg); \ if () { } else { } @@ +88,5 @@ > + function is(a, b, msg) { \ > + if (a == b) \ > + sendAsyncMessage("OK", a + " == " + b + " - " + msg); \ > + else \ > + sendAsyncMessage("KO", a + " != " + b + " - " + msg); \ ditto @@ +95,5 @@ > + function finish() { \ > + sendAsyncMessage("DONE",""); \ > + } \ > + \ > + function cbError() { \ onError ::: dom/browser-element/BrowserElementParent.cpp @@ +169,5 @@ > + nsCOMPtr<nsIMozBrowserFrame> browserFrame = > + do_QueryInterface(aOpenerFrameElement); > + if (browserFrame && browserFrame->GetReallyIsWidget()) { > + return BrowserElementParent::OPEN_WINDOW_CANCELLED; > + } Test this. @@ +362,5 @@ > + NS_ENSURE_TRUE(frameElement, false); > + nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(frameElement); > + if (browserFrame && browserFrame->GetReallyIsWidget()) { > + return true; > + } Test this. ::: dom/browser-element/BrowserElementParent.jsm @@ +119,2 @@ > > + // Not expose security sensitive browser API for widgets Each method in this list should be tested like getScreenshot. @@ +275,5 @@ > + } else if (self._isAlive() && > + (aMsg.data.msg_name in mmSecuritySensitiveCalls) && > + !isWidget ) { > + return mmSecuritySensitiveCalls[aMsg.data.msg_name] > + .apply(self, arguments); Check !self._isAlive() and bailout early in this function.

Plan to land it by the end of v2.1 sprint 3 (August 29).

Update according to feedback comment 43

Comment on attachment 8472237 [details] [diff] [review] Part 2(v2): Only limited browser API are available to a widget Review of attachment 8472237 [details] [diff] [review]: ----------------------------------------------------------------- Looks good but I still have some questions ::: content/base/public/nsIFrameLoader.idl @@ +285,2 @@ > */ > readonly attribute boolean ownerIsBrowserOrAppFrame; Please add a comment in previous patch that says that nsIMozBrowserFrame::appManifestURL could return either the app manifest or widget manifest, otherwise the code in GetReallyIsApp and this comment is rather non intuitive. ::: content/html/content/src/nsGenericHTMLFrameElement.cpp @@ +315,5 @@ > +{ > + if (!Preferences::GetBool("dom.enable_widgets")) { > + *aOut = false; > + return NS_OK; > + } Cache this like nsGenericHTMLFrameElement::BrowserFramesEnabled() ::: dom/interfaces/html/nsIMozBrowserFrame.idl @@ +37,5 @@ > + * frame (this requirement will go away eventually), the frame's mozwidget > + * attribute must point to the manifest of a valid app , and the src should > + * be in the |widgetPages| specified by the manifest. > + */ > + [infallible] readonly attribute boolean reallyIsWidget; Do we check the src is really in the |widgetPages|? Do we have tests for this?

> ::: dom/interfaces/html/nsIMozBrowserFrame.idl > @@ +37,5 @@ > > + * frame (this requirement will go away eventually), the frame's mozwidget > > + * attribute must point to the manifest of a valid app , and the src should > > + * be in the |widgetPages| specified by the manifest. > > + */ > > + [infallible] readonly attribute boolean reallyIsWidget; > > Do we check the src is really in the |widgetPages|? Do we have tests for > this? We check the fact in nsGenericHTMLFrameElement::GetManifestURLByType. I'll implement tests for this. By the way, the behavour of invalid src will be changed in bug 1043110.

(In reply to Junior Hsu [:juniorhsu] from comment #48) > Created attachment 8474951 [details] [diff] [review] > Part 1: Load a widget as an app if the |src| is in the |widgetPages| Modify comments by comment 46, carry r+

Modify by comment 46

Comment on attachment 8472237 [details] [diff] [review] Part 2(v2): Only limited browser API are available to a widget obsolete owing to new patch

Comment on attachment 8474951 [details] [diff] [review] Part 1: Load a widget as an app if the |src| is in the |widgetPages| Review of attachment 8474951 [details] [diff] [review]: ----------------------------------------------------------------- This looks mostly good to me. However is there anything that ensures that the widget can't navigate to pages other than what's listed in widgetPages? This patch seems to make sure that we never open a page other than ones enumerated in manifest, but we should also make sure that if there's a link to other pages, that we prevent such navigation. Or is that handled in a later patch? Also, fabrice should probably look over the Webapps.jsm changes if he wants to.

(In reply to Junior Hsu [:juniorhsu] from comment #6) > There's an ambiguous behaviour need to be clairfied. > In the WIP patch, if the |src| is not listed in the |widgetPages| list, the > iframe will be a general iframe, not a widget/app. > Is the behaviour reasonable? Or it should be a blank page. I think this is ok. It would be slightly cleaner to open a blank page, but if that's complex, then it seems ok to do what you're doing since it's a case that's unlikely to be hit commonly.

(In reply to Jonas Sicking (:sicking) from comment #52) > Comment on attachment 8474951 [details] [diff] [review] > Part 1: Load a widget as an app if the |src| is in the |widgetPages| > > Review of attachment 8474951 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks mostly good to me. > > However is there anything that ensures that the widget can't navigate to > pages other than what's listed in widgetPages? This patch seems to make sure > that we never open a page other than ones enumerated in manifest, but we > should also make sure that if there's a link to other pages, that we prevent > such navigation. > > Or is that handled in a later patch? The navigation issue is tracked by bug 1043110. The whole feature is under permission and preference, so IMO it's ok to handle it later. > > Also, fabrice should probably look over the Webapps.jsm changes if he wants > to. fabrice has reviewed this patch, including Webapps.jsm. Thanks for your help.

Sounds good. We should make sure to fix the navigation issue before turning the preference on.

Comment on attachment 8474953 [details] [diff] [review] Part 2(v3): Only limited browser API are available to a widget Review of attachment 8474953 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsGenericHTMLFrameElement.cpp @@ +328,5 @@ > +/* [infallible] */ NS_IMETHODIMP > +nsGenericHTMLFrameElement::GetReallyIsWidget(bool *aOut) > +{ > + if (!nsGenericHTMLFrameElement::WidgetsEnabled()) { > + *aOut = false; Put this line at the beginning of the function. ::: content/html/content/src/nsGenericHTMLFrameElement.h @@ +90,5 @@ > } > > private: > void GetManifestURLByType(nsIAtom *aAppType, nsAString& aOut); > + static bool WidgetsEnabled(); If you want to make this private then make it file local static function or put it into a anonymous namespace in .cpp Otherwise make it public and alongside the BrowserFramesEnabled() declaration.

Rebase to m-c, carry r+, sr+

Rebase to m-c and modify based on comment 56, carry r+

try result: https://tbpl.mozilla.org/?tree=Try&rev=b7a167901153

https://hg.mozilla.org/integration/mozilla-inbound/rev/e020d647d6d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/e71a3cac1b3d

Reverted for exceptions during test_widget.html: https://tbpl.mozilla.org/php/getParsedLog.php?id=46259112&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=46259188&tree=Mozilla-Inbound Note the try run only tested mochitest-{2.5}, this failure was in mochitest-1. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/860f2941b071 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/03acff0b4f82

Actually, this was bug 1014023, sorry misread. Relanded: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/498506843eaf remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6e1d4006ef

https://hg.mozilla.org/mozilla-central/rev/498506843eaf https://hg.mozilla.org/mozilla-central/rev/4e6e1d4006ef

No end-user QA verification needed, so marking verified.