Last Comment Bug 781379 - getNotInstalled should be under mgmt and return apps from all origins
: getNotInstalled should be under mgmt and return apps from all origins
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: (not reading bugmail) Nick Desaulniers [:\n]
:
Mentors:
Depends on: 783391
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-08 16:36 PDT by Anant Narayanan [:anant]
Modified: 2012-08-21 06:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
getNotInstalled should be under mgmt and return apps from all origins (10.39 KB, patch)
2012-08-20 10:50 PDT, (not reading bugmail) Nick Desaulniers [:\n]
no flags Details | Diff | Splinter Review
getNotInstalled should be under mgmt and return apps from all origins (10.39 KB, patch)
2012-08-20 14:01 PDT, (not reading bugmail) Nick Desaulniers [:\n]
no flags Details | Diff | Splinter Review

Description Anant Narayanan [:anant] 2012-08-08 16:36:01 PDT
In bug 768276 the getNotInstalled function was added, but directly under navigator.mozApps. It should be under navigator.mozApps.mgmt, which signifies APIs that are "privileged".

Both getNotInstalled and getAll fall under the privileged category, while getInstalled can stay directly under mozApps since it only returns apps that were installed from the caller's origin.
Comment 1 (not reading bugmail) Nick Desaulniers [:\n] 2012-08-16 15:03:30 PDT
Code is functional (I think, but I'll be more confident after unit tests), and I've fixed regressions with the other tests.  Now just to write up a chrome mochitest (not a browser mochitest)!

test with:
TEST_PATH=dom/tests/mochitest/webapps/ make -C objdir/ mochitest-chrome
Comment 2 (not reading bugmail) Nick Desaulniers [:\n] 2012-08-20 10:24:08 PDT
Tests are green.  Rebasing, and running tests again to double check everything, then submitting patch for review.
Comment 3 (not reading bugmail) Nick Desaulniers [:\n] 2012-08-20 10:50:04 PDT
Created attachment 653436 [details] [diff] [review]
getNotInstalled should be under mgmt and return apps from all origins
Comment 4 :Felipe Gomes (needinfo me!) 2012-08-20 13:32:11 PDT
Comment on attachment 653436 [details] [diff] [review]
getNotInstalled should be under mgmt and return apps from all origins

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

::: dom/tests/mochitest/webapps/test_getNotInstalled.xul
@@ +17,5 @@
> +
> +let _isLaunchable;
> +let steps = [
> +  cleanUp, installApp, monkeyPatchDOMApplicationRegistry, getNotInstalled,
> +  cleanUp, unmonkeyPatchDOMApplicationRegistry, tearDown

I believe you need to undo the monkey patching before calling cleanUp, otherwise the app you installed won't be uninstalled because the getAll used in uninstallAll will skip it
Comment 5 (not reading bugmail) Nick Desaulniers [:\n] 2012-08-20 14:01:48 PDT
Created attachment 653509 [details] [diff] [review]
getNotInstalled should be under mgmt and return apps from all origins
Comment 6 (not reading bugmail) Nick Desaulniers [:\n] 2012-08-20 14:03:00 PDT
(In reply to :Felipe Gomes from comment #4)
> Comment on attachment 653436 [details] [diff] [review]
> getNotInstalled should be under mgmt and return apps from all origins
> 
> Review of attachment 653436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/tests/mochitest/webapps/test_getNotInstalled.xul
> @@ +17,5 @@
> > +
> > +let _isLaunchable;
> > +let steps = [
> > +  cleanUp, installApp, monkeyPatchDOMApplicationRegistry, getNotInstalled,
> > +  cleanUp, unmonkeyPatchDOMApplicationRegistry, tearDown
> 
> I believe you need to undo the monkey patching before calling cleanUp,
> otherwise the app you installed won't be uninstalled because the getAll used
> in uninstallAll will skip it

Good catch, I reordered the steps in the test.
Comment 8 [:fabrice] Fabrice Desré 2012-08-20 20:22:19 PDT
Comment on attachment 653509 [details] [diff] [review]
getNotInstalled should be under mgmt and return apps from all origins

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

Please file a follow-up or backout this patch.

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +54,5 @@
>    /**
> +   * the request will return the applications acquired from all origins but
> +   * which are not launchable (e.g. by not being natively installed), or null.
> +   */
> +  nsIDOMDOMRequest getNotInstalled();

You must change the UUID of the interface.

@@ -95,5 @@
>    /**
> -   * the request will return the applications acquired from this origin but which
> -   * are not launchable (e.g. by not being natively installed), or null.
> -   */
> -  nsIDOMDOMRequest getNotInstalled();

And this one also.
Comment 9 Anant Narayanan [:anant] 2012-08-20 20:31:12 PDT
Comment on attachment 653509 [details] [diff] [review]
getNotInstalled should be under mgmt and return apps from all origins

Bug 784245 is the follow-up.
Comment 10 Ed Morley [:emorley] 2012-08-21 06:29:07 PDT
https://hg.mozilla.org/mozilla-central/rev/3e952e6793ae

Note You need to log in before you can comment on or make changes to this bug.