Closed Bug 1115496 Opened 9 years ago Closed 9 years ago

[DeviceStorage] Provide event from outdated DeviceStorage instance object to notify Gaia inconsistently.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
2.2 S9 (3apr)
Tracking Status
firefox40 --- fixed

People

(Reporter: iliu, Assigned: alchen)

References

Details

Attachments

(2 files, 2 obsolete files)

Gaia cannot keep the reference of DeviceStorage object after the default storage location changed.
The issue is come from bug 1110068, bug 1115492. We can reference the use case from the two bugs. I file the bug for keeping discussion for improving DeviceStorage API.
Summary:

Currently, Gaia apps have to observe settings key 'device.storage.writable.name' for reaching new DeviceStorage instance object.

navigator.mozSettings.addObserver('device.storage.writable.name', function() {
  // TODO: reaching new DeviceStorage instance object
});

Somehow, this is not reasonable - for other app who wants to access device-storage, that means they need to have the settings permission to know the device storage's state change - does not make sense anyway.
The default media location can be changed by "Settings -> media storage -> Default media location".
In this behavior, there is no gecko involved. (only change the value of settings key)
The event should be filed at the moment which user change the default media location.
The event is filed from mozSettings, not DeviceStorage object. So that I filed the issue here with previous discussion. In Gaia side, all apps who is using DeviceStorage have to request settings permission to know the device storage's instance outdated(default storage changed to internal, but the instance is still pointing to external). Otherwise, apps have to access device storage from navigator each time.
Update the latest discussion conclusion:

1.
System app cannot broadcast a event to public.
So if we want to file a event when key changed and broadcast to public, we need to file this event in gecko.

2.
Since it is a common issue in using settings key use case, we need to sync with other component for a normal solution.
Ian will help us to ask RIL for their solution.
Then we can use the same way to solve this problem.
(In reply to Alphan Chen[:Alphan] from comment #5)
> Update the latest discussion conclusion:
> 
> 2.
> Since it is a common issue in using settings key use case, we need to sync
> with other component for a normal solution.
> Ian will help us to ask RIL for their solution.
> Then we can use the same way to solve this problem.

I have asked EJ for the problem in RIL. Looks like it's different case. That problem is relative with performance of getter `navigator.mozMobileConnections.length`. It will make gecko to init object `mozMobileConnections`. We cannot reference solution here.
I would like to raise a proposal for this bug.

[How to notify gaia?]
Gecko will use "change" event for original default location(e.g : sdcard) to notify gaia.
Gaia can listen this event or use devicestorage.onchange to do corresponding thing when the event is fired.

[The detail of this change event:]
reason:  default-location-changed 
path:    sdcard1 (the storage name of the new default location)
Hi Dave,
this is the gecko implementation based on Comment 7.
Need your feedback.
Thanks.
Attachment #8565311 - Flags: feedback?(dhylands)
Hi Ian and Fred,
here is the gaia example.
Could you take a look and give some feedbacks for this proposal?
Thanks.
Attachment #8565314 - Flags: feedback?(iliu)
Attachment #8565314 - Flags: feedback?(gasolin)
Comment on attachment 8565311 [details] [diff] [review]
(0217)Use change event to notify gaia that this storage is not default location anymore

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

Sorry about the late rely. I was on PTO last week and forgot to update my bugzilla status.

My only conern with issuing a change event is that we need to find all of the existing code which uses change events and verify that adding a new event doesn't break existing behaviours. It shouldn't but we need to check.

I'd like to see some of the C strings converted into constants (like "device.storage.writable.name" and "change"). I'm probably guilty of introducing bare strings.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3364,5 @@
>      return NS_ERROR_NOT_AVAILABLE;
>    }
> +
> +  Preferences::AddStrongObserver(this, "device.storage.writable.name");
> +  mIsDefaultLocation = (mStorageName == mozilla::Preferences::GetString("device.storage.writable.name"));

nit: try to keep lines less than 80 columns.

@@ +4326,5 @@
>  }
>  
> +void
> +nsDOMDeviceStorage::DispatchDefaultChangeEvent()
> +{

I'd prefer if this used the nsDOMDeviceStorage::Default() method or perhaps nsDOMDeviceStorage::GetDefaultStorageName so that we aren't duplicating logic.

@@ +4330,5 @@
> +{
> +    nsAdoptingString DefaultLocation;
> +    DefaultLocation = mozilla::Preferences::GetString("device.storage.writable.name");
> +
> +    if (mIsDefaultLocation) {

Why only when you were the default?

I would have thought that becoming the default might also be useful.

@@ +4428,5 @@
>    }
>  
> +  if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) &&
> +               aData &&
> +               nsDependentString(aData).Equals(NS_LITERAL_STRING("device.storage.writable.name")))

We should probably make device.storage.writable.name
Attachment #8565311 - Flags: feedback?(dhylands) → feedback+
I don't understand the change. Does it mean the event name renamed from 'change' to be 'onchange'? If yes, why we should do so? We cannot reuse the original event name here? Thanks.

//this.picture.addEventListener('change', this.onStorageChange);
this.picture.onchange = this.onStorageChange;
Flags: needinfo?(alchen)
Comment on attachment 8565314 [details] [diff] [review]
(gaia example) Use change event to change the storage if the default location is changed

If I understand correctly, this patch introduce new `default-location-changed` value in DeviceStorage's change event, to replace the original settings observer.

Then, you should call `this.onStorageVolumeChanged` in that event handler so the  StorageVolume change action are still handled.


Though `this.picture.addEventListener('change'` & this.picture.onchange are equivalent, please keep the origin syntax.
Attachment #8565314 - Flags: feedback?(gasolin)
Comment on attachment 8565314 [details] [diff] [review]
(gaia example) Use change event to change the storage if the default location is changed

As Fred's comment 12, feedback+ with me.
Attachment #8565314 - Flags: feedback?(iliu) → feedback+
Flags: needinfo?(alchen)
(In reply to Dave Hylands [:dhylands] from comment #10)
> Comment on attachment 8565311 [details] [diff] [review]
> (0217)Use change event to notify gaia that this storage is not default
> location anymore
> 
> Review of attachment 8565311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry about the late rely. I was on PTO last week and forgot to update my
> bugzilla status.
> 
> My only conern with issuing a change event is that we need to find all of
> the existing code which uses change events and verify that adding a new
> event doesn't break existing behaviours. It shouldn't but we need to check.
> 
I am checking this part now.


> 
> @@ +4330,5 @@
> > +{
> > +    nsAdoptingString DefaultLocation;
> > +    DefaultLocation = mozilla::Preferences::GetString("device.storage.writable.name");
> > +
> > +    if (mIsDefaultLocation) {
> 
> Why only when you were the default?
> 
> I would have thought that becoming the default might also be useful.
> 

For default become non-default case, this device storage instance should be updated.
However, for non-default become default case, I cannot find any condition which needs to deal with it.
If we also need this case, I think we can add one more reason for it.
Maybe "default-location-changed" for "default -> non-default",
      "become-default-location" for "non-default -> default".
(In reply to Alphan Chen [:Alphan] from comment #14)
> (In reply to Dave Hylands [:dhylands] from comment #10)
> > Comment on attachment 8565311 [details] [diff] [review]
> > (0217)Use change event to notify gaia that this storage is not default
> > location anymore
> > 
> > Review of attachment 8565311 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Sorry about the late rely. I was on PTO last week and forgot to update my
> > bugzilla status.
> > 
> > My only conern with issuing a change event is that we need to find all of
> > the existing code which uses change events and verify that adding a new
> > event doesn't break existing behaviours. It shouldn't but we need to check.
> > 
> I am checking this part now.
> 

I checked all the gaia code. (search "on.change" and "addEventListener('change'" )

There is only one concern in 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".

I prefer to Rewrite the logic of change event handler. (Only handle the reason they should care.)
This is what most apps do in their change event handler.
>  switch (value) {
>    case 'deleted':
>      var filepath = this.checkFilepath(e.path);
>      this.emit('itemdeleted', { path: filepath });
>      break;
>    case 'available':
>    case 'shared':
>    case 'unavailable':
>      this.setState(value);
>  }
Hi Dave,
I would like to file a bug for raising this concern.
This bug will depend on that bug.
What do you think?
Flags: needinfo?(dhylands)
(In reply to Alphan Chen [:Alphan] from comment #16)
> Hi Dave,
> I would like to file a bug for raising this concern.
> This bug will depend on that bug.
> What do you think?

Seems reasonable to me. You should probably cc :mikeh on anything camera related.
Flags: needinfo?(dhylands)
Depends on: 1141958
Assignee: nobody → alchen
Hi Dave,
please review the latest patch.
As I said in comment 14, I add two more reasons for change event.
"default-location-changed" for DS from default to non-default
"become-default-location" for DS from non-default to default
Attachment #8565311 - Attachment is obsolete: true
Attachment #8583701 - Flags: review?(dhylands)
Comment on attachment 8583701 [details] [diff] [review]
(0326) )Use change event to notify gaia that this storage is not default location anymore

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +4347,5 @@
> +
> +  if (mIsDefaultLocation) {
> +    init.mReason.AssignLiteral("default-location-changed");
> +  } else {
> +    init.mReason.AssignLiteral("become-default-location");

nit: I would use became rather than become (become-default-location sounds like it's using become as a verb). become is present tense. became is past tense. The change has already happened (so past tense).
Attachment #8583701 - Flags: review?(dhylands) → review+
Here is try server result. It looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc285057d774
(The fail items in B2G Desktop Linux x64 debug build should be common problems which are not related to this patch)
Attachment #8583701 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f4108e07d926
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S9 (3apr)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: