930358 - Use manifestURL instead of app origin to identify a keyboard (IME) app

Status: RESOLVED FIXED

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

Description

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

According to Fabrice's advice below, we should use manifest URL instead of app origin to identify a keyboard app.

----
The manifest URL must be used to identify an app. 
Currently we support only one app per origin, but that will change so the origin won't be appropriate anymore.
---

With this change, we need to look into keyboard_helper and what we store the enabled layouts in mozSettings, and the build time config about keyboard layouts. See the dependency bugs for details,


** For Triage **
Nominate this as koi+, since this is an important part of 3rd-party IME framework.

Comment 1

[Ivan Tsay (:ITsay)]

Plus it based on Rudy's comment

Comment 2

[Corey Frang [:gnarf]]

Rudy - I'm all over this chunk of the codebase and would be happy to start making this sweeping change.  I'm assuming it's not a big problem to have 912010 land then a few patches later change the format which breaks the default settings stored in the meantime?

Comment 3

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Yes, I think it is ok to land Bug 912010 first.
Thanks.

Comment 4

[Corey Frang [:gnarf]]

Rudy, do you want me to take this?  It's blocking a bug I'm assigned to anyway and would be happy to start making this change everywhere.

Comment 5

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I think it is already night at your time zone, so if you can wait then I'll fix this today.

And then you fix another bug based on this? Sounds good?

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Reduce v1.2 scope as there is no partner request on shipping a phone with preloaded 3rd-party keyboards.

If this patch is completed in time and needed for other koi+ bug, or it is deemed safer to uplift to v1.2 than not (judging by the maintenance cost of the branch, stability etc), I will be happy to a+ the patch.

Comment 7

[Corey Frang [:gnarf]]

I think this is more about supporting multiple manifests in a packaged third party app than it is about a partner with preloaded 3rd-party keyboards.  If 1.2 supports multiple manifests in an app, this is definitely still koi+ Tim

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Corey Frang [:gnarf] from comment #7)
> I think this is more about supporting multiple manifests in a packaged third
> party app than it is about a partner with preloaded 3rd-party keyboards.  If
> 1.2 supports multiple manifests in an app, this is definitely still koi+ Tim

I have never heard such requirement for v1.2, but Fabrice could confirm.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Triage: let's fix this in v1.3 as a follow-up.

Comment 10

[Corey Frang [:gnarf]]

Another thing to mention is that it's probably easiest to do this now, when the format of the setting is already changing, otherwise we will have to add back compat for 1.2 settings to convert into the format for 1.3.  If we know we want/need to do this, there is no harm in "doing it now".  Basically "appOrigin" is >not< a unique key if we plan to allow multiple manifests per app.

Comment 11

[[:fabrice] Fabrice Desré]

nit: We don't want multiple manifests per app, but multiple apps per origin, each app being identified by its manifest url.

But I really don't think that it's ok to land new code that uses origin and not manifest url. We've been saying that for a long time now.

Comment 12

[Corey Frang [:gnarf]]

Then this should be a koi+ so that the new settings code properly uses manifestURL?

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Corey Frang [:gnarf] from comment #12)
> Then this should be a koi+ so that the new settings code properly uses
> manifestURL?

We can't block on that, but we must fix the bug and request uplift approval.

Comment 14

[Corey Frang [:gnarf]]

Rudy, do you still plan on having a fix for this soon? I really think this needs to land with all that code from bug 912010.  So much so that I offered to take care of this part of the problem inside my work for bug 912010! :)

I really think it would be a pretty big mistake to ship the new settings code I put into 912010 only to have to write code to be back-compat with settings from the 1.2 devices in the 1.3 cycle.  We >just< changed how these settings are stored, we should change it to be right in this release.

I also imagine since appOrigin is not a unique key, this is just waiting for a bunch of little bugs with keyboard apps that share app.origin to come out and bite us in the 1.2 cycle.

Comment 15

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I would also like to address this issue in v1.2, but will focus on koi+ bugs, Bug913784 and Bug913783 first.
Will make the "system language -> keyboard layout" mapping correct first in those 2 bugs and then come back to make manifestURL change here.

Comment 16

[Jason Smith [:jsmith]]

This is not a committed feature for 1.3, so this should not block the release. 3rd party keyboard support can ride the trains to 1.3 if it's safe enough risk-wise.

Comment 17

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V1

Hi all,

As Comment 0, we should use manifestURL instead of appOrigin as an app's ID.

Corey, could you help take a look at the keyboard_helper part and the related tests?

Arthur, Gary, please help look at settings and keyboard manager, respectively.
Thanks.

Comment 18

[Corey Frang [:gnarf]]

Comment on attachment 8342456 [details] [review]
Patch V1

f+ from me - Looking great!  Couple of tiny nits and a bigger question, who's answer is fairly insignificant in the grand scheme of landing this.  Switched from a review flag to feedback since I'm not a peer here!

Comment 19

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Comment on attachment 8342456 [details] [review]
Patch V1

good to me, r=me.
Thank you, Rudy.

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8342456 [details] [review]
Patch V1

r=me for the settings part. Thanks!

Comment 21

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Merge to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/846b677b4975639dc61fce28e21d36d724f38160

--
Gentlemen, thanks for the review.

Comment 22

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8342456 [details] [review]
Patch V1

Hoping to get this into v1.2 for Bug 944290, data migration issue of keyboard settings.

If this issue does not get into v1.2, it would be much harder to address Bug 944290, a koi+ bug, while considering v1.2 -> v1.3 migration.

--
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): N/A, this is a refinement.
[User impact] if declined: N/A.
[Testing completed]: Yes, travis and munaul test.
[Risk to taking this patch] (and alternatives if risky): low.
[String changes made]: N/A

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8342456 [details] [review]
Patch V1

I think your reason is valid but I believe we actually created a dependency (thus, making this bug koi+) so this patch should go to 1.2 without needing approval.

Comment 24

[Jason Smith [:jsmith]]

Comment on attachment 8342456 [details] [review]
Patch V1

We do not accept approvals for 1.2 anymore. Only blockers can land.

Comment 25

[Corey Frang [:gnarf]]

Jason, this needs to land in 1.2 to make life easier for everyone... It should be a blocker. 

Read my reasoning on https://bugzilla.mozilla.org/show_bug.cgi?id=888253#c14 - This code needs to be in 1.2, it makes life sane again, only requiring one "settings database upgrade" in 1.2, and then not needing to write 2 versions of the patch for 888253, and then another "version" of this patch which can upgrade the settings format from the 1.2 format to the 1.3 format.

We should just make this patch a part of 1.2, so that we only end up with 2 keyboard settings formats, pre 1.2 and post 1.2

Comment 26

[Jason Smith [:jsmith]]

(In reply to Corey Frang [:gnarf] from comment #25)
> Jason, this needs to land in 1.2 to make life easier for everyone... It
> should be a blocker. 
> 
> Read my reasoning on https://bugzilla.mozilla.org/show_bug.cgi?id=888253#c14
> - This code needs to be in 1.2, it makes life sane again, only requiring one
> "settings database upgrade" in 1.2, and then not needing to write 2 versions
> of the patch for 888253, and then another "version" of this patch which can
> upgrade the settings format from the 1.2 format to the 1.3 format.
> 
> We should just make this patch a part of 1.2, so that we only end up with 2
> keyboard settings formats, pre 1.2 and post 1.2

I don't think that makes a strong case for blocking on this. It's perfectly normal to have cases where you have a branch-specific patch for 1.2 because 1.3 has a different workflow in the implementation.

This can only be a blocker if this as a stand-alone can block itself.

Comment 27

[Corey Frang [:gnarf]]

(In reply to Jason Smith [:jsmith] from comment #26)
> This can only be a blocker if this as a stand-alone can block itself.


It was already blocking koi+ when it started it's life: https://bugzilla.mozilla.org/show_bug.cgi?id=930358#c1

This should definitely be a part of the 3rd party framework changes in 1.2, as was mentioned in the original post of the bug.

Also, by >not< taking this into 1.2, you are forcing pretty much triple the work out of the engineering team, and increasing the risk of settings being lost over upgrades (adding another setting migration step to 1.3)...

This patch will need another follow up to migrate settings in whatever version it lands, and the other patch will need to be written twice.

Or we can take this code we already have written into 1.2, land bug 888235 with it, and be already done with the headaches.

Comment 28

[Jason Smith [:jsmith]]

(In reply to Corey Frang [:gnarf] from comment #27)
> (In reply to Jason Smith [:jsmith] from comment #26)
> > This can only be a blocker if this as a stand-alone can block itself.
> 
> 
> It was already blocking koi+ when it started it's life:
> https://bugzilla.mozilla.org/show_bug.cgi?id=930358#c1
> 
> This should definitely be a part of the 3rd party framework changes in 1.2,
> as was mentioned in the original post of the bug.

Note - it was koi+ originally because this was part of the 3rd party framework work, but that work moved to be shipped in 1.4.

> 
> Also, by >not< taking this into 1.2, you are forcing pretty much triple the
> work out of the engineering team, and increasing the risk of settings being
> lost over upgrades (adding another setting migration step to 1.3)...
> 
> This patch will need another follow up to migrate settings in whatever
> version it lands, and the other patch will need to be written twice.
> 
> Or we can take this code we already have written into 1.2, land bug 888235
> with it, and be already done with the headaches.

If there's a strong concern with risk here that warrants taking this patch, then that seems okay to nominate this bug for. That seems like a case we can make in triage to block on this.

Comment 29

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Uplifted to v1.2,
https://github.com/mozilla-b2g/gaia/commit/6d02039072a2ae5cf9225a6f4c78ed49decfab5c