Closed Bug 915570 Opened 11 years ago Closed 11 years ago

Rename 'keyboard' permission and role

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g koi+
Tracking Flags:
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: xyuan, Assigned: timdream)

References

Details

(Whiteboard: [3rd-party-keyboard])

Attachments

(3 files, 2 obsolete files)

Gaia patch, to be land *before* Gecko: https://github.com/mozilla-b2g/gaia/pull/13255
(deleted), text/x-github-pull-request
rudyl
: review+
rudyl
: feedback+
Details
Patch for commit
(deleted), patch
Details | Diff | Splinter Review
Github: https://github.com/mozilla-b2g/gaia/pull/13304
(deleted), text/x-github-pull-request
rudyl
: review+
Details
I'd like to rename the permission and role of 'keyboard' to 'inputmethod'. Because we may have kind of voice input method without keyboard in the future, 'inputmethod' is more appropriate for input method apps.
I really don't care that strongly, so definitely leave the decision up to you here. However "input method" doesn't mean a lot to people that hasn't worked with complex scripts, so people in most of the americas and europe. Maybe "userinput" would be better, or simply "input"? But again, totally your decision.
"input" sounds good to me and Google chrome extension uses this name as well.
Yuan, how about moving this bug forward for v1.2? I guess for this change to land, we need some subtleties here to make Gaia and Gecko synced in order not to break the integration tests. Can you add support for permission=input (I guess Gecko won't take role into consideration now, right?). And then I can update Gaia part? Thank you.
Whiteboard: [3rd-party-keyboard]
OK, I'll be responsible for the gecko changes.
Assignee: nobody → xyuan
Stealing, if all goes well in bug 920977. Also koi+'ing the bug since we should finalize the permission/role string before 3rd-party developer consumption.
Assignee: xyuan → timdream
blocking-b2g: --- → koi+
Should I rename 'inputmethod-manage' permission to 'input-manage' too?
Flags: needinfo?(jonas)
Attached patch WIP v1 (obsolete) (deleted) — — Splinter Review
So I changed keyboard to input and inputmethod-manage to input-manage. I searched codebase with inputmethod-manage, but keyboard is a generic word so I ended locating the strings from bug 838308. I hope this is all we need. Note that this patch is on top of bug 920977.
Attachment #825174 - Flags: feedback?(xyuan)
Component: Gaia::Keyboard → General
Comment on attachment 825174 [details] [diff] [review] WIP v1 Review of attachment 825174 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/inputmethod/Keyboard.jsm @@ +110,5 @@ > return; > } > > + if (!mm.assertPermission("input") || > + !mm.assertPermission("input-manage")) { We only need to check |input| permission here. |assertPermission| will kill the process if the permission is not equal. If the mm doesn't have |input| permission, the process will be killed and has not chance to check |input-manage| permission.
Attachment #825174 - Flags: feedback?(xyuan)
(In reply to Yuan Xulei [:yxl] from comment #8) > Comment on attachment 825174 [details] [diff] [review] > WIP v1 > > Review of attachment 825174 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/inputmethod/Keyboard.jsm > @@ +110,5 @@ > > return; > > } > > > > + if (!mm.assertPermission("input") || > > + !mm.assertPermission("input-manage")) { > > We only need to check |input| permission here. |assertPermission| will kill > the process if the permission is not equal. If the mm doesn't have |input| > permission, the process will be killed and has not chance to check > |input-manage| permission. LOL, I was thinking about *maybe* we would have an app that comes with only input-manage permission but not input permission. How do we test that? Is that necessary in your opinion?
Flags: needinfo?(xyuan)
I duplicate the permissions in the manifest to prevent bustage here. Both to be tested together manually.
Attachment #825215 - Flags: feedback?(rlu)
Comment on attachment 825215 [details] Gaia patch, to be land *before* Gecko: https://github.com/mozilla-b2g/gaia/pull/13255 Looks good to me. Thanks.
Attachment #825215 - Flags: feedback?(rlu) → feedback+
Attached patch WIP v2 (obsolete) (deleted) — — Splinter Review
Need to include bug 920831.
Attachment #825174 - Attachment is obsolete: true
Comment on attachment 825729 [details] [diff] [review] WIP v2 The patches are ready for review because try passed on Gecko patch and Travis errors are unrelated. I've also manually tested the patches together.
Attachment #825729 - Flags: review?(xyuan)
Comment on attachment 825215 [details] Gaia patch, to be land *before* Gecko: https://github.com/mozilla-b2g/gaia/pull/13255 The patches are ready for review because try passed on Gecko patch and Travis errors are unrelated. I've also manually tested the patches together.
Attachment #825215 - Flags: review?(rlu)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #9) > (In reply to Yuan Xulei [:yxl] from comment #8) > > Comment on attachment 825174 [details] [diff] [review] > > WIP v1 > > > > Review of attachment 825174 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/inputmethod/Keyboard.jsm > > @@ +110,5 @@ > > > return; > > > } > > > > > > + if (!mm.assertPermission("input") || > > > + !mm.assertPermission("input-manage")) { > > > > We only need to check |input| permission here. |assertPermission| will kill > > the process if the permission is not equal. If the mm doesn't have |input| > > permission, the process will be killed and has not chance to check > > |input-manage| permission. > > LOL, I was thinking about *maybe* we would have an app that comes with only > input-manage permission but not input permission. How do we test that? Is > that necessary in your opinion? One way I think is separating the messages into two types. One for system app and one for keyboard app. Each type of the message will test against its own permission type. I perfer to single permission for the system app, but it is an optional improvement, not a requirement.
Comment on attachment 825215 [details] Gaia patch, to be land *before* Gecko: https://github.com/mozilla-b2g/gaia/pull/13255 r=me. Thank you.
Attachment #825215 - Flags: review?(rlu) → review+
Comment on attachment 825729 [details] [diff] [review] WIP v2 Review of attachment 825729 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/inputmethod/Keyboard.jsm @@ +110,5 @@ > return; > } > > + if (!mm.assertPermission("input") || > + !mm.assertPermission("input-manage")) { if |!mm.assertPermission("input")| is false, the process will be killed, and we cannot reach |mm.assertPermission("input-manage")|. So we don't need to check the second permission.
Attachment #825729 - Flags: review?(xyuan) → review+
Gaia, master: https://github.com/mozilla-b2g/gaia/commit/54f3e81d56061ccfbe59f03472bc3470737b346f I will push another patch to remove the old permission after Gecko part have landed.
Attached patch Patch for commit (deleted) — — Splinter Review
Not flagging checkin-needed until I land the Gaia patch to both master and v1.2.
PS I am waiting for bug 912010 to settle on v1.2 before uplift this patch.
Now I am waiting for bug 931977 and bug 928281. I would like to make sure we have enough tbpl test runs to isolate the breakage (if any of the patches have caused it).
Attachment #825729 - Attachment is obsolete: true
Setting status-b2g-v1.2 to affected so I won't forget to uplift this to b2g26 :) https://hg.mozilla.org/integration/b2g-inbound/rev/2d0ee3cec349
status-b2g-v1.2: fixed → affected
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2d0ee3cec349
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Let's push a part 2 on the same bug to remove the old permissions.
Attachment #826264 - Flags: review?(rlu)
I didn't rename test-ime, so here it is: master: 06b931c937215404d70bd96dfde9a170f26b50e4 v1.2: 99975756a26761e2e7d575211338b0803e0a67f2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: