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

Status: RESOLVED FIXED

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

Description

[Enpei]

Attachment: 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%

Comment 1

[Jason Smith [:jsmith]]

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?

Comment 2

[Gabriele Svelto [:gsvelto]]

(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.

Comment 3

[Gabriele Svelto [:gsvelto]]

Blocking on bug 938540 as we need the new notification API to implement this.

Comment 4

[Jason Smith [:jsmith]]

Not a system bug - moving into the Gaia component, until we get a bugzilla component created for WAP Push.

Comment 5

[Kai-Chih Hu [:khu]]

Joe, can you help to triage? Thanks.

Comment 6

[Joe Cheng [:jcheng] (please needinfo)]

triage: not blocking. this seem like a test case instead of a an useful use case.

Comment 8

[buri.blff]

Sorry to urging you, but it should be fixed on v1.3.

Comment 9

[Jason Smith [:jsmith]]

(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.

Comment 10

[Gabriele Svelto [:gsvelto]]

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.

Comment 11

[Preeti Raghunath(:Preeti)]

Not blocking on 1.3 as we've seen the issue in too many releases.

Also refer to comment 10.

Comment 12

[Jack Liu]

Dears,

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

Thanks.

Comment 13

[Gabriele Svelto [:gsvelto]]

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.

Comment 14

[Preeti Raghunath(:Preeti)]

Jack

The risk here seems to rather high and might end up bad experience either way.

Comment 15

[Jason Smith [:jsmith]]

Discussed via Vance - there's agreement now this isn't a blocker.

Comment 16

[Jack Liu]

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.

Comment 17

[jinghua.xing]

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.

Comment 18

[Gabriele Svelto [:gsvelto]]

(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.

Comment 19

[jinghua.xing]

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.

Comment 20

[jinghua.xing]

Attachment: 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.

Comment 21

[Julien Wajsberg [:julienw]]

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

Comment 22

[jinghua.xing]

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();
       }

Comment 23

[Julien Wajsberg [:julienw]]

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?

Comment 24

[Gabriele Svelto [:gsvelto]]

Attachment: [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.

Comment 25

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Introduce an event triggered when messages are deleted from the database and use it to clear notifications for said messages

Comment 26

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

clear my ni since Gabriele is helping to move this bug forward

Comment 27

[Julien Wajsberg [:julienw]]

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)

Comment 28

[Gabriele Svelto [:gsvelto]]

(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

[jinghua.xing]

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.

Comment 30

[Gabriele Svelto [:gsvelto]]

(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'.

Comment 31

[Julien Wajsberg [:julienw]]

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.

Comment 32

[Gabriele Svelto [:gsvelto]]

(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.

Comment 33

[Gabriele Svelto [:gsvelto]]

Attachment: [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.

Comment 34

[Gabriele Svelto [:gsvelto]]

Attachment: [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.

Comment 35

[Gabriele Svelto [:gsvelto]]

Attachment: [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).

Comment 36

[Julien Wajsberg [:julienw]]

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?

Comment 37

[Gabriele Svelto [:gsvelto]]

(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 :-(

Comment 38

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 1/3 v2] Move the EventDispatcher sources into shared

Comment 39

[Julien Wajsberg [:julienw]]

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.

Comment 40

[Julien Wajsberg [:julienw]]

(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 41

[Julien Wajsberg [:julienw]]

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.

Comment 42

[Gabriele Svelto [:gsvelto]]

(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.

Comment 43

[Gabriele Svelto [:gsvelto]]

(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.

Comment 44

[Julien Wajsberg [:julienw]]

(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 :)

Comment 45

[Julien Wajsberg [:julienw]]

(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?

Comment 46

[Gabriele Svelto [:gsvelto]]

(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.

Comment 47

[Gabriele Svelto [:gsvelto]]

Attachment: [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.

Comment 48

[Kevin Grandon :kgrandon]

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.

Comment 49

[Gabriele Svelto [:gsvelto]]

(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 50

[Julien Wajsberg [:julienw]]

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.

Comment 51

[Julien Wajsberg [:julienw]]

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 52

[Julien Wajsberg [:julienw]]

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.

Comment 53

[Gabriele Svelto [:gsvelto]]

(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.

Comment 54

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 1/3 v4] Move the EventDispatcher sources into shared

Updated patch with nits addressed, carrying over the review flags.

Comment 55

[Gabriele Svelto [:gsvelto]]

Attachment: [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.

Comment 56

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 3/3 v2] Discard SI messages with a delete action but no explicit si-id field

Updated patch.

Comment 57

[Julien Wajsberg [:julienw]]

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 58

[Julien Wajsberg [:julienw]]

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

Comment 59

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 1/3 v5] Move the EventDispatcher sources into shared

Rebased patch, carrying over the r+.

Comment 60

[Gabriele Svelto [:gsvelto]]

Attachment: [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 :)

Comment 61

[Julien Wajsberg [:julienw]]

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

Comment 62

[Julien Wajsberg [:julienw]]

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?

Comment 63

[Gabriele Svelto [:gsvelto]]

(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...).

Comment 64

[Julien Wajsberg [:julienw]]

(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...).

Comment 65

[Gabriele Svelto [:gsvelto]]

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?

Comment 66

[Julien Wajsberg [:julienw]]

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 :)

Comment 67

[Gabriele Svelto [:gsvelto]]

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

Comment 68

[jinghua.xing]

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?

Comment 69

[Gabriele Svelto [:gsvelto]]

(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.