874769 - [Gaia] Use new WebAPI to access card state related attribute/event.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Edgar Chen [:edgar]]

In bug 874744, we plan to move card state related attribute/event from mozMobileConnection to mozIccManager. This bug is filed in Gaia side to make corresponding changes. Thanks.

Comment 1

[Edgar Chen [:edgar]]

Working branch: https://github.com/EdgarChen/gaia/commits/bug_874769

Comment 2

[Edgar Chen [:edgar]]

Attachment: gaia pull request #10451

Comment 3

[Edgar Chen [:edgar]]

Comment on attachment 764002 [details]
gaia pull request #10451

bug 874744 is already r+ and sr+. I would like to request review for this pull request first. Because cardState is used by many apps, so I split the patch into 6 parts, each part is for the changes of a specific app. I plan to merge the patches of Gaia and Gecko together after all got r+. Thanks

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 764002 [details]
gaia pull request #10451

r+ to the contacts part,

Thanks Edgar for taking care of this!

Comment 5

[Fernando Campo (:fcampo)]

Comment on attachment 764002 [details]
gaia pull request #10451

FTU looks good too, thanks!

Comment 6

[Salvador de la Puente González [:salva]]

I'm to wait until the BE patch lands as it is a key part on Cost Control and I want to try if something is broken.

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 764002 [details]
gaia pull request #10451

r=me on the dialer part.

Thank you for taking the time to craft this nice, green, pull request :)

Comment 8

[Edgar Chen [:edgar]]

(In reply to Salvador de la Puente González [:salva] from comment #6)
> I'm to wait until the BE patch lands as it is a key part on Cost Control and
> I want to try if something is broken.

Got it, please let me know when BE patch is landed, I will rebase for it. Thanks. :)

Comment 9

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

Comment on attachment 764002 [details]
gaia pull request #10451

Nice work! Thanks for taking care of this. r=me.

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 764002 [details]
gaia pull request #10451

r+.
Is there any followup plan to move remaining iccinfo/iccinfochange to IccManager? This seems be on the halfway of the final target.

Comment 11

[Edgar Chen [:edgar]]

(In reply to Alive Kuo [:alive] from comment #10)
> Comment on attachment 764002 [details]
> gaia pull request #10451
> 
> r+.
> Is there any followup plan to move remaining iccinfo/iccinfochange to
> IccManager? This seems be on the halfway of the final target.

Yes, please see bug 875721 and bug 877978. Thanks

Comment 12

[Salvador de la Puente González [:salva]]

(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> (In reply to Salvador de la Puente González [:salva] from comment #6)
> > I'm to wait until the BE patch lands as it is a key part on Cost Control and
> > I want to try if something is broken.
> 
> Got it, please let me know when BE patch is landed, I will rebase for it.
> Thanks. :)

The back-end part is yours as well (bug 874744), if you provide me an image to test with it, then I could review this part.

Comment 13

[Edgar Chen [:edgar]]

(In reply to Salvador de la Puente González [:salva] from comment #12)
> (In reply to Edgar Chen [:edgar][:echen] from comment #8)
> > (In reply to Salvador de la Puente González [:salva] from comment #6)
> > > I'm to wait until the BE patch lands as it is a key part on Cost Control and
> > > I want to try if something is broken.
> > 
> > Got it, please let me know when BE patch is landed, I will rebase for it.
> > Thanks. :)
> 
> The back-end part is yours as well (bug 874744), if you provide me an image
> to test with it, then I could review this part.

Ok, I can provide the test image.

Comment 14

[Edgar Chen [:edgar]]

BTW, in bug 884274, :julienw mention that many of gaia developers use gaia master + gecko b2g18 to work. The back-end changes will be only happened in gecko m-c. Maybe we should consider to make this changes can support both gecko m-c and gecko b2g18. Any thoughts?

Comment 15

[Edgar Chen [:edgar]]

Bug 884274 has created a helper for backward compatibility between gecko m-c and b2g18 for Icc API changes. We can use this for card state as well. I am going to provide a new version using this helper, will update later.

Comment 16

[Edgar Chen [:edgar]]

Attachment: gaia pull request #10735

gaia pull request #10735 (Using IccHelper)

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 770055 [details]
gaia pull request #10735

Salvador

Comment 18

[Edgar Chen [:edgar]]

Comment on attachment 770055 [details]
gaia pull request #10735

The new patches use IccHelper to access card state related events and attributes for the backward compatibility of gaia master on b2g18.
Salvador, Francisco, Fernando, Etienne, Arthur, Alive, please help to review the changes, thanks.

Comment 19

[Etienne Segonzac (:etienne)]

Comment on attachment 770055 [details]
gaia pull request #10735

Commented on github. We should lazy load the IccHelper in the dialer part.

Comment 20

[Salvador de la Puente González [:salva]]

Comment on attachment 770055 [details]
gaia pull request #10735

It seems to completely work. Fine for me.

Comment 21

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 770055 [details]
gaia pull request #10735

r+ to the contacts part once addressed the comment on github.

Thanks a lot Edgar!

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 770055 [details]
gaia pull request #10735

Happy API wrapper.

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 770055 [details]
gaia pull request #10735

Update new patch for dialer (User lazy loader), request review again. Thanks.

Comment 24

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

Comment on attachment 770055 [details]
gaia pull request #10735

Thank you for the effort, Edgar! r=me.

Comment 25

[Etienne Segonzac (:etienne)]

Comment on attachment 770055 [details]
gaia pull request #10735

Thanks! All good for the dialer part.

Comment 26

[Edgar Chen [:edgar]]

Hi :arcturus, due to :fcampo is on PTO, could you help to review FTU part for me as well? Thanks in advance!

Comment 27

[Francisco Jordano [:arcturus] [:francisco]]

Hi Edgar,

no problem I'll review the FTU part :)

Thanks!

Comment 28

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 770055 [details]
gaia pull request #10735

FTU part checked on the phone.

Working perfectly, thanks a lot Edgar!

Comment 29

[Edgar Chen [:edgar]]

Thanks for the reviewing, everyone!

Comment 30

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

master: https://github.com/mozilla-b2g/gaia/commit/2f49c776a10357a5fe0ed2796439a6ce23d7e101

Comment 31

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Arthur Chen [:arthurcc] from comment #30)
> master:
> https://github.com/mozilla-b2g/gaia/commit/
> 2f49c776a10357a5fe0ed2796439a6ce23d7e101

I assume this patch has caused a 100ms startup regression on both contacts and dialer early july. The regression has been hidden because the we switched a little bit after from b2g-18 to m-c which is a bigger regression.

I bet that the main reason for this is because mozMobileConnection is expensive and should any calls related to it should be delayed when it is needed while the current icc_helper.js code makes a call to mozMobileConnection during app startup.

Ben, Kevin can you guys checks my assertion and make the appropriate fixes to icc_helper.js to avoid calling mozMobileConnection is it is not needed.

Comment 32

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Also one of Francisco comment has not been addressed if i understood correcty. The script is useless in the contacts app if you don't go to settings so he asked to lazy load it instead of always loading it.

Comment 33

[Kevin Grandon :kgrandon]

It feels like this would generally be fixed if we delay the calling of things that need icc_helper. I've opened bug 913540 to track this work. We'll work on prioritizing this bug and submitting patches where needed. Thanks!