1050097 - [Camera] Add error handling while |videoStorage.addNamed| occurs error.

Status: RESOLVED FIXED

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

Description

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

There is no error handling/message while |videoStorage.addNamed| occurs error.
This issue is a follow up from bug 147228.

Comment 1

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

Attachment: pull request: https://github.com/mozilla-b2g/gaia/pull/22612

Hi Diego,
   This is my patch for fixing this issue.
   Since |videoStorage.addNamed| occurs error, then all UI will be stuck.
   So I emit 'storage:createVideoError' and use |onRecordingError| to 'prompt dialog' let user know current status.

   I'm not sure this is a best way for fixing this issue, please help to review this patch and give me some feedback.
   
   Thanks.

Comment 2

[Peipei Cheng (needinfo if you need my action)]

[Blocking Requested - why for this release]:
since this blocks bug 1047228 which is 1.4+. should we also include this is v1.4?

Comment 3

[Diego Marcos [:dmarcos]]

Comment on attachment 8469070 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/22612

Thanks for the patch. I left my comments on github. r- because:

It's be better to pass the error on the function callback instead of triggering an event

Comment 4

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

Comment on attachment 8469070 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/22612

Hi Diego,
   I've addressed your comment and fixed unit tests, please help to review my patch again.
  
   Thanks.

Comment 5

[Diego Marcos [:dmarcos]]

Comment on attachment 8469070 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/22612

This starts to look really sweet! We're close. I made more suggestions on github. Mostly to keep consistency with the rest of the code base.

Comment 6

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

Comment on attachment 8469070 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/22612

Hi Diego,
   Please help to review this patch again, thanks.

Comment 7

[Diego Marcos [:dmarcos]]

Comment on attachment 8469070 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/22612

Yeah! It looks great. This needs to be uplifted right?

Comment 8

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

Hmmmmm... Need to get v1.4+ and approval then we can uplift.
Thanks for your review again.

Comment 9

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

merged in gaia master:
https://github.com/mozilla-b2g/gaia/commit/9ce01d8fa33acf4fad5a6a43cfeca1a8d715de7b.

Comment 10

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

landed in v1.4
https://github.com/mozilla-b2g/gaia/commit/ec395d47cee9234ae93d2410d8ef8724dfc5d6cb

Hi Wayne,
   Sorry, I made a mistake with changed v1.4+ to v1.4?, please help me to change it to right way.
   Thanks.

Comment 11

[Wayne Chang [:wchang]]

Thanks Gary.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Is this needed for v2.0 as well? If so, please nominate the patch for gaia v2.0 uplift.

Comment 13

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

Hi Pike,
  I've added v1.4 patch with new string, maybe we need l10n team support.
  Could you help to take a look? 
  Please refer to comment 10.
Thanks

Comment 14

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

landed in v2.0
https://github.com/mozilla-b2g/gaia/commit/e230621063cd37b328a1c6b0c13b1e43e28c8d54

Comment 15

[Francesco Lodolo [:flod]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> Is this needed for v2.0 as well? If so, please nominate the patch for gaia
> v2.0 uplift.

And then you commit on 2.0...

Please back it out. You can't land string changes on branches that are string frozen (anything besides master).

Comment 16

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

back my patch out on v2.0.
https://github.com/mozilla-b2g/gaia/commit/cade2fdbb2230670788dcf2fc7b100f4a37b6458#diff-d41d8cd98f00b204e9800998ecf8427e

Comment 17

[Francesco Lodolo [:flod]]

Thanks. I'm honestly confused by this bug: am I reading it wrong or this bug got approvals for 1.4 without any release-driver being involved? Is this supposed to happen?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(In reply to Francesco Lodolo [:flod] from comment #17)
> Thanks. I'm honestly confused by this bug: am I reading it wrong or this bug
> got approvals for 1.4 without any release-driver being involved? Is this
> supposed to happen?

Wayne is one of the 1.4 drivers.

That said, I'm backing this out from v1.4 as well until the string situation is sorted out. We're way past string freeze.

v1.4: https://github.com/mozilla-b2g/gaia/commit/296f95231db42d25d9b92867489d5ba954c663e0

And like flod said, this *must* have v2.0 approval before landing there. 1.4 blocking status does not grant auto-approval as of a few weeks ago. It was announced on dev-gaia when the change was made and the B2G Landing Page also says the same.
https://wiki.mozilla.org/Release_Management/B2G_Landing

Comment 19

[Wayne Chang [:wchang]]

Apologies for missing the string changes implied by this bug. I'll talk to Gary and clarify the string situation and update here.

Comment 20

[bhavana bajaj [:bajaj]]

Wayne, please adhere to string freeze here. For 2.0 I don't think flame is impacted by 1047228, so this bug would be a wontfix as the primary reason for blocking this on 1.4 was 1047228

Comment 21

[Wayne Chang [:wchang]]

After discussing with Gary and others, this bug is a fail safe for cases where 1047228 do not cover. Ideally bug 1047228 should resolve situations where this bug would occur.
Removing 1.4+ here, the two bugs are not dependent on each other. Leaving bug 1047228 as POVB for partner to pick up.