Closed Bug 729036 Opened 8 years ago Closed 8 years ago
Create function in the API to ping the blocklist
The function will be created in the utils API.
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
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.
I also agree about naming. This is more readable now.
Comment on attachment 599500 [details] [diff] [review] patch v3 [landed] LGTM, thanks for the change! r+
Attachment #599500 - Flags: review?(gmealer) → review+
Comment on attachment 599500 [details] [diff] [review] patch v3 [landed] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/23bacba3b900 (default) http://hg.mozilla.org/qa/mozmill-tests/rev/b3cd0227d03f (mozilla-aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/8dca8cd278c0 (mozilla-beta) http://hg.mozilla.org/qa/mozmill-tests/rev/388798abdc0f (mozilla-release)
Attachment #599500 - Attachment description: patch v3 (all branches) → patch v3 [landed]
Status: ASSIGNED → RESOLVED
Closed: 8 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.
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 → ---
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+
Updated as requested.
Attachment #601923 - Flags: review?(hskupin) → review+
Can someone check this in please?
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.
Comment on attachment 601923 [details] [diff] [review] patch v5 (all) [landed] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/aa5be88dc23e (default) http://hg.mozilla.org/qa/mozmill-tests/rev/f4fd3f0f313d (mozilla-aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/690c007aafd3 (mozilla-beta) http://hg.mozilla.org/qa/mozmill-tests/rev/3c1d62b99858 (mozilla-release)
Attachment #601923 - Attachment description: patch v5 (all branches) → patch v5 (all) [landed]
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.