Closed Bug 1000313 Opened 10 years ago Closed 10 years ago

Implement a permission which allows homescreen apps to access the relevant mozApp API functions.

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: tedders1, Assigned: tedders1)

References

Details

(Whiteboard: [systemsfe][p=8][tako])

Attachments

(2 files, 13 obsolete files)

17.01 KB, patch
Details | Diff | Splinter Review
1.54 KB, patch
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #899994 +++

From Bug 899994, Comment 17:

Add a new "homescreen-webapps-manage" permission. This permission gives access to calling uninstall() and getAppIcon. But the app is only allowed to use either of these if it's the currently selected homescreen app.

From Bug 899994, Comment 24:

We will create a new permission for 3rd party homescreens called 'homescreen-webapps-manage' which will allow:

Apps.mgmt.getAll()  
Apps.mgmt.uninstall() 
Apps.mgmt.addEventListener() 
Apps.mgmt.removeEventListener()
Apps.mgmt.oninstall  (get/set)
Apps.mgmt.onuninstall (get/set)
Apps.mgmt.getAppIcon() - new API which provides the icon for a given app

_homescreen-webapps-manage_ will not grant access to any other _webapps-manage_ privileges.
For reference, _homescreen-webapps-manage_  will not grant access to:
- Other Apps.mgmt APIs not listed above
- the ability to read resources in other apps  (ie [1])
- devicestorage:apps access

[Note that getAppIcon() is a new API function to be implemented in Bug 1000305.]
Depends on: 1000315
blocking-b2g: --- → 2.0+
From my comment 26 in the other bug, since we're adding this permission in the lockscreen I think it's important to not have 'homescreen-' in the name of the permission.

(In reply to Kevin Grandon :kgrandon from comment #26)
> Having 'homescreen-' in the name of the permission seems wrong to me. Surely
> there are other privileged apps besides homescreens which might make use of
> this permission.
> 
> I would like to see us leave the name along and just grant access to the
> allowed methods. If that is not possible, something like
> "privileged-webapps-manage" makes more sense to me.
Assignee: nobody → tclancy
Target Milestone: --- → 2.0 S1 (9may)
This shouldn't block - this is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.
blocking-b2g: 2.0+ → backlog
feature-b2g: --- → 2.0
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Attached patch bug-1000313-fix.patch (obsolete) — Splinter Review
Attachment #8431697 - Flags: review?(jonas)
Attached patch bug-1000313-fix.patch (obsolete) — Splinter Review
Attachment #8431697 - Attachment is obsolete: true
Attachment #8431697 - Flags: review?(jonas)
Attachment #8431699 - Flags: review?(jonas)
Attached patch bug-1000313-fix.patch (obsolete) — Splinter Review
Now with commit comment.
Attachment #8431699 - Attachment is obsolete: true
Attachment #8431699 - Flags: review?(jonas)
Attachment #8431700 - Flags: review?(jonas)
Comment on attachment 8431700 [details] [diff] [review]
bug-1000313-fix.patch

Review of attachment 8431700 [details] [diff] [review]:
-----------------------------------------------------------------

I think this patch works accidentally because you grant all permissions to all certified apps.

kgrandon will also want to bikeshed on the new permission name...

::: b2g/chrome/content/settings.js
@@ +73,5 @@
>    Services.prefs.setBoolPref('layout.css.report_errors', value);
>  });
>  
> +SettingsListener.observe('homescreen.manifestURL', 'Sentinel Value' , function(value) {
> +  Services.prefs.setCharPref('dom.apps.homescreenURL', value);

we use dom.mozApps.XXX preferences

::: dom/apps/src/PermissionsInstaller.jsm
@@ +72,5 @@
> +          // "homescreen-manage" is implied.
> +          if ("webapps-manage" in newManifest.permissions) {
> +            if (!("homescreen-manage" in newManifest.permissions)) {
> +              newManifest.permissions["homescreen-manage"] = newManifest.permissions["webapps-manage"];
> +            }

Can we use the "additional" property like at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#147 ?

::: dom/apps/src/Webapps.js
@@ +257,5 @@
> +
> +    let checkHasHomescreenPermission = () =>
> +      Ci.nsIPermissionManager.ALLOW_ACTION ==
> +        Services.perms.testExactPermissionFromPrincipal(principal,
> +                                                        "homescreen-manage"));

Why make these functions and not just booleans?

@@ +264,5 @@
> +                        Ci.nsIPrincipal.APP_STATUS_CERTIFIED;
> +
> +    this.hasMgmtPrivilege = isCertified || checkHasWebappsPermission();
> +    this.hasHomescreenPrivilege = isCertified ||
> +      (checkIsCurrentHomescreen() && checkHasHomescreenPermission());

That grants the permissions to all certified apps...

@@ +778,5 @@
> +
> +  if (!this.hasHomescreenPrivilege) {
> +    this.uninstall = null;
> +    this.getAll = null;
> +  }

so an app that has webapps-manage but not homescreen-manage can't call uninstall() and getAll() ? I think that should be the other way around:
- give a mgmt object to apps that have either permission
- only allow uninstall(), getAll() and event handlers to be set by those who have the webapps-manage permission.

::: dom/apps/src/Webapps.jsm
@@ +1061,5 @@
>      // the pref instead of first checking if it is false.
>      Services.prefs.setBoolPref("dom.mozApps.used", true);
>  
>      // We need to check permissions for calls coming from mozApps.mgmt.
>      // These are: getAll(), getNotInstalled(), applyDownload() and uninstall().

nit: update the comment.
Comment on attachment 8431700 [details] [diff] [review]
bug-1000313-fix.patch

Review of attachment 8431700 [details] [diff] [review]:
-----------------------------------------------------------------

this needs updates based on the below (and Fabrice's) comments. So it'd be good to see a new patch.

Fabrice: Are there any checks that we could do in the parent process as well?

::: dom/apps/src/PermissionsTable.jsm
@@ +172,5 @@
>                               certified: ALLOW_ACTION
>                             },
> +                           "homescreen-manage": {
> +                             app: DENY_ACTION,
> +                             privileged: PROMPT_ACTION,

Shouldn't this be ALLOW_ACTION?

::: dom/apps/src/Webapps.js
@@ +201,5 @@
>    },
>  
>    get mgmt() {
>      if (!this.hasMgmtPrivilege) {
>        return null;

Won't this return early since hasMgmtPrivilige will be false if the page only has homescreen privilege. It's good that we're tracking them separately, but make this line:

if (!this.hasMgmtPrivilege && !this.hasHomescreenPrivilege)

Maybe we should rename hasMgmtPrivilege to hasWebappsManagePrivilege?

@@ +206,5 @@
>      }
>  
> +    if (!this._mgmt) {
> +      this._mgmt = new WebappsApplicationMgmt(this._window,
> +                                              this.hasHomescreenPrivilege);

Rather than tracking "does this mgmt object only have homescreen privilege" it would be more clear to track "does this mgmt object have fill privilege". So pass this.hasMgmtPrivilege as the second argument.

@@ +248,4 @@
>  
> +    let checkIsCurrentHomescreen = () =>
> +      this._getOrigin(aWindow.location.href) ==
> +        this._getOrigin(prefs.get("dom.apps.homescreenURL"));

Don't just check that the origins are the same. Check that the manifesturl of the current page matches the value in the pref. See the getSelf() implementation for how to get the manifesturl of the current page.

@@ +257,5 @@
> +
> +    let checkHasHomescreenPermission = () =>
> +      Ci.nsIPermissionManager.ALLOW_ACTION ==
> +        Services.perms.testExactPermissionFromPrincipal(principal,
> +                                                        "homescreen-manage"));

This should be "homescreen-webapps-manage" I think. To indicate that it's about managing webapps rather than just managing the homescreen.

And yes, just grab the booleans. No need for this lazy evaluation since the permission manager is really fast (just a memory hash table lookup)

@@ +264,5 @@
> +                        Ci.nsIPrincipal.APP_STATUS_CERTIFIED;
> +
> +    this.hasMgmtPrivilege = isCertified || checkHasWebappsPermission();
> +    this.hasHomescreenPrivilege = isCertified ||
> +      (checkIsCurrentHomescreen() && checkHasHomescreenPermission());

Remove the "isCertified ||" part. The fact that the permission is granted is enough.

@@ +778,5 @@
> +
> +  if (!this.hasHomescreenPrivilege) {
> +    this.uninstall = null;
> +    this.getAll = null;
> +  }

I agree with Fabrice here. Having webapps-manage should give access to all of mgmt. Having homescreen-webapps-manage should just give access to a subset of it.
Attachment #8431700 - Flags: review?(jonas) → review-
(In reply to Kevin Grandon :kgrandon from comment #1)
> From my comment 26 in the other bug, since we're adding this permission in
> the lockscreen I think it's important to not have 'homescreen-' in the name
> of the permission.

Actually, the way we're currently implementing this permission, it's only granted to apps that are the active homescreen. So adding it to a lockscreen app will do nothing.

What does the lockscreen need to do? It doesn't seem important for it to be able to uninstall apps for example.

Does it just need to call getAll() and getAppIcon()? And maybe events for when apps are installed/uninstalled?

I guess that's effectively everything except for uninstall()...
Attached patch bug-1000313-fix.patch (obsolete) — Splinter Review
Attachment #8431700 - Attachment is obsolete: true
Attachment #8434747 - Flags: review?(jonas)
Attached patch bug-1000313-fix.patch (obsolete) — Splinter Review
Bug 1000313 - Part 1: Implement a permission which allows homescreen to access relevant APIs. r=sicking
Attachment #8434747 - Attachment is obsolete: true
Attachment #8434747 - Flags: review?(jonas)
Attachment #8434928 - Flags: review?(jonas)
Attached patch bug-1000313-fix-part-2.patch (obsolete) — Splinter Review
Bug 1000313 - Part 2: Add a way for Webapps.jsm to check app permissions without killing app. r=ehsan
Attachment #8434929 - Flags: review?(ehsan)
feature-b2g: 2.0 → 2.1
Comment on attachment 8434929 [details] [diff] [review]
bug-1000313-fix-part-2.patch

Review of attachment 8434929 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I don't know this code well enough to feel comfortable reviewing this patch...
Attachment #8434929 - Flags: review?(ehsan) → review?(jonas)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #12)
> Comment on attachment 8434929 [details] [diff] [review]
> bug-1000313-fix-part-2.patch
> 
> Review of attachment 8434929 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I don't know this code well enough to feel comfortable reviewing this
> patch...

Ted reminded me that we had talked about this before, I'll take this!
Comment on attachment 8434929 [details] [diff] [review]
bug-1000313-fix-part-2.patch

Review of attachment 8434929 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Thanks!

::: content/base/public/nsIMessageManager.idl
@@ +428,4 @@
>     */
>    boolean assertPermission(in DOMString aPermission);
>  
> +  boolean queryPermission(in DOMString aPermission);

Please rev the uuid of the interface.

::: content/base/src/nsFrameMessageManager.cpp
@@ +840,5 @@
>  nsFrameMessageManager::AssertPermission(const nsAString& aPermission,
>                                          bool* aHasPermission)
>  {
> +  return CheckProcessInternal(PROCESS_CHECKER_ASSERT_PERMISSION,
> +                               aPermission,

Nit: please fix the indentation, there's a bunch of places in the patch where the new indentation is one whitespace off. ;)

::: dom/ipc/AppProcessChecker.cpp
@@ +83,5 @@
> +AssertAppProcess(PBrowserParent* aActor,
> +                 AppProcessQueryType aType,
> +                 const char* aCapability)
> +{
> +  return QueryAppProcess(aActor, aType, aCapability);

Can you please file a follow-up bug to get someone investigate why this function doesn't perform any assertions?  The PContentParent counterpart does an assert once it calls this on all PBrowserParents, and this setup sounds fishy to me...
Attachment #8434929 - Flags: review?(jonas) → review+
Comment on attachment 8434928 [details] [diff] [review]
bug-1000313-fix.patch

Review of attachment 8434928 [details] [diff] [review]:
-----------------------------------------------------------------

Much closer, but still a couple of things that I think needs to be fixed.

::: dom/apps/src/Webapps.js
@@ +249,3 @@
>  
> +    let isCurrentHomescreen = this._getOrigin(aWindow.location.href) ==
> +      this._getOrigin(prefs.get("dom.mozApps.homescreenURL"));

This still looks wrong. See my previous review comment.

::: dom/apps/src/Webapps.jsm
@@ +1082,5 @@
>  
> +    if (["Webapps:GetAll",
> +         "Webapps:Uninstall"].indexOf(aMessage.name) != -1) {
> +      if (!aMessage.target.queryPermission("webapps-manage") &&
> +          !aMessage.target.queryPermission("homescreen-webapps-manage")) {

For homescreen-webapps-manage, we should also check that the app is currently selected as the homescreen app.
Attachment #8434928 - Flags: review?(jonas) → review-
Comment on attachment 8434929 [details] [diff] [review]
bug-1000313-fix-part-2.patch

Review of attachment 8434929 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, I'd rather not do things this way.

What we should be doing is to pass the principal of the calling page as the fourth argument to mm.sendAsyncMessage. When a message goes from a child to the parent, the parent should check that the principal coming from the child is not a lie (e.g. it'll check that the appid of the principal matches the appid of the child process).

So far this should already be working.

The message listener should then receive a principal which it could pass to nsIPermissionManager to check if the caller has a particular permission.

This part only partially works. We apparently do pass a principal-ish thing to the message listener. But it's not a real nsIPrincipal. qDot was looking at fixing this in another bug, so you should reach out to him to coordinate.
Attachment #8434929 - Flags: review-
Attached patch bug-1000313-fix-part-2.patch (obsolete) — Splinter Review
Attachment #8434929 - Attachment is obsolete: true
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Hi Jonas. In Comment 16, you said:

> When a message goes from a child to the parent, the parent should check that the principal coming from the child is not a lie (e.g. it'll check that the appid of the principal matches the appid of the child process).

In Webapps.jsm, how can I determine what the appid of the child process is?
Flags: needinfo?(jonas)
Should I add an appId property to nsIProcessChecker (in nsIMessageManager.idl), and then call mm.appId? (I can do that, but it's not trivial.)

And is this a better solution than the patch I had before which added a queryPermission() method to nsIProcessChecker?
When you pass a message through the message manager you can pass a principal as an optional argument. The message manager will in the parent process check that that principal is correct. This code already exists. Just look at the sendMessage signature in message manager.

The piece that's missing is that the message manager doesn't expose that principal to the message receiver. Or rather, it doesn't do that in the form of a principal object. This needs to be fixed. Talk to qDot and Baku about how to fix that.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #20)
> When you pass a message through the message manager you can pass a principal
> as an optional argument. The message manager will in the parent process
> check that that principal is correct. This code already exists. Just look at
> the sendMessage signature in message manager.
> 
> The piece that's missing is that the message manager doesn't expose that
> principal to the message receiver. Or rather, it doesn't do that in the form
> of a principal object. This needs to be fixed. Talk to qDot and Baku about
> how to fix that.

Hi Jonas. I've done all that already.

My question is about your comment thus:

> it'll check that the appid of the principal matches the appid of the child process

How do I get the appId of the child process? It seems that I'll have to get it from the message manager somehow. Should I add an appId property to the nsIProcessChecker interface, and then write the implementation in C++?
Flags: needinfo?(jonas)
Why do you need to get the appid of the child process? The message manager will already check that the appid matches that of the child process. So the principal that you get from the message manager is trustworthy.

All you need to do is to check that the principal has permission to perform the operation that is requested. You Chan use nsipermissionmanager to check that.
Flags: needinfo?(jonas)
> So the principal that you get from the message manager is trustworthy.

Oh, okay. I misunderstood. I thought you were saying that I needed to do that check. If nsIPrincipal is doing the check, everything is fine.

Thanks.
Attached patch bug-1000313-fix-part-1.patch (obsolete) — Splinter Review
Bug 1000313 - Part 1: Adding KillChild() method to nsIMessageManager. r=sicking
Attachment #8434928 - Attachment is obsolete: true
Attachment #8458671 - Flags: review?(jonas)
Attached patch bug-1000313-fix-part-2.patch (obsolete) — Splinter Review
Bug 1000313 - Part 2: Adding homescreen-webapps-manage permission. r=sicking,qdot

I'm adding qdot to this review for the _getRealPrincipal method.
Attachment #8437190 - Attachment is obsolete: true
Attachment #8458674 - Flags: review?(kyle)
Attachment #8458674 - Flags: review?(jonas)
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Blocks: 899994
No longer depends on: 899994
Blocks: 1042797
No longer blocks: 1042797
Blocks: 1042807
No longer blocks: 1042807
blocking-b2g: backlog → ---
Whiteboard: [systemsfe][p=8] → [systemsfe][p=8][tako]
Comment on attachment 8458674 [details] [diff] [review]
bug-1000313-fix-part-2.patch

Review of attachment 8458674 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with recommended changes.

::: dom/apps/src/PermissionsTable.jsm
@@ +174,5 @@
> +                           "homescreen-webapps-manage": {
> +                             app: DENY_ACTION,
> +                             privileged: ALLOW_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },

You'll need to add 

additional: ["settings-api"]

to this for when Bug 846200 lands. That's how we let privileged apps access the settings API.

::: dom/apps/src/Webapps.js
@@ +273,5 @@
> +    let isCurrentHomescreen =
> +      app.manifestURL == prefs.get("dom.mozApps.homescreenURL") &&
> +      app.appStatus != Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED;
> +
> +    let hasWebappsPermission = Ci.nsIPermissionManager.ALLOW_ACTION ==

Nit: Here and below, some parens might make logic like this a little easier to read (said the lisp coding reviewer).

::: dom/apps/src/Webapps.jsm
@@ +1082,5 @@
> +    }
> +    if (aPseudoPrincipal.origin == "[System Principal]") {
> +      return Services.scriptSecurityManager.getSystemPrincipal();
> +    } else {
> +      let appId = aPseudoPrincipal.appId != 0 ? aPseudoPrincipal.appId : null;

We can probably just go with whatever aPseudoPrincipal.appId is here, no need to cast. I had that in my stuff for settings because of some other things I was debugging.
Attachment #8458674 - Flags: review?(kyle) → review+
Comment on attachment 8458674 [details] [diff] [review]
bug-1000313-fix-part-2.patch

Review of attachment 8458674 [details] [diff] [review]:
-----------------------------------------------------------------

I think there's an issue if a random page tries to use the navigator.mozApps api.

::: b2g/chrome/content/settings.js
@@ +72,5 @@
>    Services.prefs.setBoolPref('consoleservice.enabled', value);
>    Services.prefs.setBoolPref('layout.css.report_errors', value);
>  });
>  
> +SettingsListener.observe('homescreen.manifestURL', 'Sentinel Value' , function(value) {

s/'sentinel value'/''

::: dom/apps/src/Webapps.js
@@ +25,5 @@
>  
> +XPCOMUtils.defineLazyGetter(this, "permMgr", function() {
> +  return Cc["@mozilla.org/permissionmanager;1"]
> +           .getService(Ci.nsIPermissionManager);
> +});

Use Services.perms instead directly where you need it.

@@ +260,4 @@
>      this.initDOMRequestHelper(aWindow, "Webapps:Install:Return:OK");
>  
> +    let appId = this._window.document.nodePrincipal.appId;
> +    let app = appsService.getAppByLocalId(appId);

app will be null when called from a webpage. I think bad things will happen later in this function.

@@ +272,3 @@
>  
> +    let isCurrentHomescreen =
> +      app.manifestURL == prefs.get("dom.mozApps.homescreenURL") &&

nit: not sure we want to put that in the dom.mozApps space.
Also, we really need tests to check we are exposing the right calls.
Why should we automatically grant access to the full settings API whenever homescreen-webapps-manage permission is granted? Note that this would happen even for privileged apps, which we've so far said that that we would not grant full access to settings (but rather just access to individual settings).

Why not simply let the homescreen ask for the settings that it needs using separate entries in its manifest?
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #29)
> Why should we automatically grant access to the full settings API whenever
> homescreen-webapps-manage permission is granted? 

I misread what was happening there. I'll fix my review.
Comment on attachment 8458674 [details] [diff] [review]
bug-1000313-fix-part-2.patch

Review of attachment 8458674 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/PermissionsTable.jsm
@@ +174,5 @@
> +                           "homescreen-webapps-manage": {
> +                             app: DENY_ACTION,
> +                             privileged: ALLOW_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },

Ignore this actually. We don't need settings-api there. I've been working on Bug 900551 way, way too much. :)
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Hi Ted,

May we have "app://" protocol in homescreen-webapps-manage? We need "app://" protocol to retrieve "widget screenshot" and "open widget" in mozwidget iframe.
Flags: needinfo?(tclancy)
Comment on attachment 8458674 [details] [diff] [review]
bug-1000313-fix-part-2.patch

Review of attachment 8458674 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/Webapps.js
@@ +272,3 @@
>  
> +    let isCurrentHomescreen =
> +      app.manifestURL == prefs.get("dom.mozApps.homescreenURL") &&

Hi Ted,

One more question about this: gaia apps use mozSettings API/DB to configure the homescreen URL[1][2]. IIRC, gaia apps don't have a way to change preference. That's also way we use mozSettings API/DB to exchange information.


[1] https://github.com/mozilla-b2g/gaia/blob/19ed3c9e78eaf234cc08484bde6998ae21552ba5/apps/system/js/homescreen_launcher.js#L77
[2] https://github.com/mozilla-b2g/gaia/blob/19ed3c9e78eaf234cc08484bde6998ae21552ba5/apps/settings/js/homescreen.js#L48
Hi Ted,

Please ignore comment 32. I found that app:// protocol is disabled on purpose. We will find a way in Widget API.
Flags: needinfo?(tclancy)
Blocks: 1052328
Comment on attachment 8458671 [details] [diff] [review]
bug-1000313-fix-part-1.patch

Review of attachment 8458671 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but Olli should review since I'm not an owner of this code.
Attachment #8458671 - Flags: review?(jonas)
Attachment #8458671 - Flags: review?(bugs)
Attachment #8458671 - Flags: feedback+
Comment on attachment 8458674 [details] [diff] [review]
bug-1000313-fix-part-2.patch

Review of attachment 8458674 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, but fabrice owns this code so he should review.

::: dom/apps/src/Webapps.jsm
@@ +1112,5 @@
> +
> +    // We need to check permissions for calls coming from mozApps.mgmt.
> +    // These are: getAll(), getNotInstalled(), applyDownload() and uninstall().
> +
> +    let allowed = true;

Set allowed to false by default. It's always better to fail safe in case there are bugs.
Attachment #8458674 - Flags: review?(jonas)
Attachment #8458674 - Flags: review?(fabrice)
Attachment #8458674 - Flags: feedback+
Comment on attachment 8458674 [details] [diff] [review]
bug-1000313-fix-part-2.patch

Review of attachment 8458674 [details] [diff] [review]:
-----------------------------------------------------------------

Can you post an updated patch with previous comments addressed? thanks!
Attachment #8458674 - Flags: review?(fabrice)
Hi Fabrice. 

I have an updated patch, but I'm hesitant to submit it for review until I it passes TBPL. I haven't been able to do a TBPL run, because TBPL has been down whenever I've tried to use it. :-(
Attachment #8458671 - Flags: review?(bugs) → review+
Attached patch bug-1000313-fix-part-1.patch (obsolete) — Splinter Review
Rebased against master.
Attachment #8458671 - Attachment is obsolete: true
Attached patch bug-1000313-fix-part-2.patch (obsolete) — Splinter Review
Rebased against master.
Attachment #8458674 - Attachment is obsolete: true
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
feature-b2g: 2.1 → 2.2
Target Milestone: 2.1 S3 (29aug) → ---
Comment on attachment 8474744 [details] [diff] [review]
bug-1000313-fix-part-1.patch

Moving this patch to Bug 1055427.
Attachment #8474744 - Attachment is obsolete: true
Attached patch bug-1000313-fix-part-2.patch (obsolete) — Splinter Review
Rebased.
Attachment #8474745 - Attachment is obsolete: true
Use feature-b2g:2.2? rather than just 2.2.
feature-b2g: 2.2 → 2.2?
Blocks: 1092726
Attached patch new-bug-1000313-fix.patch (obsolete) — Splinter Review
Rebased & Updated.
Attachment #8477906 - Attachment is obsolete: true
Keywords: checkin-needed
Oops. Just fixing a comment.
Attachment #8515653 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/32b0ce626ec8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Hey Ted - do you by chance have some example app somewhere that this is working with? I tried adding the homescreen-webapps-manage permission to a privileged app, but I was seeing the following error. Just wondering if you had any ideas.

[JavaScript Error: "TypeError: navigator.mozApps.mgmt is undefined" {file: "...
Flags: needinfo?(tclancy)
(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #49)
> Hey Ted - do you by chance have some example app somewhere that this is
> working with? I tried adding the homescreen-webapps-manage permission to a
> privileged app, but I was seeing the following error. Just wondering if you
> had any ideas.
> 
> [JavaScript Error: "TypeError: navigator.mozApps.mgmt is undefined" {file:
> "...
I'm not Ted, but here's my result
1. permission checking in apps.webidl should be modified since Bug 899322
2. add pref to satisfy |app.manifestURL == prefs.get("dom.mozApps.homescreenURL")|

At least I can get navigator.mozApps.mgmt after the two changes
Junior - Thanks for the investigation. Did you have a patch sitting somewhere that fixes these? If so, did you want to upload it here or maybe in another bug? I would just love to see this fixed, and was considering looking into it. If you already have something available, that would save us time though. Thanks!
Flags: needinfo?(juhsu)
1. I have a patch for webidl as attachment. However, I think some APIs (Bug 1072090/Bug 982874/...) should be discussed for exposure to |homescreen-webapps-manage|, thus causing me to file bug 1097468 to move the discussion there. 
2. For the preference part, IMO it's design on purpose.
Flags: needinfo?(juhsu)
Comment on attachment 8521141 [details] [diff] [review]
homescreen-webapps-manage

Review of attachment 8521141 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch Junior. Fabrice - Can we get this patch reviewed?
Attachment #8521141 - Flags: review?(fabrice)
This bug is closed. Can you move that to a follow-up please? Also, we'll need tests.
Comment on attachment 8521141 [details] [diff] [review]
homescreen-webapps-manage

Review of attachment 8521141 [details] [diff] [review]:
-----------------------------------------------------------------

You should rather ask Jonas to review since you need a DOM peer review to land webidl changes.
Attachment #8521141 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #54)
> This bug is closed. Can you move that to a follow-up please? Also, we'll
> need tests.

Oh right, I think this might be bug 1097468? Will follow-up in that bug.
Flags: needinfo?(tclancy)
Depends on: 1097468
Depends on: 1108749
No longer depends on: 1108749
feature-b2g: 2.2? → ---
Blocks: TV_FxOS2.5
No longer blocks: TV_FxOS2.5
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: