Last Comment Bug 855727 - SimplePush: 409 error code for conflicting channelIDs might be redundant?
: SimplePush: 409 error code for conflicting channelIDs might be redundant?
Status: RESOLVED FIXED
[mentor=nsm]
:
Product: Core
Classification: Components
Component: DOM: Push Notifications (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla25
Assigned To: Jakub Siemiatkowski (:sasklacz)
:
Mentors:
Depends on: simple-push-b2g
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-28 09:05 PDT by Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
Modified: 2013-07-29 15:47 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First patch (2.52 KB, patch)
2013-07-19 09:27 PDT, Jakub Siemiatkowski (:sasklacz)
nsm.nikhil: review+
Details | Diff | Splinter Review

Description Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2013-03-28 09:05:19 PDT
Considering the premise of UUIDs is to be unique, is this check extra paranoid?

Filing follow up to simplify protocol once bug 822712 lands.
Comment 1 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2013-03-28 09:07:07 PDT
See https://bugzilla.mozilla.org/show_bug.cgi?id=822712#c60 (search for string 'case 409')
Comment 2 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2013-03-28 09:11:48 PDT
Also https://bugzilla.mozilla.org/show_bug.cgi?id=822712#c63
Comment 3 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2013-05-22 15:26:33 PDT
Marked as a good first bug for contributors. It's mainly a matter of removing code from the client.
Comment 4 Jakub Siemiatkowski (:sasklacz) 2013-07-17 08:38:19 PDT
Where to download the required repo from ? It's somewhere on moz hg ?
Comment 5 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2013-07-17 09:17:17 PDT
The required code is already in mozilla-central, so once you have a build [1], you can start hacking.

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions
Comment 6 Jakub Siemiatkowski (:sasklacz) 2013-07-18 06:33:25 PDT
Can you assign this ticket to me ? I'm waiting for my MBA to finish the build.
Comment 7 Jakub Siemiatkowski (:sasklacz) 2013-07-19 09:27:43 PDT
Created attachment 778519 [details] [diff] [review]
First patch
Comment 8 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2013-07-24 11:03:50 PDT
Comment on attachment 778519 [details] [diff] [review]
First patch

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

::: dom/push/src/PushService.jsm
@@ +1134,2 @@
>  
> +    if (reply.status) {

|reply.status| is always present, so you cannot error based on its presence alone. A non-200 value is considered an error.

@@ +1134,4 @@
>  
> +    if (reply.status) {
> +      debug("General failure " + reply.status);
> +      throw { requestID: aPageRecord.requestID, error: reply.error };    

Trailing whitespace.
Comment 9 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2013-07-24 11:05:38 PDT
Does the tef server send 409?
Comment 10 Guillermo López :willyaranda (probably SLOW response) 2013-07-24 14:42:53 PDT
Nope, we are not sending that statuscode.
Comment 11 Jakub Siemiatkowski (:sasklacz) 2013-07-25 08:32:38 PDT
In the original version it was :


>    switch (reply.status) {
>      case kERROR_CHID_CONFLICT:
>        ...
>      default:
>        ...
>    }

and the function itself is called when an error occurs (as the name suggests). So we should always just throw the default or add additional check for status different than 200 ?
Comment 12 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2013-07-25 09:18:07 PDT
Comment on attachment 778519 [details] [diff] [review]
First patch

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

Yes, you're right. In that case the code is ok, just take care of the trailing whitespace.
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-07-29 09:03:17 PDT
https://hg.mozilla.org/projects/birch/rev/ed1b011524af

Thanks for the patch, Jakub! One request, please use commit messages that explain what the patch is doing rather than just restating the bug title. Thanks!
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-07-29 15:47:41 PDT
https://hg.mozilla.org/mozilla-central/rev/ed1b011524af

Note You need to log in before you can comment on or make changes to this bug.