Create function in the API to ping the blocklist

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: remus.pop, Assigned: remus.pop)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
The function will be created in the utils API.
(Assignee)

Updated

7 years ago
Blocks: 684679
(Assignee)

Comment 1

7 years ago
Created attachment 599145 [details] [diff] [review]
patch v1 (all branches)

Added function pingBlocklist() to the utils module so that Firefox refreshes it's blocklist.
Attachment #599145 - Flags: review?(alex.lakatos)
Comment on attachment 599145 [details] [diff] [review]
patch v1 (all branches)

A small nit. r+ with this addressed.
> /**
>+ * Pings the blocklist so Firefox gets aware of the new  blocklist
>+ */
Helper function to ping the blocklist URL so Firefox gets aware of the new  blocklist
Attachment #599145 - Flags: review?(alex.lakatos) → review-
(In reply to Alex Lakatos[:AlexLakatos] from comment #2)
> Helper function to ping the blocklist URL so Firefox gets aware of the new 
> blocklist
make that blocklist Service
(Assignee)

Comment 4

7 years ago
Created attachment 599149 [details] [diff] [review]
patch v2 (all branches)

Fixed.
Attachment #599145 - Attachment is obsolete: true
Attachment #599149 - Flags: review?(alex.lakatos)
Attachment #599149 - Flags: review?(hskupin)
Attachment #599149 - Flags: review?(alex.lakatos)
Attachment #599149 - Flags: review+
Comment on attachment 599149 [details] [diff] [review]
patch v2 (all branches)

Henrik is out on PTO so asking if Geo can do the review here.
Attachment #599149 - Flags: review?(hskupin) → review?(gmealer)
Comment on attachment 599149 [details] [diff] [review]
patch v2 (all branches)

Code is a simple change and looks fine. One comment:

I'd prefer it were called updateBlocklist(). Generally if your comment is "do [foo] so that [bar]," the function should be named after [bar]. 

Reason is you're pinging, but you're pinging in order to update. If updating happened some other way later (say, a new API call) you'd change the function to the new way. It's nice if the name still stays accurate in that case. 

Naming after desired result also makes the code communicate more to the first-time reader. "Why would they ping?" vs. "Oh, they're updating."

That said, I'm aware that https://wiki.mozilla.org/Blocklisting/Testing specifically calls it "ping," so will let this be a judgment call on your part.

r=me after considering that.
Attachment #599149 - Flags: review?(gmealer) → review+
I agree with your feedback Geo. Calling it updateBlocklist will be more universally understood. We should endeavour to write code that is as universally understandable as possible.
(Assignee)

Comment 8

7 years ago
Created attachment 599500 [details] [diff] [review]
patch v3 [landed]

I also agree about naming. This is more readable now.
Attachment #599149 - Attachment is obsolete: true
Attachment #599500 - Flags: review?(gmealer)
Comment on attachment 599500 [details] [diff] [review]
patch v3 [landed]

LGTM, thanks for the change! r+
Attachment #599500 - Flags: review?(gmealer) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Remus, for follow-up patches against shared modules please take care of the right alphabetical order of helper functions. If we are too loose here modules will become harder to read. Thanks.
(Assignee)

Comment 12

7 years ago
Sorry about that. I forgot to check after updating the function name. Should I provide a new patch?
Does this need to go to esr branch too?
Yeah, please do so. Lets re-use this bug. Further we don't checkin new tests and APIs on ESR branches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

7 years ago
Created attachment 601601 [details] [diff] [review]
patch v4 (all branches)

Moved updateBlocklist so now it's in alphabetical order.
Attachment #601601 - Flags: review?(hskupin)
Comment on attachment 601601 [details] [diff] [review]
patch v4 (all branches)

Please also change the order in the exports section. With it fixed good to go.
Attachment #601601 - Flags: review?(hskupin) → review+
(Assignee)

Comment 16

7 years ago
Created attachment 601923 [details] [diff] [review]
patch v5 (all) [landed]

Updated as requested.
Attachment #601601 - Attachment is obsolete: true
Attachment #601923 - Flags: review?(hskupin)
Attachment #601923 - Flags: review?(hskupin) → review+
Can someone check this in please?
Keywords: checkin-needed
What's checkin policy on these? I thought it was within Anthony's team. If not, I can land it once clarified.
It's on my plate, Geo. I'll get to it later tomorrow once 11.0b5 is released. Feel free to take it if it can't wait until then.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.