[WAP push] When DUT receives SI message with delete action attribute, all SI messages with same SI-ID should be removed from utility tray.

RESOLVED FIXED

Status

Firefox OS
Gaia::Wappush
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Enpei, Assigned: gsvelto)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog, b2g-v1.3T affected, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.1 affected, b2g-v2.2 affected, b2g-master fixed)

Details

(Whiteboard: permafail)

Attachments

(5 attachments, 9 obsolete attachments)

710.80 KB, text/plain
Details
46 bytes, text/x-github-pull-request
Details | Review | Splinter Review
4.73 KB, patch
julienw
: review+
Details | Diff | Splinter Review
48.34 KB, patch
gsvelto
: review+
Details | Diff | Splinter Review
17.23 KB, patch
julienw
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 8336638 [details]
time stamp: 16:36, 11/22.

Currently on v1.2, all same SI-ID messages will be marked as expired. But the correct behavior should be all SI messages including the one with delete action attribute should be removed from utility tray.

* Build Number                
Gaia:     ce276842c9ac1746073271fb736dfdb626a89240
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/36c4c667b9f2
BuildID   20131121004002
Version   26.0

* Reproduce Steps
1. Send 2 SI messages with same SI-ID.
2. Send another SI message with same SI-ID and delete action attribute as step 1.

* Expected Result
1. Delete action message will not be displayed on utility tray.
2. 2 SI messages with same SI-ID will be removed from utility tray.

* Actual Result
1. Delete action message will be displayed on utility tray.
2. 2 SI messages with same SI-ID will be marked as expired messages.

* Occurrence rate
100%
(Reporter)

Updated

5 years ago
blocking-b2g: --- → 1.3?
Why would this block 1.3 specifically? This is reproducing on 1.2, so what makes this a blocker for 1.3 but not a blocker for 1.2?
(Assignee)

Comment 2

5 years ago
(In reply to Jason Smith [:jsmith] from comment #1)
> Why would this block 1.3 specifically? This is reproducing on 1.2, so what
> makes this a blocker for 1.3 but not a blocker for 1.2?

This functionality was not supposed to be in 1.2, I didn't know it and went ahead and implemented the whole spec though with certain UI limitations due to the lack of specific functionality in 1.2. So the fact that 1.2 handles this condition in a suboptimal way is not really a problem as it was not even supposed to handle it at all.
(Assignee)

Comment 3

5 years ago
Blocking on bug 938540 as we need the new notification API to implement this.
Depends on: 938540
Not a system bug - moving into the Gaia component, until we get a bugzilla component created for WAP Push.
Component: Gaia::System → Gaia

Comment 5

5 years ago
Joe, can you help to triage? Thanks.
Flags: needinfo?(jcheng)
triage: not blocking. this seem like a test case instead of a an useful use case.
blocking-b2g: 1.3? → ---
Flags: needinfo?(jcheng)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 961924

Comment 8

5 years ago
Sorry to urging you, but it should be fixed on v1.3.
blocking-b2g: --- → 1.3?
(In reply to buri.blff from comment #8)
> Sorry to urging you, but it should be fixed on v1.3.

Why? We need a rationale why this would be a blocker.
(Assignee)

Comment 10

5 years ago
Making this a 1.3 blocker is problematic as it depends on the new notification API. This will require us to uplift at least 4 different bugs AFAIK and it seems too late in the cycle and too risky for such a big change as we might break other applications too.

Also I'd like to point out that the 'delete' action itself is implemented, the only thing that is not is removing the notification. When a user taps on the notification whose message has been deleted he will be shown a screen telling that the message has expired. This is obviously not optimal but creates a consistent behavior.
Not blocking on 1.3 as we've seen the issue in too many releases.

Also refer to comment 10.
blocking-b2g: 1.3? → backlog
(Assignee)

Updated

5 years ago
Component: Gaia → Gaia::Wappush
Hardware: x86_64 → ARM

Updated

4 years ago
Whiteboard: burirun1.3-2

Comment 12

4 years ago
Dears,

Nominate as 1.3? because it may becomes a blocker of IOT. 

Thanks.
blocking-b2g: backlog → 1.3?
(Assignee)

Comment 13

4 years ago
As I mentioned in comment 10 fixing this bug requires the new notifications API that we've decided not to uplift due to risk: the changes it introduces are significant and would affect all applications that use notifications. Within the 1.3 timeframe and with the APIs we have available there fixing this bug is impossible. What we do instead is marking deleted messages as expired so when the user taps on a notification it will be informed that the message has been deleted.
Jack

The risk here seems to rather high and might end up bad experience either way.
Flags: needinfo?(liuyongming)
Discussed via Vance - there's agreement now this isn't a blocker.
blocking-b2g: 1.3? → backlog

Comment 16

4 years ago
Dears, 

Because Wap push message is new feature introduced in v1.3, so there are also risks of being considered as blocker by carriers or certification organization, I'll try to get feedback from them.
Flags: needinfo?(liuyongming)
status-b2g-v1.4: --- → affected
Whiteboard: burirun1.3-2 → burirun1.3-2, burirun1.4-2
status-b2g-v1.3T: --- → affected
Whiteboard: burirun1.3-2, burirun1.4-2 → permafail
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
Flags: needinfo?(dharris)
QA Whiteboard: [QAnalyst-Triage?]
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(dharris)

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
(Assignee)

Updated

4 years ago
See Also: → bug 1067731

Comment 17

4 years ago
Gabriele

I want to know how does this issue going on as as the new notification API you mentioned in comment3 is already implemented.

Thank you.
Flags: needinfo?(gsvelto)
(Assignee)

Comment 18

4 years ago
(In reply to jinghua.xing from comment #17)
> I want to know how does this issue going on as as the new notification API
> you mentioned in comment3 is already implemented.

Unfortunately I didn't have time to work on this yet. If you need this change soon please ask Mozilla's release management to triage and re-prioritize this bug.
Flags: needinfo?(gsvelto)

Comment 19

4 years ago
Hi,vance

This issue is a block issue for us and it remains for a long time. As the issue can also reproduced in v2.1 and the block issue mentioned in comment3 is already fixed, can you help us push this issue? 

Thank you.
Flags: needinfo?(vchen)

Comment 20

4 years ago
Created attachment 8538345 [details] [diff] [review]
remove_message_on_utility_tray.patch

Julien:
I tried to make a solution for this issue and made a patch for it. Can you help me review it? 
Thank you.
Attachment #8538345 - Flags: review?(felash)
Comment on attachment 8538345 [details] [diff] [review]
remove_message_on_utility_tray.patch

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

I think you need to handle this from wpm_onWapPushReceived in WapPushManager, otherwise we have a cyclic dependency, and this will make the application less maintainable.

Also you'll need to add unit tests for this code. You can find some information about unit tests in Gaia in [1].

[1] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests
Attachment #8538345 - Flags: review?(felash)

Comment 22

4 years ago
How did I use the code below directly in deleteById() function in messagedb.js? In this way we do not have cyclic dependency and we needn't to pass the timestamps of message with same id to WapPushManager.

--- a/apps/wappush/js/messagedb.js
+++ b/apps/wappush/js/messagedb.js
@@ -204,6 +204,17 @@ var MessageDB = (function() {
       var cursor = event.target.result;
 
       if (cursor) {
+        var timeTag = cursor.value.timestamp;
+        Notification.get({tag: timeTag}).then(
+          function onSuccess(notifications) {
+            for (var i = 0; i < notifications.length; i++) {
+              notifications[i].close();
+            }
+          },
+          function onError(error) {
+            console.error('Notification.get() promise error: ' + error.name);
+          }
+        );
         cursor.delete();
         cursor.continue();
       }
Flags: needinfo?(felash)
No sorry, MessageDB, as the name suggests, should only handle the database operations.

As I said in comment 21, I think it's better to handle the "frontend" part in wpm_onWapPushReceived in WapPushManager. Please tell me why you don't like this suggestion?
Flags: needinfo?(felash)
(Assignee)

Comment 24

4 years ago
Created attachment 8542117 [details] [diff] [review]
[PATCH] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

Stealing this bug since I know how to fix this properly, I just never had the time to.

This patch introduces the core of a complete MVC approach to this app so that we can listen for changes in the model and uses it to remove notifications when messages are deleted.

In practice this is implemented in the following way:

- The message database object is augmented with methods to add and remove listeners, DOM event listener-style

- The said methods can be used to listen for a `messagedeleted' event which is triggered every time we delete a message; the event is passed the message object as its sole parameter

- We hook up the rest of the app to use this to clear notifications whenever an event is deleted

Note that this allowed me to remove the CP message specific code that handled clearing notifications which is nice because the wpm_clearNotifications function is now private again.
Assignee: nobody → gsvelto
Attachment #8538345 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8542117 - Flags: review?(felash)
(Assignee)

Comment 25

4 years ago
Created attachment 8542118 [details] [review]
[PULL REQUEST] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages
clear my ni since Gabriele is helping to move this bug forward
Flags: needinfo?(vchen)
Comment on attachment 8542117 [details] [diff] [review]
[PATCH] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

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

nit: you need a rebase :)

Something is weird with your event handlers that you tried to do as DOMmy as possible.

In DOM:

* we call addEventListener on a DOM node
* we call dispatchEvent on a DOM node (the same one or a child)
* the argument is the event object, the target is automatically the DOM node we call dispatchEvent on.

In your implementation:

* we call addEventListener on MessageDB
* but you call dispatchEvent on MessageDB, with the message argument as target. This is quite weird to have a different target than the thing we called addEventListener on.

I understand you want an easy way to provide a payload, but I think this is confusing.

Instead I suggest you take SMS' EventDispatcher from [1] (copying to /shared/js first, and possibly filing a bug to use the shared version in SMS, unless you want to do it in this patch). At least we know it works well, it has shorter method names, is well-tested, and it's not confusing because it does not try to copy the DOM's event system.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/event_dispatcher.js

Tell me if you disagree, and I can revisit :)

::: apps/wappush/test/unit/messagedb_test.js
@@ +331,5 @@
> +          type: 'messagedeleted',
> +          target: messages.current
> +        });
> +      }).then(done, done);
> +    });

I don't get the difference between this and the other test change ?

::: apps/wappush/test/unit/wappush_test.js
@@ +752,5 @@
> +        return WapPushManager.onWapPushReceived(messages.delete);
> +      }).then(function() {
> +        sinon.assert.calledOnce(Notification);
> +        sinon.assert.calledWith(MockNotification.get, { tag: 0 });
> +        sinon.assert.calledOnce(MockNotification.prototype.close);

what you really want to know is whether close was called on the notification returned by MockNotification.get. So you should so something like:

  // before actions
  var fakeNotif = new MockNotification();
  MockNotification.get.withArgs({tag : 0}).returns(Promise.resolve([fakeNotif]));

  // assertions time
  sinon.assert.calledOn(MockNotification.prototype.close, fakeNotif);

(I didn't test, but you get the idea)
Attachment #8542117 - Flags: review?(felash)
(Assignee)

Comment 28

4 years ago
(In reply to Julien Wajsberg [:julienw] (PTO until 1/5) from comment #27)
> Something is weird with your event handlers that you tried to do as DOMmy as
> possible.
>
> [...]
>
> I understand you want an easy way to provide a payload, but I think this is
> confusing.

Yeah, that's a good point.

> Instead I suggest you take SMS' EventDispatcher from [1] (copying to
> /shared/js first, and possibly filing a bug to use the shared version in
> SMS, unless you want to do it in this patch). At least we know it works
> well, it has shorter method names, is well-tested, and it's not confusing
> because it does not try to copy the DOM's event system.

That sounds like an excellent idea. And even more so considering we might have to merge WAP Push into SMS in a (distant?) future. I'll pick up this idea and address the tests too as you've suggested.

Comment 29

3 years ago
Gabriele:

I have found another problem with the si message. 

In WAP-167 6.2, it says for the deletion of the message:
>An SI with the action attribute set to “delete” MUST have an explicitly assigned value for si-id.

But in our design, according to WAP-167 5.2.1:
>If the 'si-id' attribute is not specified, its value is considered to be the same as the value of the 
>'href' attribute

So if the delete message has no explicitly si-id, we will use its href as its si-id, and it will delete all the si messages with same href and no si-id which we received before.

Do you think it is a problem? Need we open a new bug to follow this?
Thank you.
Flags: needinfo?(gsvelto)
(Assignee)

Comment 30

3 years ago
(In reply to jinghua.xing from comment #29)
> Do you think it is a problem? Need we open a new bug to follow this?
> Thank you.

I don't think so, the wording of the spec is very clear, paragraph 5.2.1 states exactly that:

"If this attribute is not specified, its value is considered to be the same as the value of the href attribute."

So I think it's safe to assume - before doing any processing of the message - that if it doesn't have a 'si-id' field, but it does have an 'href' field then 'si-id' == 'href'.
Flags: needinfo?(gsvelto)
Gabriele, the wording for "delete" is clear too: "MUST have an _explicitly_ assigned value". To me, if you infer 'si-id' from 'href', it's not explicit anymore.
(Assignee)

Comment 32

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #31)
> Gabriele, the wording for "delete" is clear too: "MUST have an _explicitly_
> assigned value". To me, if you infer 'si-id' from 'href', it's not explicit
> anymore.

I have just checked my version of the spec and it didn't have that wording but I now noticed that it's not the latest version and the following version does have this requirement. I will fold that fix into this patch then.
(Assignee)

Comment 33

3 years ago
Created attachment 8557332 [details] [diff] [review]
[PATCH 1/3] Move the EventDispatcher sources into shared

I finally found the time to work on this, here's the patch-set.

First of all I've moved the EventDispatcher sources into the shared directory to be able to use them. I absolutely love this stuff, I wish I had it in the past.
Attachment #8557332 - Flags: review?(felash)
(Assignee)

Comment 34

3 years ago
Created attachment 8557333 [details] [diff] [review]
[PATCH 2/3 v2] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

This is the actual changes using the EventDispatcher instead of my clumsy event listener thingamajig.
Attachment #8542117 - Attachment is obsolete: true
Attachment #8557333 - Flags: review?(felash)
(Assignee)

Comment 35

3 years ago
Created attachment 8557336 [details] [diff] [review]
[PATCH 3/3] Discard SI messages with a delete action but no explicit si-id field

And finally this fixes the issue with `action="delete"' messages that didn't have an explicit `si-id' field. Note how I was already discarding those messages, but I was doing it in the wrong place: after having used the `href' field to populate the `si-id' one instead of before :-/

The PR has been updated with all the patches pushed on top (and rebased on the current master).
Attachment #8557336 - Flags: review?(felash)
Comment on attachment 8557332 [details] [diff] [review]
[PATCH 1/3] Move the EventDispatcher sources into shared

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

I think you forgot to move the test event_dispatcher_test.js? Or maybe you forgot to add it to your patch?
(Assignee)

Comment 37

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #36)
> I think you forgot to move the test event_dispatcher_test.js? Or maybe you
> forgot to add it to your patch?

I forgot to move it into sharedtest :-(
(Assignee)

Comment 38

3 years ago
Created attachment 8557612 [details] [diff] [review]
[PATCH 1/3 v2] Move the EventDispatcher sources into shared
Attachment #8557332 - Attachment is obsolete: true
Attachment #8557332 - Flags: review?(felash)
Attachment #8557612 - Flags: review?(felash)
Comment on attachment 8557612 [details] [diff] [review]
[PATCH 1/3 v2] Move the EventDispatcher sources into shared

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

You forgot to change the files:

  apps/sms/test/unit/inter_instance_event_dispatcher_test.js
  apps/sms/test/unit/message_manager_test.js

r=me with these changes. Of course we'll need a green build before merging.
Attachment #8557612 - Flags: review?(felash) → review+
(In reply to Julien Wajsberg [:julienw] from comment #39)

> r=me with these changes. Of course we'll need a green build before merging.

We'll also need some comments at the start of the file to explain how it works, if you're willing to do it.
Comment on attachment 8557333 [details] [diff] [review]
[PATCH 2/3 v2] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

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

r=me with one issue and some comments.

I see cp_screen_helper.js has no unit test at all, this is quite bad :/ no mystery the missing clearNotifications was not caught :(
I see the file is required in wappush_test.js though, but is it tested at all?

::: apps/wappush/js/cp_screen_helper.js
@@ -397,5 @@
>     */
>    function cpsh_onQuit(evt) {
>      evt.preventDefault();
>  
> -    WapPushManager.clearNotifications(messageTag);

It's used once more in this file, I think you'll want to remove the call.

::: apps/wappush/js/messagedb.js
@@ +307,5 @@
> +  function mdb_dispatchEvent(type, target) {
> +    /* We explicitly use the MessageDB singleton as the EventDispatcher code
> +     * requires the `this' reference to operate. */
> +    MessageDB.emit(type, target);
> +  }

Just wondering whether mdb_dispatchEvent is useful at all? Why not calling MessageDB.emit directly?

::: apps/wappush/test/unit/messagedb_test.js
@@ +242,5 @@
>      });
>  
> +    test('delete action messages dispatch a `messagedeleted\' event',
> +    function(done) {
> +      var onMessageDeletedStub = this.sinon.stub();

nit: you don't need "this" if you create a new function. Because you don't need to restore it :)

@@ +252,5 @@
> +        MessageDB.off('messagedeleted', onMessageDeletedStub);
> +        sinon.assert.calledOnce(onMessageDeletedStub);
> +        sinon.assert.calledWith(onMessageDeletedStub, messages.current);
> +      }).then(done, done);
> +    });

It's not mandatory, but just to be sure for the future ("belt and suspenders") you could add "MessageDB.offAll()" in a teardown function.
Attachment #8557333 - Flags: review?(felash) → review+
(Assignee)

Comment 42

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #39)
> You forgot to change the files:
> 
>   apps/sms/test/unit/inter_instance_event_dispatcher_test.js
>   apps/sms/test/unit/message_manager_test.js
> 
> r=me with these changes. Of course we'll need a green build before merging.

Meh, I'm getting sloppy :(

(In reply to Julien Wajsberg [:julienw] from comment #40)
> We'll also need some comments at the start of the file to explain how it
> works, if you're willing to do it.

Yup, I'm very willing to. There's quite a bit of other code in Gaia that use DOMEvent-look-alikes and other systems like that and it's all duplicated from what I could tell. They could all use some shared code that works well.
(Assignee)

Comment 43

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #41)
> I see cp_screen_helper.js has no unit test at all, this is quite bad :/ no
> mystery the missing clearNotifications was not caught :(
> I see the file is required in wappush_test.js though, but is it tested at
> all?

Yes, there's a full suite here:

https://github.com/mozilla-b2g/gaia/blob/1509b2cedfd7d19c20a48a1901296e529470645d/apps/wappush/test/unit/wappush_test.js

However like other things in WAP Push those are poor man's integration tests that try to cover most  of the flows. CP really needs some proper integration tests but since I'm pretty much the only one working on it lately I really didn't have the time to write some.

> It's used once more in this file, I think you'll want to remove the call.

Thanks, I had missed it.

> Just wondering whether mdb_dispatchEvent is useful at all? Why not calling
> MessageDB.emit directly?

Not really, but I wanted to leave a comment as to why I'm using the MessageDB variable explicitly and the function gives me a convenient place to do so.

> nit: you don't need "this" if you create a new function. Because you don't
> need to restore it :)

Right, thanks for the tip, I've used it out of habit.

> It's not mandatory, but just to be sure for the future ("belt and
> suspenders") you could add "MessageDB.offAll()" in a teardown function.

Good point, I'll do.
(In reply to Gabriele Svelto [:gsvelto] from comment #42)

> Yup, I'm very willing to. There's quite a bit of other code in Gaia that use
> DOMEvent-look-alikes and other systems like that and it's all duplicated
> from what I could tell. They could all use some shared code that works well.

I can send a message to dev-gaia after we land :)
(In reply to Julien Wajsberg [:julienw] from comment #44)
> (In reply to Gabriele Svelto [:gsvelto] from comment #42)
> 
> > Yup, I'm very willing to. There's quite a bit of other code in Gaia that use
> > DOMEvent-look-alikes and other systems like that and it's all duplicated
> > from what I could tell. They could all use some shared code that works well.
> 
> I can send a message to dev-gaia after we land :)

BTW do you think we should ask a final review from Kevin for this?
(Assignee)

Comment 46

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #45)
> BTW do you think we should ask a final review from Kevin for this?

Yeah, I was also thinking of filing some bugs for replacing existing code like I did for the JSON loader helper.
(Assignee)

Comment 47

3 years ago
Created attachment 8559718 [details] [diff] [review]
[PATCH 1/3 v3] Move the EventDispatcher sources into shared

Same patch as before but with a detailed description added so that hopefully more people can make use of it. I'm adding Kevin to the review as discussed before. A quick check in shared turns out these files already implementing their own events (in various subtly different ways):

shared/js/mediadb.js
shared/js/collections_database.js
shared/js/homescreens/vertical_preferences.js
shared/js/fxa_iac_client.js
shared/js/bookmarks_database.js

More under apps:

apps/settings/js/modules/apps_cache.js
apps/settings/js/modules/settings_cache.js
apps/video/js/video.js
apps/calendar/js/responder.js
apps/camera/bower_components/device-orientation/index.js
apps/callscreen/js/audio_competing_helper.js

I've turned these out with a simple grep but I'm convinced there might be more. If we agree to put on this in shared/ I'll look through all these and file a separate bug for each app/component to use our shared code where possible. I did the same with the JSON loader and these kind of simple changes not only help with removing duplicate code but they also make good mentored, first bugs for new contributors.
Attachment #8557612 - Attachment is obsolete: true
Attachment #8559718 - Flags: review?(kgrandon)
Attachment #8559718 - Flags: review?(felash)
Comment on attachment 8559718 [details] [diff] [review]
[PATCH 1/3 v3] Move the EventDispatcher sources into shared

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

Seems like the reivew request here is just for moving of the event_dispatcher into shared/? This sounds like a good idea to me, especially since a few apps are using this type of functionality.

Might be a good idea to email dev-gaia about this change and get people looking at porting other implementations to use the shared library. Nice work!

::: shared/js/event_dispatcher.js
@@ +14,5 @@
> + * present in the list will be allowed. Using events not present in the list
> + * will cause other functions to throw an error:
> + *
> + * var obj = EventDispatcher.mixin(new SomeObj(), [
> + *   "somethinghappened",

nit: gaia standards say that we should use single quotes, shouldn't the examples use single quotes as well?

@@ +18,5 @@
> + *   "somethinghappened",
> + *   "somethingelsehappened"
> + * ]);
> + *
> + * The wrapped object will have four new methods: `on', `off', `offAll'

nit: these quotes seem mismatched and it's distracting to read.
Attachment #8559718 - Flags: review?(kgrandon) → review+
(Assignee)

Comment 49

3 years ago
(In reply to Kevin Grandon :kgrandon from comment #48)
> Seems like the reivew request here is just for moving of the
> event_dispatcher into shared/? This sounds like a good idea to me,
> especially since a few apps are using this type of functionality.
> 
> Might be a good idea to email dev-gaia about this change and get people
> looking at porting other implementations to use the shared library. Nice
> work!

Yes, we were planning on pushing this to dev-gaia.

> nit: gaia standards say that we should use single quotes, shouldn't the
> examples use single quotes as well?

Good point. It's a habit I have from writing C code.

> nit: these quotes seem mismatched and it's distracting to read.

Another habit I've inherited by following GNU coding guidelines :) I'll update the PR with both fixed.
Comment on attachment 8559718 [details] [diff] [review]
[PATCH 1/3 v3] Move the EventDispatcher sources into shared

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

r=me with some documentation nits and a green try of course

::: apps/sms/test/unit/compose_test.js
@@ -35,3 @@
>  require('/test/unit/mock_smil.js');
> -require('/shared/test/unit/mocks/mock_async_storage.js');
> -require('/shared/test/unit/mocks/mock_l10n.js');

thanks :)

::: shared/js/event_dispatcher.js
@@ +16,5 @@
> + *
> + * var obj = EventDispatcher.mixin(new SomeObj(), [
> + *   "somethinghappened",
> + *   "somethingelsehappened"
> + * ]);

nit: maybe add a line that then it's recommended to pass the allowed events.

@@ +37,5 @@
> + *
> + * And use `offAll' to remove all registered event listeners for the specified
> + * event:
> + *
> + * obj.offAll("somethinghappened");

nit: add that you can use offAll without a parameter to remove all listeners -- useful for unit tests.
Attachment #8559718 - Flags: review?(felash) → review+
Comment on attachment 8557333 [details] [diff] [review]
[PATCH 2/3 v2] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

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

::: apps/wappush/js/messagedb.js
@@ +1,4 @@
>  /* -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- /
>  /* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
>  
> +/* global EventDispatcher, IDBKeyRange, Promise */

Maybe I'm blind but I don't see where you include event_dispatcher.js? Do you miss a change in index.html?
Comment on attachment 8557336 [details] [diff] [review]
[PATCH 3/3] Discard SI messages with a delete action but no explicit si-id field

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

See below

::: apps/wappush/js/parsed_message.js
@@ +180,5 @@
> +      if (!obj.id && obj.href) {
> +        /* WAP-167 5.2.1: If the 'si-id' attribute is not specified, its value
> +         * is considered to be the same as the value of the 'href' attribute */
> +        obj.id = obj.href;
> +      }

tell me what you think, but I'd move the 2 last conditions just after setting obj.id earlier.

Maybe something like this:

  if (indicationNode.hasAttribute('si-id')) {
    obj.id = indicationNode.getAttribute('si-id');
  } else if (obj.action === 'delete') {
    return null;
  } else if (obj.href) {
    obj.id = obj.href;
  }

The rationale is that all "set" operations to obj.id are located in the same code area and we can follow the logic more easily.
Attachment #8557336 - Flags: review?(felash)
(Assignee)

Comment 53

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #51)
> Maybe I'm blind but I don't see where you include event_dispatcher.js? Do
> you miss a change in index.html?

Yup, I forgot to add it there.

(In reply to Julien Wajsberg [:julienw] from comment #52)
> The rationale is that all "set" operations to obj.id are located in the same
> code area and we can follow the logic more easily.

Yes, it makes the code more streamlined. I'll do it and put the 'si-id' processing below the 'action' part so that I already have the action property set if the node was present. Updated patches coming.
(Assignee)

Comment 54

3 years ago
Created attachment 8562172 [details] [diff] [review]
[PATCH 1/3 v4] Move the EventDispatcher sources into shared

Updated patch with nits addressed, carrying over the review flags.
Attachment #8559718 - Attachment is obsolete: true
Attachment #8562172 - Flags: review+
(Assignee)

Comment 55

3 years ago
Created attachment 8562173 [details] [diff] [review]
[PATCH 2/3 v3] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

Updated patch with nits addressed, carrying over the r+ flag.
Attachment #8557333 - Attachment is obsolete: true
Attachment #8562173 - Flags: review+
(Assignee)

Comment 56

3 years ago
Created attachment 8562176 [details] [diff] [review]
[PATCH 3/3 v2] Discard SI messages with a delete action but no explicit si-id field

Updated patch.
Attachment #8557336 - Attachment is obsolete: true
Attachment #8562176 - Flags: review?(felash)
Comment on attachment 8562172 [details] [diff] [review]
[PATCH 1/3 v4] Move the EventDispatcher sources into shared

There is a slight rebase conflicts to handle now in apps/sms/test/unit/startup_test.js.
Comment on attachment 8562173 [details] [diff] [review]
[PATCH 2/3 v3] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

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

::: apps/wappush/test/unit/messagedb_test.js
@@ +246,5 @@
>      });
>  
> +    test('delete action messages dispatch a `messagedeleted\' event',
> +    function(done) {
> +      var onMessageDeletedStub = this.sinon.stub();

nit: you still don't need "this" here ;)

"this" is useful when you need to restore; here you don't need to restore anything as you're creating a function out of nowhere.

@@ +318,5 @@
>        }, done);
>      });
> +
> +    test('a `messagedeleted\' event is dispatched', function(done) {
> +      var onMessageDeletedStub = this.sinon.stub();

ditto
(Assignee)

Comment 59

3 years ago
Created attachment 8569216 [details] [diff] [review]
[PATCH 1/3 v5] Move the EventDispatcher sources into shared

Rebased patch, carrying over the r+.
Attachment #8562172 - Attachment is obsolete: true
Attachment #8569216 - Flags: review+
(Assignee)

Comment 60

3 years ago
Created attachment 8569217 [details] [diff] [review]
[PATCH 2/3 v4] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

Addressed the nits in comment 58, I hope we're good this time :)
Attachment #8562173 - Attachment is obsolete: true
Attachment #8569217 - Flags: review?(felash)
Comment on attachment 8569217 [details] [diff] [review]
[PATCH 2/3 v4] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

r=me, just take care it's still green before merging
Attachment #8569217 - Flags: review?(felash) → review+
Comment on attachment 8562176 [details] [diff] [review]
[PATCH 3/3 v2] Discard SI messages with a delete action but no explicit si-id field

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

r=me with 1 nit and 1 question

::: apps/wappush/js/parsed_message.js
@@ +168,5 @@
>  
> +      // 'si-id' attribute, optional, string
> +      if (indicationNode.hasAttribute('si-id')) {
> +        obj.id = indicationNode.getAttribute('si-id');
> +      } else if (obj.action && (obj.action === 'delete')) {

I'm just wondering why you check if obj.action is truthy... if it equals delete it's necessary truthy, right ? Therefore I think the first part of the condition is useless.

@@ +175,2 @@
>          return null;
> +      } else if (obj.href) {

wondering if it can be an empty string? what should we do when obj.id is an empty string in the end?
Attachment #8562176 - Flags: review?(felash) → review+
(Assignee)

Comment 63

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #62)
> I'm just wondering why you check if obj.action is truthy... if it equals
> delete it's necessary truthy, right ? Therefore I think the first part of
> the condition is useless.

What if it's undefined? Doesn't if (obj.action === 'delete') raise an error if the action field is undefined?

> wondering if it can be an empty string? what should we do when obj.id is an
> empty string in the end?

Let me check the spec (I'd be inclined to consider an empty string as if the id field wasn't there but you never know with these specs...).
(In reply to Gabriele Svelto [:gsvelto] from comment #63)
> (In reply to Julien Wajsberg [:julienw] from comment #62)
> > I'm just wondering why you check if obj.action is truthy... if it equals
> > delete it's necessary truthy, right ? Therefore I think the first part of
> > the condition is useless.
> 
> What if it's undefined? Doesn't if (obj.action === 'delete') raise an error
> if the action field is undefined?

nope :)
Only if 'obj' is undefined or null. But it would fail earlier in that case ;)

(obj.action === "delete") is false if obj.action is undefined.

> 
> > wondering if it can be an empty string? what should we do when obj.id is an
> > empty string in the end?
> 
> Let me check the spec (I'd be inclined to consider an empty string as if the
> id field wasn't there but you never know with these specs...).
(Assignee)

Comment 65

3 years ago
The spec doesn't say anything about the contents of the si-id field or if an empty string should be special-cased. Contrary to what I said before since the spec is basically an XML document and it's flagging the entire field as optional then I'd say the setting it to an empty string really means that's the value we should use because the field it's present. The relevant code in Gecko doesn't seem to care and will pass along such a field. What do you think?
I'd say, let's ask someone who might know better ;)

Otherwise I've mixed feelings. I'd say: land this and file a separate bug to find out :)
Flags: needinfo?(Jinghua.Xing)
(Assignee)

Comment 67

3 years ago
Addressed the final nit and try is now green: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=9b13a4c2d2cc

Landed on gaia/master 8838b6b73c52e318fa64c94491d0424e964eaeca

https://github.com/mozilla-b2g/gaia/commit/8838b6b73c52e318fa64c94491d0424e964eaeca
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1137263

Comment 68

3 years ago
Sorry for the delay. I think if si-id is an empty string, it is also be defined, so it should be explicitly assigned. What do you think?
Flags: needinfo?(Jinghua.Xing)
(Assignee)

Comment 69

3 years ago
(In reply to jinghua.xing from comment #68)
> Sorry for the delay. I think if si-id is an empty string, it is also be
> defined, so it should be explicitly assigned. What do you think?

OK, let's open a follow up then.
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.