Closed Bug 1197700 Opened 9 years ago Closed 9 years ago

Correct mistakes in InputMethod.webidl

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file, 5 obsolete files)

InputMethod.webidl does not actually describe what the API do currently. Preferably we should normalize that first before working on anything else.

My patch will probably be a little bit opinionated but I hope we are going toward the right direction.
Summary: Normalize mistakes in InputMethod.webidl → Correct mistakes in InputMethod.webidl
Attached patch bug1197700.patch (obsolete) — Splinter Review
This patch re-formats the Web IDL file to normalize the style of the comment, indents, and properly document and enforce the dictionary and enum types. The value of InputContext#type is finally correctly implemented too (see fixed test).

I've also remove the permission check in MozKeyboard.js entirely and rely on Web IDL binding to enforce that. The permission checking on IPC messages remains.

Jan, I wonder if you have some spare time for the review? One thing to watch out is the the "inputType" v.s. "type" in the IPC message sent -- I think I've changed all of them. Unfortunately we can't test mozChromeEvents -- I intend to remove them one by one after this bug.

Kanru, could you help me review the permission bits? Since :masayuki said he is not familiar with B2G permissions...

Olli, I am sorry that the IDL contains many white space changes. As stated the while space changes are about styling so the part to super-review is the newly added dictionaries and enums, and permissions.

Thanks!
Attachment #8653963 - Flags: superreview?(bugs)
Attachment #8653963 - Flags: review?(kchen)
Attachment #8653963 - Flags: review?(janjongboom)
Comment on attachment 8653963 [details] [diff] [review]
bug1197700.patch

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

I only checked the webidl file. The permissions looks fine. But the patch introduced more trailing white spaces in the comments, maybe by accident?
Attachment #8653963 - Flags: review?(kchen) → review+
(In reply to Kan-Ru Chen [:kanru] from comment #2)
> Comment on attachment 8653963 [details] [diff] [review]
> bug1197700.patch
> 
> Review of attachment 8653963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I only checked the webidl file. The permissions looks fine. But the patch
> introduced more trailing white spaces in the comments, maybe by accident?

Thanks, white space removed.
Comment on attachment 8653963 [details] [diff] [review]
bug1197700.patch

It is a bit odd to have so finegrained  permission handling in MozInputMethod
(like, why doesn't input-manage have access to all non-chrome-only stuff).
But fine.
Attachment #8653963 - Flags: superreview?(bugs) → superreview+
I guess you removed it already, but:

bug1197700.patch:344: trailing whitespace.
 * element and the input content (a.k.a. input app, virtual keyboard app,
bug1197700.patch:347: trailing whitespace.
 * The API also contains a few Gaia System app only methods
bug1197700.patch:514: trailing whitespace.
   * OS should sliently ignore this request if the app is currently not the
bug1197700.patch:523: trailing whitespace.
   * OS should sliently ignore this request if the app is currently not the
bug1197700.patch:537: trailing whitespace.
   *

Building now...
Comment on attachment 8653963 [details] [diff] [review]
bug1197700.patch

I really think this should have been in 2 separate patches tbh.

Anyway: we should change the way 'blur' works. It was messy already but that is not cleaned up, just adds more code and the if statement that we have in handleFocusChange is a sign that this is not a clean protocol from forms.js -> MozKeyboard -> system app. I think we should add a new message type in forms.js:unhandleFocus like "Forms:Input:Blur" or something.

> * Group "text". Be reminded that contenteditable element is assigned with
> * a type of "textarea".
Should be 'an input type of', at least it was not clear from me from the patch file. If you're just reading the IDL file it might be alright.

Anyway: based on this patch I'm really wondering why we need the 'type' field on a context anyway? We're not ever gonna show a different keyboard whether it's input, textarea or contenteditable. Can we just remove the type property? Do we even use it in Gaia? I can't really think of a place where we do...
Attachment #8653963 - Flags: review?(janjongboom) → review-
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #6)
> Comment on attachment 8653963 [details] [diff] [review]
> bug1197700.patch
> 
> I really think this should have been in 2 separate patches tbh.
> 
> Anyway: we should change the way 'blur' works. It was messy already but that
> is not cleaned up, just adds more code and the if statement that we have in
> handleFocusChange is a sign that this is not a clean protocol from forms.js
> -> MozKeyboard -> system app. I think we should add a new message type in
> forms.js:unhandleFocus like "Forms:Input:Blur" or something.
> 

I can fix this in this patch if you want.

> > * Group "text". Be reminded that contenteditable element is assigned with
> > * a type of "textarea".
> Should be 'an input type of', at least it was not clear from me from the
> patch file. If you're just reading the IDL file it might be alright.
> 
> Anyway: based on this patch I'm really wondering why we need the 'type'
> field on a context anyway? We're not ever gonna show a different keyboard
> whether it's input, textarea or contenteditable. Can we just remove the type
> property? Do we even use it in Gaia? I can't really think of a place where
> we do...

You are right, we don't use that in Gaia Keyboard because it recognizes every possible |inputType| and |inputType| of each of the |type| doesn't overlap. It's possible some other keyboard doesn't handle |inputType| though, and I am just finishing up what's originally planned for |type|. Do you still think I should remove it?
Attached patch bug1197700.patch (obsolete) — Splinter Review
This patches address the problem raised in the previous comment. It is indeed more clear.

The local build have not finish yet but I have pushed to try first.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=242273eac9d9

(Carrying over superreivew+ from the previous patch)
Attachment #8653963 - Attachment is obsolete: true
Attachment #8658514 - Flags: superreview+
Attachment #8658514 - Flags: review?(janjongboom)
Comment on attachment 8658514 [details] [diff] [review]
bug1197700.patch

... and it doesn't work locally.
Attachment #8658514 - Attachment is obsolete: true
Attachment #8658514 - Flags: review?(janjongboom)
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #7)
> You are right, we don't use that in Gaia Keyboard because it recognizes
> every possible |inputType| and |inputType| of each of the |type| doesn't
> overlap. It's possible some other keyboard doesn't handle |inputType|
> though, and I am just finishing up what's originally planned for |type|. Do
> you still think I should remove it?

Actually, since we assign contenteditable with inputType=textarea, the only way to distinguish the two would be to inspect the |type| attribute. You can see that by looking at the test.
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #11)
> Actually, since we assign contenteditable with inputType=textarea, the only
> way to distinguish the two would be to inspect the |type| attribute. You can
> see that by looking at the test.

... the test of bug 1201407 which is more exhaust than test_basic.html.
Comment on attachment 8658519 [details] [diff] [review]
bug1197700.patch

r=me for dom/inputmethod/.

@tim, I'd add the following to this patch:

diff --git a/dom/inputmethod/mochitest/test_bug1059163.html b/dom/inputmethod/mochitest/test_bug1059163.html
index 79da5d9..12b1173 100644
--- a/dom/inputmethod/mochitest/test_bug1059163.html
+++ b/dom/inputmethod/mochitest/test_bug1059163.html
@@ -54,6 +54,9 @@ function runTest() {
     mm.loadFrameScript('data:,(' + encodeURIComponent(appFrameScript.toString()) + ')();', false);

     im.oninputcontextchange = function() {
+      is(im.inputcontext.type, 'contenteditable', 'type');
+      is(im.inputcontext.inputType, 'textarea', 'inputType');
+
       if (im.inputcontext) {
         im.oninputcontextchange = null;
         register();
Attachment #8658519 - Flags: review?(janjongboom) → review+
Attached patch Patch to commit (obsolete) — Splinter Review
Attachment #8658519 - Attachment is obsolete: true
Attachment #8659023 - Flags: superreview+
Attachment #8659023 - Flags: review+
sorry had to back this out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=13873434&repo=mozilla-inbound
Flags: needinfo?(timdream)
Attached patch bug1197700.patch (obsolete) — Splinter Review
I think below is what's needed since the patch now hides the constructors behind a permission. Strangely, MozInputMethod (constructor of navigator.mozInputMethod) is not listed in the test.

:smaug, could you bless the additional change with review? Thanks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67c162f24e2e

diff --git a/dom/tests/mochitest/general/test_interfaces.html b/dom/tests/mochitest/general/test_interfaces.html
--- a/dom/tests/mochitest/general/test_interfaces.html
+++ b/dom/tests/mochitest/general/test_interfaces.html
@@ -781,19 +781,19 @@ var interfaceNamesInGlobalScope =
     "MozContactChangeEvent",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     "MozCSSKeyframeRule",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     "MozCSSKeyframesRule",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozEmergencyCbModeEvent", b2g: true},
 // IMPORTANT: Do not change this list without review from a DOM peer!
-    {name: "MozInputContext", b2g: true},
-// IMPORTANT: Do not change this list without review from a DOM peer!
-    {name: "MozInputMethodManager", b2g: true},
+    {name: "MozInputContext", b2g: true, permission: ["input"]},
+// IMPORTANT: Do not change this list without review from a DOM peer!
+    {name: "MozInputMethodManager", b2g: true, permission: ["input"]},
 // IMPORTANT: Do not change this list without review from a DOM peer!
     "MozMmsMessage",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozMobileCellInfo", b2g: true},
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozMobileConnection", b2g: true},
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozMobileConnectionArray", b2g: true},
Attachment #8659023 - Attachment is obsolete: true
Flags: needinfo?(timdream)
Attachment #8659268 - Flags: superreview?(bugs)
Attachment #8659268 - Flags: review+
Attachment #8659268 - Flags: superreview?(bugs) → superreview+
Attached patch Patch to commentSplinter Review
Fix one error spotted in bug 1201407 comment 16.
Attachment #8659734 - Flags: superreview+
Attachment #8659734 - Flags: review+
Attachment #8659268 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/dc7a86c09438
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.