Closed Bug 942790 Opened 6 years ago Closed 6 years ago

Re-enable keyboard oop

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C1/1.4 S1(20dec)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

(Whiteboard: [3rd-party-keyboard][ft:system-platform])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #941885 +++

We ran into platform issue bug 942788 when switching keyboard oop on. This is somehow unfortunate because the regression happened around the same time. After that bug is resolved we need to re-enable keyboard oop on master branch to unlock development.
blocking-b2g: --- → 1.3?
Comment on attachment 8338202 [details] [review]
mozilla-b2g:master PR#14037

This must land AFTER revert of bug 917416 reaches m-c.
Attachment #8338202 - Flags: review?(rlu)
Comment on attachment 8338202 [details] [review]
mozilla-b2g:master PR#14037

Thanks.
Attachment #8338202 - Flags: review?(rlu) → review+
Assignee: nobody → timdream
blocking-b2g: 1.3? → 1.3+
Blocks: 930402
master: https://github.com/mozilla-b2g/gaia/commit/afa10908940f8d84f9e0477deeeba6569bf780a1
No longer blocks: 930402
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Jason, this has nothing to do with bug 930402.
Tim, does that mean that we now run all keyboards OOP? If so, I first want performance timings without OOP to make sure we don't regress what is already a pain point performance-wise.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #5)
> Jason, this has nothing to do with bug 930402.

No. This was marked on this bug because it was dependent on having 3rd party keyboard enabled. Re-adding the dependency.
Blocks: 930402
(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #5)
> > Jason, this has nothing to do with bug 930402.
> 
> No. This was marked on this bug because it was dependent on having 3rd party
> keyboard enabled. Re-adding the dependency.

Jason, you really need to read the context of both bugs.

This bug enables 3rd-party keyboard along with oop. Enabling this feature (literally, "allowing 3rd-party keyboard app to be installed") has nothing to do with the behavior of the default keyboard app (bug 930402), and the two fixes have no dependency at all. I am not sure what the dependency you are taking about since I am taking care of the both bugs and I am suppose the person to understand everything.

I appreciate your time and effort to touch every bug and make sure they are directed the proper persons, but it would be better if you give more rationale when tagging bugs.

If you are looking at tagging bug with "hey, this is related to overall 3rd-party keyboard support work", or "hey, this behavior change is related to 3rd-party keyboard support work", there is a whiteboard flag and bug 816869 for that.
No longer blocks: 930402
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #8)
> (In reply to Jason Smith [:jsmith] from comment #7)
> > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> > comment #5)
> > > Jason, this has nothing to do with bug 930402.
> > 
> > No. This was marked on this bug because it was dependent on having 3rd party
> > keyboard enabled. Re-adding the dependency.
> 
> Jason, you really need to read the context of both bugs.

Everyone in triage read bug 930402 to be applicable to third-party keyboards - the bug title in the other bug said "[e.me] Support for 3rd party keyboards." That immediately reads off as a requirement as part of third-party keyboard work.

> 
> This bug enables 3rd-party keyboard along with oop. Enabling this feature
> (literally, "allowing 3rd-party keyboard app to be installed") has nothing
> to do with the behavior of the default keyboard app (bug 930402), and the
> two fixes have no dependency at all. I am not sure what the dependency you
> are taking about since I am taking care of the both bugs and I am suppose
> the person to understand everything.

Then next time I would suggest responding to a inquiry when someone asks your for team's input to triage something (https://bugzilla.mozilla.org/show_bug.cgi?id=930402#c5). We had to make a decision here with low context because repeated inquiries did not get us information we needed from your team. As a result, we had make a decision based on what people knew in that triage session.

I also do not see how the original definition of bug 930420 has anything to do with default keyboard alone - the discussion in the bug is talking heavily about differences with introduction of third-party keyboards. The bug has been morphed however at this point, so it's literally confusing to anyone what's being tracked there at this point.

> 
> I appreciate your time and effort to touch every bug and make sure they are
> directed the proper persons, but it would be better if you give more
> rationale when tagging bugs.

That was a triage decision. The people in triage had to make a decision with the context we knew. We did ask for input, but you didn't respond.
Could OOP keyboard help performance by allowing parallel painting on dual core devices?
(In reply to Andreas Gal :gal from comment #10)
> Could OOP keyboard help performance by allowing parallel painting on dual
> core devices?

Maybe, but we'll also have slightly slower IPC (and we do two much of that - 8 async js messages per touch tap) so I'd like to get Eiderticker measurements.
(In reply to Jason Smith [:jsmith] from comment #9)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> > This bug enables 3rd-party keyboard along with oop. Enabling this feature
> > (literally, "allowing 3rd-party keyboard app to be installed") has nothing
> > to do with the behavior of the default keyboard app (bug 930402), and the
> > two fixes have no dependency at all. I am not sure what the dependency you
> > are taking about since I am taking care of the both bugs and I am suppose
> > the person to understand everything.
> 
> Then next time I would suggest responding to a inquiry when someone asks
> your for team's input to triage something
> (https://bugzilla.mozilla.org/show_bug.cgi?id=930402#c5). We had to make a
> decision here with low context because repeated inquiries did not get us
> information we needed from your team. As a result, we had make a decision
> based on what people knew in that triage session.

For the record, I did provide an explanation on bug 930402 comment 9 asking e.me to move away from relying on keyboard.current mozSetting value. Given the fact Ran decline to do that, I morph the bug in bug 930402 comment 18 so that built-in keyboard could continue provide the information to e.me legacy code.

It's reasonable to say I didn't provide enough information between bug 930402 comment 5 to  bug 930402 comment 8, but it's not reasonable say the cause of confusion on flipping flags in comment 7 is because I did not provide enough information (note the date).

Now, can we get back to work?
Public update per comment 6, comment 10, and comment 11: an e-mail has been sent to loop Jed and William on possible performance issues. Bugs to be file if they are.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #12)
> For the record, I did provide an explanation on bug 930402 comment 9 asking
> e.me to move away from relying on keyboard.current mozSetting value. Given
> the fact Ran decline to do that, I morph the bug in bug 930402 comment 18 so
> that built-in keyboard could continue provide the information to e.me legacy
> code.
> 
> It's reasonable to say I didn't provide enough information between bug
> 930402 comment 5 to  bug 930402 comment 8, but it's not reasonable say the
> cause of confusion on flipping flags in comment 7 is because I did not
> provide enough information (note the date).

Re-read bug 930402 I realized I had given the same information to Ran ~1 month ago in bug 930402 comment 2. I don't understand why triage session failed to read that.
This patch caused a serious performance hit to all of our keyboard UI automation - the keyboard is now taking way too long too load across the entire phone. We need to back this out.
[master 03e6e87] Revert "Merge pull request #14037 from timdream/keyboard-oop-re
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 944009
Given the fact that we're putting forth a heavy effort to prevent crash landings in the last two weeks of 1.3 feature dev, I think this needs to wait until 1.4. We've already seen some pretty bad fallout per comment 15 from the followup attempt of enabling oop for 3rd party keyboards. This also is not a committed feature for 1.3.
blocking-b2g: 1.3+ → 1.3?
Jason, OOP keyboard/keyboard framework was already committed for 1.2. We can discuss slipping this again, but thats of course really not great (but we will if there is no other way). Could you and Tim talk directly and discuss the risks and a test plan? I would like the two of you to make a joint recommendation whether we can salvage the OOP work for 1.3 and what the concerns and risks are. Thanks!
(In reply to Andreas Gal :gal from comment #18)
> Jason, OOP keyboard/keyboard framework was already committed for 1.2. We can
> discuss slipping this again, but thats of course really not great (but we
> will if there is no other way). Could you and Tim talk directly and discuss
> the risks and a test plan? I would like the two of you to make a joint
> recommendation whether we can salvage the OOP work for 1.3 and what the
> concerns and risks are. Thanks!

Sure. I'll followup with Tim offline.
Also, just trust the Sheriff...
Whiteboard: [3rd-party-keyboard] → [3rd-party-keyboard][ucid:SystemPlatform46, 1.4:p1, ft:system-platform]
Moving to 1.4? per offline discussion - we'll be keeping this preffed off for 1.3.
blocking-b2g: 1.3? → 1.4?
Plus it for v1.4 as the committed feature.
blocking-b2g: 1.4? → 1.4+
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
Log some test result here, after testing a local build with the patch in bug 944397 applied, we still could reproduce the case that sometimes the built-in would be shown after you click on an input field.

BTW, according to William, the automation test could pass.
--

Repro steps for the above issue
==============================
1. Try to launch several apps, like contacts, browser, gallery, etc.
2. Use b2g-ps to see when keyboard app is OOMed.
3. Go to an input field and click on it => sometimes the keyboard would not show up.
Stealing this bug from Tim.
Assignee: timdream → rlu
(In reply to Rudy Lu [:rudyl] from comment #23)
> Log some test result here, after testing a local build with the patch in bug
> 944397 applied, we still could reproduce the case that sometimes the
> built-in would be shown after you click on an input field.
> 
> BTW, according to William, the automation test could pass.
> --
> 
> Repro steps for the above issue
> ==============================
> 1. Try to launch several apps, like contacts, browser, gallery, etc.
> 2. Use b2g-ps to see when keyboard app is OOMed.
> 3. Go to an input field and click on it => sometimes the keyboard would not
> show up.

Thanks Rudy,

This is intermittent issue, it does not always occur but keyboard would not show up in low percentage after OOM.

I am trying to investigate the root cause.
No longer blocks: 810686
After further testing, Bug 950573 is the root cause for both symptoms:
 1. As comment 24 stated, the keyboard would not show up. 
 2. As Bug 950573's original description, the keyboard would show up but the input function does not work.

So, I guess we are blocked by Bug 944397 only to re-enable keyboard OOP.
William, do you support the above estimation? Should we attempt to re-enable keyboard oop this week? If QA is confident on this I would like to have this resolved before end-of-year. Thanks!
Flags: needinfo?(whsu)
Hi, Tim,

Sure. If we have solved all block issues of keyboard OOP, I suggest to re-enable it on V1.4 since we can stabilize it in early stage.

Many thanks!
Flags: needinfo?(whsu)
I will re-land this bug today so it could hit TBPL tonight and smoketest tomorrow. Hopefully we could get this working this time.
Assignee: rlu → timdream
master: ef87cccd69aba806b4fc5bc79991bf9a7132714e
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 956125
Depends on: 970188
Depends on: 970193
Move 3rd party keyboard feature meta to bug 964670
Whiteboard: [3rd-party-keyboard][ucid:SystemPlatform46, 1.4:p1, ft:system-platform] → [3rd-party-keyboard][ft:system-platform]
Move depending to bug to correct use story bug.
No longer depends on: 970188, 970193
Blocks: 964670
No longer blocks: 1.4-system-platform
You need to log in before you can comment on or make changes to this bug.