Closed Bug 941885 Opened 11 years ago Closed 11 years ago

Pref Off 3rd Party Keyboard Support on 1.2 and master

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::System
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

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

People

(Reporter: jsmith, Assigned: timdream)

References

Details

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

Attachments

(3 files, 2 obsolete files)

2013-11-22-15-17-58.png
14.41 KB, image/png
Details
mozilla-b2g:master PR#13967
46 bytes, text/x-github-pull-request
rudyl
: review+
Details | Review
mozilla-b2g:v1.2 PR#13968
46 bytes, text/x-github-pull-request
rudyl
: review+
Details | Review 
Per a discussion with Fabrice & Andreas, 3rd party keyboard needs to be turned off for 1.3, whether that means backed out or preffed off.
Blocks: 3rd-party-keyboard
blocking-b2g: --- → koi+
Whiteboard: [3rd-party-keyboard]
(In reply to Jason Smith [:jsmith] from comment #0)
> Per a discussion with Fabrice & Andreas, 3rd party keyboard needs to be
> turned off for 1.3, whether that means backed out or preffed off.

Meant to say - turned off for 1.2
With little information on this side of the ocean, I could try to figure out what's the requirement here based on discussion happen in this office.

It's too risky to backout anything -- disable in v1.2 is the only available option here. However, we would have to keep it enabled in master so that we could begin testing and identify the gap between now and production.

We could always disable it again in v1.3 branch when it branches out.

I will come up with a patch again later today.
Assignee: nobody → timdream
Assignee: timdream → nobody
Assignee: nobody → timdream
Summary: Backout or Pref Off 3rd Party Keyboard Support for 1.2 → Pref Off 3rd Party Keyboard Support for 1.2
Comment on attachment 8336650 [details] [review]
mozilla-b2g:master PR#13945

After looking up app_install_manager.js, I realized I cannot prevent an input packaged app from being installed by just looking at it's updateManifest, since it does not contain information on it's type/role. So I ended up did the pref in keyboard_help.js.

When 'keyboard.3rd-party-app.enabled' is set to true, keyboard apps will be run out-of-process, and UI for selecting layouts other than Gaia keyboard apps will be shown. When set to false, keyboard apps will be run in-process and UI will not show any non-Gaia keyboard apps (so it's impossible launch any of them even if you manage to install it).

Developer option is being removed too so the value doesn't get flipped during runtime. Only developers with root access will be able to flip the value.

The option is set to true in master, and false in v1.2 branch.
Attachment #8336650 - Flags: review?(rlu)
Comment on attachment 8336653 [details] [review]
mozilla-b2g:v1.2 PR#13946

This is the same patch commit on v1.2 with additional commit to turn the pref off.
Attachment #8336653 - Flags: review?(rlu)
Comment on attachment 8336650 [details] [review]
mozilla-b2g:master PR#13945

Looks good.
Thanks.
Attachment #8336650 - Flags: review?(rlu) → review+
Comment on attachment 8336653 [details] [review]
mozilla-b2g:v1.2 PR#13946

r=me. Thank you.
Attachment #8336653 - Flags: review?(rlu) → review+
This has broken the keyboard (for me at least) on M-C + Master. The keyboard comes up over the page (hides it) and whenever a key is touched the keyboard hides again... and the key isn't send to the form. Rolling back this commit fixes it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #10)
> This has broken the keyboard (for me at least) on M-C + Master. The keyboard
> comes up over the page (hides it) and whenever a key is touched the keyboard
> hides again... and the key isn't send to the form. Rolling back this commit
> fixes it.

You need to update Gecko to include bug 847763, or simply disable keyboard oop on master build by flipping the settings entry.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Attached image 2013-11-22-15-17-58.png — 
This patch has caused a large white box to appear above the keyboard.

The white box is perfectly sized to obscure the whole of the remaining app area (see screenshot).

Everything is still functional, but the main app space is hidden by this box.

I think we should back this patch out.
For above comment # 15:

Base: V1.2_US_20131115.cfg
Gaia:     13687c598c3b5ef4cff9eef8d0ed3bb57c5f9d65
Gecko:    http://hg.mozilla.org/mozilla-central/rev/dbf94e314cde
BuildID   20131122040202
Version   28.0a1
Device Hamachi
John - Can you back this out?
Flags: needinfo?(jhford)
(In reply to Jason Smith [:jsmith] from comment #17)
> John - Can you back this out?

I am backing out the master patch myself. Sorry for not checking B2G/Desktop first.
Flags: needinfo?(jhford)
master revert: 2cc78d696482e0434b584f5645af55e3105e59a2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #19)
> master revert: 2cc78d696482e0434b584f5645af55e3105e59a2

Does this also need to be reverted on 1.2 as well?
To prevent this bug popping up in koi+ query, if the breakage doesn't get resolved in days I will re-land but disable the patch on master, and figure out what it takes to enable it on another bug.

Monday.
(In reply to Jason Smith [:jsmith] from comment #20)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #19)
> > master revert: 2cc78d696482e0434b584f5645af55e3105e59a2
> 
> Does this also need to be reverted on 1.2 as well?

No. v1.2 is fine. It's landed and disabled (unless for some strange reason people found the same problem in v1.2 branch)
William - Can you do some negative testing here to see what happens in this patch to pref off 3rd party keyboards?
Flags: needinfo?(whsu)
Keywords: verifyme
v1.2 revert: 5a0802f44565ac9a17f5a720259ac2689ab67ff7

I have other concern with my own patch. Will submit a patch for re-review and reland.
status-b2g-v1.2: fixed → affected
Flags: needinfo?(whsu)
Keywords: verifyme
Comment on attachment 8337222 [details] [review]
mozilla-b2g:master PR#13967

This patch will now disable the feature one master branch too until we found what is going on in comment 15.
Attachment #8337222 - Flags: review?(rlu)
Comment on attachment 8337223 [details] [review]
mozilla-b2g:v1.2 PR#13968

v1.2 patch with updated unit tests.
Attachment #8337223 - Flags: review?(rlu)
Attachment #8336650 - Attachment is obsolete: true
Attachment #8336653 - Attachment is obsolete: true
Comment on attachment 8337222 [details] [review]
mozilla-b2g:master PR#13967

r=me.
Thanks.
Attachment #8337222 - Flags: review?(rlu) → review+
Comment on attachment 8337223 [details] [review]
mozilla-b2g:v1.2 PR#13968

r+ as well.
Attachment #8337223 - Flags: review?(rlu) → review+
Whats the plan for 1.3? Do we have a list of bugs we need to have confidence to let this ride the 1.3 train?
(In reply to Andreas Gal :gal from comment #31)
> Whats the plan for 1.3? Do we have a list of bugs we need to have confidence
> to let this ride the 1.3 train?

I thought we are not supposed to add feature to v1.3 now?

Some improvements has already been working on in bug 835404 and friends -- stability of 3rd-party keyboard app itself would need to be identified this week.
Tagging verifyme per request in comment 23.
Flags: needinfo?(whsu)
Keywords: verifyme
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #32)
> (In reply to Andreas Gal :gal from comment #31)
> > Whats the plan for 1.3? Do we have a list of bugs we need to have confidence
> > to let this ride the 1.3 train?
> 
> I thought we are not supposed to add feature to v1.3 now?
> 
> Some improvements has already been working on in bug 835404 and friends --
> stability of 3rd-party keyboard app itself would need to be identified this
> week.

Since this missed 1.2, its definitely in scope for 1.3 (with high urgency). If you think you are really close, I am sure we can negotiate with drivers to try for 1.3 (I am pretty sure nobody will fight that). The idea is to not drag this out into the stabilization window (for 1.2 we are 2 1/2 months into that). If you are close we can enable this for 1.3 and see if its in a better shape on that train (seems so). 1.3 stabilization is only about to start. If we see more bugs as we go through 1.3 stabilization, we will face the same decision. If not, its in. Everyone wants this feature in, just not at the expense of blowing up the schedule.

Note: We have a external dependency on this work for 1.4, so we can as well try for 1.3, because no latter than 1.4 we will have to block on this. There would be some benefit of getting the chance to test this in the 1.3 cycle (and have people write external keyboards).
(In reply to Andreas Gal :gal from comment #35)
> Since this missed 1.2, its definitely in scope for 1.3 (with high urgency).
> If you think you are really close, I am sure we can negotiate with drivers
> to try for 1.3 (I am pretty sure nobody will fight that). The idea is to not
> drag this out into the stabilization window (for 1.2 we are 2 1/2 months
> into that). If you are close we can enable this for 1.3 and see if its in a
> better shape on that train (seems so). 1.3 stabilization is only about to
> start. If we see more bugs as we go through 1.3 stabilization, we will face
> the same decision. If not, its in. Everyone wants this feature in, just not
> at the expense of blowing up the schedule.
> 
> Note: We have a external dependency on this work for 1.4, so we can as well
> try for 1.3, because no latter than 1.4 we will have to block on this. There
> would be some benefit of getting the chance to test this in the 1.3 cycle
> (and have people write external keyboards).

We are working on evaluating feasibility of the feature for v1.3, start by finding the cause of comment 15. It looks like we hit a 0-day(?) regression unrelated to keyboard. I've filed bug 942788 on that.

Bug 942790 filed to re-enable keyboard oop (and 3rd-party keyboard support in general) once the said bug is resolved.
Hi, Tim,

My bad. Too many trivial things occupied my resource so late replied you.
-----------------------------------------------------------------------
I did the following tests.
1. Installation test of third party apps ( Side effect checking )
   => Pass
2. Installation test of third party keyboard
   => Fail. I can install third party via app manager and it works on FxOS.
3. Process monitoring of third party keyboard (* This test depends on second test)
   => Fail. I can see a process is initialized

* Test Result:
  I didn't see any side effect on third party "app".
  But, while I used App Manager to do the test, I found
  (1) The third party keyboard can be installed.
  (2) I can see the third party on the keyboard settings page
  (3) The third party keyboard still works on FxOS.
  So, we need to handle this case and prevent user to install third party keyboard via app manager.
  Anyway, I will discuss this problem with You and Rudy later.

* Test Build:
 - Gaia:     92cd11ea023dd6598d82d859ae3c945ff6589ce6
 - Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/14e91ab12441
 - BuildID   20131127004001
 - Version   26.0


By the way, I can see the keyboard process was created on V1.3 build.
I will start to test OOP feature on V1.3 build.

william@tpemozilla:~/Workspace_B2G $ adb shell ps | grep b2g
root      109   1     168312 63508 ffffffff 4013d4e0 S /system/b2g/b2g
app_375   375   109   78760  29332 ffffffff 400ee4e0 S /system/b2g/plugin-container <System>
app_950   950   109   74728  27592 ffffffff 401244e0 S /system/b2g/plugin-container
app_1013  1013  109   82424  28320 ffffffff 400124e0 S /system/b2g/plugin-container
app_1051  1051  109   69536  25380 ffffffff 400794e0 S /system/b2g/plugin-container <Message app>
app_1086  1086  109   82072  32488 ffffffff 400964e0 S /system/b2g/plugin-container <Keyboard process>
root      1090  109   66344  22648 ffffffff 400e24e0 S /system/b2g/plugin-container

Many thanks.
Have a nice day!
Flags: needinfo?(whsu)
--------- Keep Needinfo myself to remind myself ---------
Flags: needinfo?(whsu)
Hi, Tim,

Thanks for aggressively handle this case.
Per we discussed test result this morning, the test results were impacted by preloaded keyboard app and test environment because I flashed the engineering build.
So, I retested the it.

1. Installation test of third party apps ( Side effect checking )
   => Pass
2. Installation test of third party keyboard
   => Pass. But, as we discussed, the installation UI is easy to confuse user.
3. Process monitoring of third party keyboard (* This test depends on second test)
   => Pass.

<Process>
APPLICATION      USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
b2g              root      136   1     198244 72872 ffffffff 4010c604 S /system/b2g/b2g
Homescreen       app_441   441   136   73472  31360 ffffffff 40021604 S /system/b2g/plugin-container
Usage            app_479   479   136   66296  26636 ffffffff 4012c604 S /system/b2g/plugin-container
Settings         app_680   680   136   72576  31424 ffffffff 400df604 S /system/b2g/plugin-container
Messages         app_704   704   136   68476  28884 ffffffff 40115604 S /system/b2g/plugin-container
(Pre-allocated)  root      766   136   63108  22732 ffffffff 40029604 S /system/b2g/plugin-container

I would like to say the patch work as we expected but still have UI defect.
Many thanks!
Flags: needinfo?(whsu)
Status: RESOLVED → VERIFIED
Can you open a followup bug for the item you've found?
Keywords: verifyme
(In reply to Jason Smith [:jsmith] from comment #40)
> Can you open a followup bug for the item you've found?

Please file the bug on installation UI and assign to me, thanks, i.e. "[v1.2] Installation UI still shown during keyboard app installation".
Flags: needinfo?(whsu)
Hi, Tim,

Thanks for the reminder.
I submitted the bug - Bug 944295
Flags: needinfo?(whsu)
Status: VERIFIED → RESOLVED
Closed: 11 years ago → 11 years ago
Summary: Pref Off 3rd Party Keyboard Support for 1.2 → Pref Off 3rd Party Keyboard Support on 1.2 and master
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: