729036 - Create function in the API to ping the blocklist

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

The function will be created in the utils API.

Comment 1

[Remus Pop (:RemusPop)]

Attachment: patch v1 (all branches)

Added function pingBlocklist() to the utils module so that Firefox refreshes it's blocklist.

Comment 2

[Alex Lakatos[:AlexLakatos]]

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

Comment 3

[Alex Lakatos[:AlexLakatos]]

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

[Remus Pop (:RemusPop)]

Attachment: patch v2 (all branches)

Fixed.

Comment 5

[u279076]

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.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

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.

Comment 7

[u279076]

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.

Comment 8

[Remus Pop (:RemusPop)]

Attachment: patch v3 [landed]

I also agree about naming. This is more readable now.

Comment 9

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 599500 [details] [diff] [review]
patch v3 [landed]

LGTM, thanks for the change! r+

Comment 10

[u279076]

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)

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 12

[Remus Pop (:RemusPop)]

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?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yeah, please do so. Lets re-use this bug. Further we don't checkin new tests and APIs on ESR branches.

Comment 14

[Remus Pop (:RemusPop)]

Attachment: patch v4 (all branches)

Moved updateBlocklist so now it's in alphabetical order.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 16

[Remus Pop (:RemusPop)]

Attachment: patch v5 (all) [landed]

Updated as requested.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can someone check this in please?

Comment 18

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

What's checkin policy on these? I thought it was within Anthony's team. If not, I can land it once clarified.

Comment 19

[u279076]

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 20

[u279076]

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)