Closed
Bug 1097468
Opened 10 years ago
Closed 10 years ago
expose |homescreen-webapps-manage| via webidl
Categories
(Core Graveyard :: DOM: Apps, defect)
Core Graveyard
DOM: Apps
Tracking
(firefox40 fixed)
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: CuveeHsu, Assigned: CuveeHsu)
References
Details
(Whiteboard: [ft:conndevices])
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
CuveeHsu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
CuveeHsu
:
review+
|
Details | Diff | Splinter Review |
Since the mozApps API was converted to use webidl in bug 899322, permission checking in apps.webidl should be discussed to let privileged app use mozApps.mgmt
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → juhsu
Comment 1•10 years ago
|
||
Hey Junior - Is this bug to follow-up with the patch here: https://bug1000313.bugzilla.mozilla.org/attachment.cgi?id=8521141 ?
Blocks: 1000313
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #1) > Hey Junior - Is this bug to follow-up with the patch here: > https://bug1000313.bugzilla.mozilla.org/attachment.cgi?id=8521141 ? Yes, but it's a WIP patch. I'd like to claim explictly in webidl what's done in the patch of bug 1000313: Expose to |homescreen-webapps-manage| DOMRequest getAll(); DOMRequest uninstall(DOMApplication app); attribute EventHandler oninstall; attribute EventHandler onuninstall; attribute EventHandler onenabledstatechange; NOT Expose to |homescreen-webapps-manage| DOMRequest getNotInstalled(); void applyDownload(DOMApplication app); Promise<DOMApplication> import(Blob blob); Promise<any> extractManifest(Blob blob); void setEnabled(DOMApplication app, boolean state); Fabrice, I'd like to double-check if non-exposure of import/enable API is what we want. Could you please confirm that?
Flags: needinfo?(fabrice)
Assignee | ||
Comment 4•10 years ago
|
||
A short question: In Apps.webidl, |mozApps.mgmt| is of DOMApplicationsManager, which is [ChromeOnly]. However, functions such as |getAll| should be exposed to privileged apps with |homescreen-webapps-manage| permission. Should I remove [ChromeOnly] from |DOMApplicationsManager| and add [ChromeOnly] to methods like |getNotInstalled|? Code Snippet as [JSImplementation="@mozilla.org/webapps/manager;1"] interface DOMApplicationsManager : EventTarget { [CheckPermissions="webapps-manage homescreen-webapps-manage"] DOMRequest getAll(); [ChromeOnly, CheckPermissions="webapps-manage] DOMRequest getNotInstalled();
Flags: needinfo?(fabrice)
Comment 5•10 years ago
|
||
No, don't change the [ChromeOnly] on DOMApplicationsManager. It's there to prevent the interface to be exposed on the global. That would also make no sense to have getNotInstalled be chrome-only. I would focus only on the permissions.
Flags: needinfo?(fabrice)
Assignee | ||
Comment 6•10 years ago
|
||
The patch is for the webidl change. Will add tests in the next part after the change is agreed.
Attachment #8522891 -
Flags: review?(jonas)
Updated•10 years ago
|
Whiteboard: [ft:conndevices]
Target Milestone: --- → 2.2 S1 (5dec)
Comment 7•10 years ago
|
||
Any update here? Can anyone else review this code if Jonas is busy?
Attachment #8522891 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Since the change is granted, I'll add some tests.
Comment 9•10 years ago
|
||
Junior - I tried with the patch, but I'm seeing an error, though the error details have changed. Any idea if I'm doing something wrong? Here is the error I receive: [JavaScript Error: "TypeError: Return value of DOMApplicationsRegistry.mgmt is not an object."]
Flags: needinfo?(juhsu)
Comment 10•10 years ago
|
||
Is the pref/setting mapping at http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#82 working properly? It is used to decide whether we expose mgmt at http://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.js#269
Comment 11•10 years ago
|
||
Ah, ok that's probably it. I don't think this is going to work for third party developers then as you can't use WebIDE to develop home screens. Let's figure that out in another bug. Thanks for chiming in.
Assignee | ||
Comment 12•10 years ago
|
||
Fabrice and comment of bug 1000313[1] has explained. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1000313#c50 https://bugzilla.mozilla.org/show_bug.cgi?id=1000313#c52
Flags: needinfo?(juhsu)
Assignee | ||
Comment 13•10 years ago
|
||
Rebase, carry r+
Attachment #8522891 -
Attachment is obsolete: true
Attachment #8573859 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f25a7cb4fad Try seems fine. I'm going to write test
Assignee | ||
Comment 15•10 years ago
|
||
Two questions I have encountered: 1. inproc-app does not have its own permission. permissions are kinda per process. 2. fail to launch a signed app in mochitest-plain Therefore I'll try to launch a oop app and push permission by special power
Assignee | ||
Comment 16•10 years ago
|
||
test for permission |homescreen-webapps-manage| I encountered lots of wired things while cooking this test. Will describe them later. try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a94a76dac517
Attachment #8584419 -
Flags: review?(fabrice)
Comment 17•10 years ago
|
||
Comment on attachment 8584419 [details] [diff] [review] Part 2: test Review of attachment 8584419 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit fixed. thanks! ::: dom/apps/tests/test_third_party_homescreen.html @@ +21,5 @@ > +function runApp(aApp, aCallback) { > + var ifr = document.createElement('iframe'); > + ifr.setAttribute('mozbrowser', 'true'); > + ifr.setAttribute('mozapp', aApp.manifestURL); > + ifr.src = aApp.origin + aApp.manifest.launch_path; you need to resolve the launch_path against the app manifest url, not append to the origin. @@ +171,5 @@ > + info("Running " + app.manifestURL); > + runApp(app, continueTest); > + yield undefined; > + > + SpecialPowers.popPermissions(continueTest); I don't think you need to call that explicitly. The harness should do it for you when the test finishes.
Attachment #8584419 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 18•10 years ago
|
||
> @@ +171,5 @@ > > + info("Running " + app.manifestURL); > > + runApp(app, continueTest); > > + yield undefined; > > + > > + SpecialPowers.popPermissions(continueTest); > > I don't think you need to call that explicitly. The harness should do it for > you when the test finishes. It's a weird fact: If we don't call the |popPermissions| explicitly, the test will get stucked in |flushPermissions| in https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#1018 I haven't found the root cause, but I believe something went wrong if I push permissions in an inproc iframe.
Assignee | ||
Comment 19•10 years ago
|
||
I got the root cause of comment 18. Uninstall an app will remove all the permissions for the app. But |flushPermissions| needs a |perm-changes| notification to invoke the callback https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#876 Thus I'd like to keep the |popPermissions| before unstalling the app. Is that OK, Fabrice?
Flags: needinfo?(fabrice)
Assignee | ||
Comment 20•10 years ago
|
||
Modify by comment 17 but keep the |popPermissions| Carry r+ Btw, I list some facts I found: 1. The permissions of in-proc package app inherits from its embedder. Per Bug 1059662 comment 0, it's like a known issue. 2. I can not load a uri from a package app in mochitest-plain, even with webapps-manage permission. That's weird since I can load by handy push the uri to the awesome bar. 3. Inner page can not get |mozApps.mgmt|, thus causing me use |loadFrameScript|. I guess maybe it's blocked since the mozapp iframe is a hosted app. Not sure. 4. comment 18 and comment 19 describe a error-prone way if the permission pushed by |SpecialPowers.pushPermissions| is undone before the test finishes
Attachment #8584419 -
Attachment is obsolete: true
Attachment #8585916 -
Flags: review+
Comment 21•10 years ago
|
||
(In reply to Junior Hsu [:juniorhsu] from comment #19) > I got the root cause of comment 18. > Uninstall an app will remove all the permissions for the app. > But |flushPermissions| needs a |perm-changes| notification to invoke the > callback > https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/ > specialpowersAPI.js#876 > > Thus I'd like to keep the |popPermissions| before unstalling the app. > Is that OK, Fabrice? Yep, it's fine for me.
Flags: needinfo?(fabrice)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 22•10 years ago
|
||
Hi, Part 1 failed to apply: renamed 1097468 -> homescreen-webapps-manage applying homescreen-webapps-manage patching file dom/webidl/Apps.webidl Hunk #2 FAILED at 104 1 out of 2 hunks FAILED -- saving rejects to file dom/webidl/Apps.webidl.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh homescreen-webapps-manage could you take a look, thanks!
Flags: needinfo?(juhsu)
Keywords: checkin-needed
Assignee | ||
Comment 23•10 years ago
|
||
Rebase, carry r+.
Attachment #8573859 -
Attachment is obsolete: true
Flags: needinfo?(juhsu)
Attachment #8586598 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
rebase, carry r+
Attachment #8585916 -
Attachment is obsolete: true
Attachment #8586599 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 25•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a626a11ac38 https://hg.mozilla.org/integration/mozilla-inbound/rev/57d347aa0701
Keywords: checkin-needed
Comment 26•10 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8338752&repo=mozilla-inbound
Flags: needinfo?(juhsu)
Assignee | ||
Comment 27•10 years ago
|
||
test failures since bug 1059662 blocks embedding inproc app in content process. Skip the test case. Wait for try happy https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcb07319e500
Attachment #8586599 -
Attachment is obsolete: true
Flags: needinfo?(juhsu)
Attachment #8587164 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Target Milestone: 2.2 S1 (5dec) → ---
Comment 28•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/961fe9c61047 https://hg.mozilla.org/integration/b2g-inbound/rev/61480347343d
Flags: in-testsuite+
Keywords: checkin-needed
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/961fe9c61047 https://hg.mozilla.org/mozilla-central/rev/61480347343d
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•