Closed Bug 1141958 Opened 7 years ago Closed 7 years ago

[B2G][Camera] Refine the change event handler in storage.js

Categories

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

ARM
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alchen, Assigned: alchen)

References

Details

Attachments

(2 files, 1 obsolete file)

Since I am trying to trigger an change event when default location is changed.(bug 1115496), I double check the codes which listen to change event.

Here is the event handler of camera app.
https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/storage.js#L162

When there is change event happened and the reason is not 'deleted', it will set the state as "reason".
For now, the reason can be "available", "unavailable", "shared", "low-disk-space", "available-disk-space".

Basically the state of device storage should be "available" or "unavailable" or "shared". These three states should be the state you need to take care.

"low-disk-space" and "available-disk-space" are for monitoring low storage condition. I think we should not set the state as these two reasons. It would be great if we can refine the change event handler.

Here is what most apps do in their change event handler.
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/mediadb.js#L776
Hi Wilson,
could you give some feedbacks about this change?
Attachment #8575877 - Flags: feedback?(wilsonpage)
Attachment #8575877 - Flags: feedback?(wilsonpage) → feedback?(wilsonpage)
Comment on attachment 8575877 [details]
Refine the change event handler to prevent some unexpected reasons

Hi Mikeh,
Since Wilson is PTO, could you help on this?
Thanks a lot.
Attachment #8575877 - Flags: feedback?(mhabicher)
Blocks: 1115496
Comment on attachment 8575877 [details]
Refine the change event handler to prevent some unexpected reasons

This looks fine to me, but justindarc knows more about the Camera app.
Attachment #8575877 - Flags: feedback?(mhabicher)
Attachment #8575877 - Flags: feedback?(jdarcangelo)
Attachment #8575877 - Flags: feedback+
Comment on attachment 8575877 [details]
Refine the change event handler to prevent some unexpected reasons

LGTM
Attachment #8575877 - Flags: feedback?(jdarcangelo) → feedback+
Comment on attachment 8577039 [details] [review]
[gaia] alphan102:bug1141958 > mozilla-b2g:master

Hi justindarc,
please help to review this pull request.
Thanks.
Attachment #8577039 - Flags: review?(jdarcangelo)
Comment on attachment 8577039 [details] [review]
[gaia] alphan102:bug1141958 > mozilla-b2g:master

Can you please update the storage unit tests to add a test that makes sure that only available/unavailable/shared cause setState() to be called? Thanks!
Attachment #8577039 - Flags: review?(jdarcangelo) → review-
Attachment #8575877 - Flags: feedback?(wilsonpage)
(In reply to Justin D'Arcangelo [:justindarc] from comment #7)
> Comment on attachment 8577039 [details] [review]
> [gaia] alphan102:bug1141958 > mozilla-b2g:master
> 
> Can you please update the storage unit tests to add a test that makes sure
> that only available/unavailable/shared cause setState() to be called? Thanks!

Hi Justin,
Could you explain more detail about the test you want?
-> Send changes event with different reasons first ?!
-> Check if the handler only call setState() in the case we want ?!
Actually, I am not familiar with the camera unit test.
It would be nice if I can get more tips.


By the way, there are one more case to call setState. (Set the state as "nospace")
https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/storage.js#L282
Flags: needinfo?(jdarcangelo)
(In reply to Alphan Chen [:Alphan] from comment #8)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #7)
> > Comment on attachment 8577039 [details] [review]
> > [gaia] alphan102:bug1141958 > mozilla-b2g:master
> > 
> > Can you please update the storage unit tests to add a test that makes sure
> > that only available/unavailable/shared cause setState() to be called? Thanks!
> 
> Hi Justin,
> Could you explain more detail about the test you want?
> -> Send changes event with different reasons first ?!
> -> Check if the handler only call setState() in the case we want ?!
> Actually, I am not familiar with the camera unit test.
> It would be nice if I can get more tips.
> 
> 
> By the way, there are one more case to call setState. (Set the state as
> "nospace")
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/storage.
> js#L282

We should add two simple tests to this file:

https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/test/unit/lib/storage_test.js

One test should invoke onStorageChange() with a reason of 'available', 'shared', 'unavailable' or 'nospace'. It should then check and assert that setState() gets called with the reason.

The other test should invoke onStorageChange() with a reason of 'foo', 'bar' or 'baz'. It should then check and assert that setState() DOESN'T get called.

You can see from the other tests in the above file how to make those assertions. If you have any more questions, you can ping me on IRC. Thanks!
Flags: needinfo?(jdarcangelo)
Assignee: nobody → alchen
Attachment #8577039 - Attachment is obsolete: true
Comment on attachment 8579861 [details] [review]
[gaia] alphan102:bug1141958_0319 > mozilla-b2g:master

Hi Justin,
I have added the unit test in this patch.
Need your review.
Thanks.
Attachment #8579861 - Flags: review?(jdarcangelo)
Comment on attachment 8579861 [details] [review]
[gaia] alphan102:bug1141958_0319 > mozilla-b2g:master

LGTM. Thanks for adding the tests!
Attachment #8579861 - Flags: review?(jdarcangelo) → review+
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.