Closed Bug 855727 Opened 10 years ago Closed 10 years ago

SimplePush: 409 error code for conflicting channelIDs might be redundant?

Categories

(Core :: DOM: Push Notifications, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: nsm, Assigned: sasklacz)

References

Details

(Whiteboard: [mentor=nsm])

Attachments

(1 file)

Considering the premise of UUIDs is to be unique, is this check extra paranoid?

Filing follow up to simplify protocol once bug 822712 lands.
Marked as a good first bug for contributors. It's mainly a matter of removing code from the client.
Whiteboard: [mentor=nsm]
Assignee: nsm.nikhil → nobody
Where to download the required repo from ? It's somewhere on moz hg ?
Flags: needinfo?(nsm.nikhil)
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
Flags: needinfo?(nsm.nikhil)
Can you assign this ticket to me ? I'm waiting for my MBA to finish the build.
Assignee: nobody → owcarnia
Attached patch First patchSplinter Review
Attachment #778519 - Flags: review?(nsm.nikhil)
Component: General → DOM: Push Notifications
Product: Boot2Gecko → Core
Version: unspecified → Trunk
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.
Attachment #778519 - Flags: review?(nsm.nikhil) → review-
Does the tef server send 409?
Flags: needinfo?(willyaranda)
Nope, we are not sending that statuscode.
Flags: needinfo?(willyaranda)
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 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.
Attachment #778519 - Flags: review- → review+
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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed1b011524af
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.