Closed
Bug 1141958
Opened 9 years ago
Closed 9 years ago
[B2G][Camera] Refine the change event handler in storage.js
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect)
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
Assignee | ||
Comment 1•9 years ago
|
||
Hi Wilson, could you give some feedbacks about this change?
Attachment #8575877 -
Flags: feedback?(wilsonpage)
Assignee | ||
Updated•9 years ago
|
Attachment #8575877 -
Flags: feedback?(wilsonpage) → feedback?(wilsonpage)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
Comment on attachment 8575877 [details]
Refine the change event handler to prevent some unexpected reasons
LGTM
Attachment #8575877 -
Flags: feedback?(jdarcangelo) → feedback+
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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-
Updated•9 years ago
|
Attachment #8575877 -
Flags: feedback?(wilsonpage)
Assignee | ||
Comment 8•9 years ago
|
||
(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)
Comment 9•9 years ago
|
||
(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 | ||
Updated•9 years ago
|
Assignee: nobody → alchen
Comment 10•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8577039 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/c0ac6c7a784da15b30809b9108975c8c7345288d
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•