Closed Bug 930358 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: rudyl, Assigned: rudyl)

References

Details

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

Attachments

(1 file)

46 bytes, text/x-github-pull-request
arthurcc
: review+
GaryChen
: review+
gnarf
: feedback+
Details | Review
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.
Plus it based on Rudy's comment
Assignee: nobody → rlu
blocking-b2g: koi? → koi+
Target Milestone: --- → 1.2 C4(Nov8)
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?
Yes, I think it is ok to land Bug 912010 first.
Thanks.
Whiteboard: [3rd-party-keyboard]
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.
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?
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.
blocking-b2g: koi+ → 1.3?
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
Flags: needinfo?(timdream)
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(timdream)
Triage: let's fix this in v1.3 as a follow-up.
blocking-b2g: 1.3? → 1.3+
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.
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.
Flags: needinfo?(fabrice)
Then this should be a koi+ so that the new settings code properly uses manifestURL?
(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.
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.
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.
Target Milestone: 1.2 C4(Nov8) → 1.3 Sprint 5 - 11/22
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
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.
blocking-b2g: 1.3+ → 1.3?
Attached file 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.
Attachment #8342456 - Flags: review?(gnarf37)
Attachment #8342456 - Flags: review?(gchen)
Attachment #8342456 - Flags: review?(arthur.chen)
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!
Attachment #8342456 - Flags: review?(gnarf37) → feedback+
Comment on attachment 8342456 [details] [review]
Patch V1

good to me, r=me.
Thank you, Rudy.
Attachment #8342456 - Flags: review?(gchen) → review+
Comment on attachment 8342456 [details] [review]
Patch V1

r=me for the settings part. Thanks!
Attachment #8342456 - Flags: review?(arthur.chen) → review+
Merge to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/846b677b4975639dc61fce28e21d36d724f38160

--
Gentlemen, thanks for the review.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
Attachment #8342456 - Flags: approval-gaia-v1.2?(timdream)
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.
Attachment #8342456 - Flags: approval-gaia-v1.2?(timdream) → approval-gaia-v1.2+
Comment on attachment 8342456 [details] [review]
Patch V1

We do not accept approvals for 1.2 anymore. Only blockers can land.
Attachment #8342456 - Flags: approval-gaia-v1.2+ → approval-gaia-v1.2-
Blocks: 888253
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
(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.
(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.
(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.
blocking-b2g: 1.3? → koi?
blocking-b2g: koi? → koi+
You need to log in before you can comment on or make changes to this bug.