992948 - Use the telephony-call-ended system message to fill call log

Status: RESOLVED FIXED

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

Description

[Etienne Segonzac (:etienne)]

In prep for bug 990003.
This will remove one callscreen-appwindow dependency.

Comment 1

[Etienne Segonzac (:etienne)]

1.3T? because it's blocking a 1.3t+

Comment 2

[Etienne Segonzac (:etienne)]

Attachment: Gaia Part (PR)

Hey, sorry for the emergency review (blocking a big 1.3t), but we're gaining in coverage :)

Comment 3

[Etienne Segonzac (:etienne)]

Attachment: Gecko part (patch)

Hey Hsin-Yi, are you available to review this small gecko ril patch?
(it's blocking a pretty big 1.3t :/)

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8402759 [details] [diff] [review]
Gecko part (patch)

Review of attachment 8402759 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this!!!

Comment 5

[Etienne Segonzac (:etienne)]

The checking-needed is for the gecko part only. It can safely land before the gaia part.

Comment 6

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ for resolving dialer incoming call performance

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/c911553312a0

Comment 8

[Etienne Segonzac (:etienne)]

Re-opening for the gaia part, sorry I didn't put the correct leave-open keyword, will do next time :)

Comment 9

[Etienne Segonzac (:etienne)]

Not sure why the tarako_ony flag was set...
But this will land on master too.

Vivien can you help with making sure the gecko part is checked in where it needs? A bit lost here.

Comment 10

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

Ryan,
sorry for the confusion here. This patch is intended to help with the work beeing done in bug 990003 which tries to make the call screen appears instantly under heavy workload condition.

While that is something which is way more obvious on Tarako, this is something that is needed everywhere. Do you mind to uplift this patch to aurora and central as well ? Thanks.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I can't help you with the Aurora uplift unless it gets approval (per the B2G Landing page, 1.3T+ doesn't grant approval for 1.4). Setting checkin-needed for trunk.

Comment 12

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

Comment on attachment 8402759 [details] [diff] [review]
Gecko part (patch)

Risk is very small since this patch only adds a new property to an existing message and does not change any logic on the Gecko side.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8402759 [details] [diff] [review]
Gecko part (patch)

not affecting gecko is a good thing - uplift!

Comment 14

[Anthony Ricaud (:rik)]

Comment on attachment 8402757 [details] [review]
Gaia Part (PR)

Yeah for less code and more tests!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c3ef5671229c

Comment 16

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

The gecko seems to be in; The gaia didn't patch cleanly.  Maybe there's something I did wrong?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c3ef5671229c

Comment 18

[Etienne Segonzac (:etienne)]

https://github.com/etiennesegonzac/gaia/commit/b3055042687fa2dd1a4f99eb4069662404c53e1a

Comment 19

[thomas tsai]

should this be uplift to v1.3t?

Comment 20

[ying.xu]

Would you please rebase your patch on v1.3t branch?
There're some conflicts when cherry-pick this into v1.3t branch.

#	双方修改：     apps/communications/dialer/js/calls_handler.js
#	双方修改：     apps/communications/dialer/js/dialer.js
#	双方修改：     apps/communications/dialer/js/handled_call.js
#	双方修改：     apps/communications/dialer/test/unit/calls_handler_test.js
#	双方修改：     apps/communications/dialer/test/unit/dialer_test.js
#	双方修改：     apps/communications/dialer/test/unit/handled_call_test.js

Comment 21

[Etienne Segonzac (:etienne)]

Gaia part uplifted to 1.3t (Gecko part already landed there): https://github.com/mozilla-b2g/gaia/commit/23488b1a45221c17e6a32fdd4c9d0fdbdcf2d021

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99430f6e5e24

v1.4: https://github.com/mozilla-b2g/gaia/commit/28716364c7a9e43e6911c74e3f80ab37af0422b7

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/99430f6e5e24
> 
> v1.4:
> https://github.com/mozilla-b2g/gaia/commit/
> 28716364c7a9e43e6911c74e3f80ab37af0422b7

This seems to have broken gaia-unit tests on Aurora: https://tbpl.mozilla.org/php/getParsedLog.php?id=38438424&tree=Mozilla-Aurora

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Gaia patch reverted:
v1.4: https://github.com/mozilla-b2g/gaia/commit/4eb2393e6a5174e6c732167866aded046c49ed97

Comment 25

[Ivan Tsay (:ITsay)]

Comment on attachment 8402757 [details] [review]
Gaia Part (PR)

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 #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 26

[Ivan Tsay (:ITsay)]

Hi Etienne,

Mind if you can share if the broken issue on gaia-unit tests in v1.4 has been resolved. We need this patch in v1.4 since it is the blocker for partner's test.

Comment 27

[Preeti Raghunath(:Preeti)]

Ivan

Please fill in the responses into this form.


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 #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 28

[Etienne Segonzac (:etienne)]

Attachment: Patch for 1.4 with tests working

Comment 29

[Preeti Raghunath(:Preeti)]

Comment on attachment 8402757 [details] [review]
Gaia Part (PR)

No response on impact so not taking in 1.4 yet. Clearing noms till response is received.

Comment 30

[Ivan Tsay (:ITsay)]

Comment on attachment 8402757 [details] [review]
Gaia Part (PR)

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 #):
[User impact] if declined: Partner blocker on Dolphin
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A

Comment 31

[Etienne Segonzac (:etienne)]

(In reply to Preeti Raghunath(:Preeti) from comment #29)
> Comment on attachment 8402757 [details] [review]
> Gaia Part (PR)
> 
> No response on impact so not taking in 1.4 yet. Clearing noms till response
> is received.

Well it's a strong dependence of bug 990003 which is already 1.4+ [1] so I'm not sure why it has to get through approval again...

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=990003#c77

Comment 32

[Jason Smith [:jsmith]]

Comment on attachment 8402757 [details] [review]
Gaia Part (PR)

See https://bugzilla.mozilla.org/show_bug.cgi?id=990003#c85.

Comment 33

[Ivan Tsay (:ITsay)]

Hi Gabriele,

Once the 1.4 compatible patch is ready, please help to submit "approval-gaia-v1.4=?" request for this bug to be uplifted to v1.4. Thanks you very much.

Comment 34

[Gabriele Svelto [:gsvelto]]

Etienne had already provided a v1.4 patch for this in attachment 8432412 [details] [diff] [review] but for some reason it doesn't apply on my tree on top of bug 993018. I'm getting a clean uplift once I've applied the patch in bug 993018 with only a couple of unit-tests failing so I'll try to make that into a workable patch.

Comment 35

[Gabriele Svelto [:gsvelto]]

Attachment: Cherry-picked patch for v1.4

This is a clean cherry-pick from master that applies on top of attachment 8446514 [details] [diff] [review], there were no conflicts while cherry-picking however two unit-tests were failing locally due to some information contained by a mock leaking from a test suite to another. I added a one-liner fix for the issue in this patch. This is a dependency of bug 990003 which should be uplifted to v1.4.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Decoupling needed for bug 990003
[User impact] if declined: Cannot uplift bug 990003 to v1.4
[Testing completed]: The patch required only a small change to a mock but no changes to the actual logic and has already been used in master for a while
[Risk to taking this patch] (and alternatives if risky): Uplifting the dependencies without this patch is unrealistic
[String changes made]: None

Quick question for Etienne, you've already made attachment 8432412 [details] [diff] [review] for this bug but it doesn't apply cleanly on top of the patch in bug 993018. Can you provide some information on that? Unless attachment 8432412 [details] [diff] [review] contained some additional important changes I'm not aware of I'd rather pick this one which is a straightforward cherry-pick from master.

Comment 36

[Etienne Segonzac (:etienne)]

(In reply to Gabriele Svelto [:gsvelto] from comment #35)
> Quick question for Etienne, you've already made attachment 8432412 [details] [diff] [review]
> [diff] [review] for this bug but it doesn't apply cleanly on top of the
> patch in bug 993018. Can you provide some information on that? Unless
> attachment 8432412 [details] [diff] [review] contained some additional
> important changes I'm not aware of I'd rather pick this one which is a
> straightforward cherry-pick from master.

No IIRC it was only test related changes (probably removing some too!), so please go ahead with the version that applies cleanly :)

Comment 37

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8432412 [details] [diff] [review]
Patch for 1.4 with tests working

Thanks for the info Etienne, I'm marking this patch as obsolete to avoid confusion.

Comment 38

[Ivan Tsay (:ITsay)]

Hi Lawrence,

Please help to check this bug for the 1.4 uplift approval. Thanks.

Comment 39

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8446565 [details] [diff] [review]
Cherry-picked patch for v1.4

Approved for 1.4.

Comment 40

[Gabriele Svelto [:gsvelto]]

Uplifted to gaia/v1.4 543b7b49cfa0c687e84662d58af594a19c4cca56

https://github.com/mozilla-b2g/gaia/commit/543b7b49cfa0c687e84662d58af594a19c4cca56