Last Comment Bug 630575 - Properly distinguish soft and hard blocked items in the update pings
: Properly distinguish soft and hard blocked items in the update pings
Status: RESOLVED FIXED
[has patch]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla2.0b12
Assigned To: Dave Townsend [:mossop]
:
: Andy McKay [:andym]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-01 10:00 PST by Dave Townsend [:mossop]
Modified: 2011-03-23 11:29 PDT (History)
3 users (show)
dtownsend: in‑testsuite+
dtownsend: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.17-fixed
.19-fixed


Attachments
patch rev 1 (7.42 KB, patch)
2011-02-09 16:59 PST, Dave Townsend [:mossop]
blair: review+
gavin.sharp: approval2.0+
Details | Diff | Splinter Review
branch patch (8.17 KB, patch)
2011-03-07 11:59 PST, Dave Townsend [:mossop]
robert.strong.bugs: review+
dveditz: approval1.9.2.17+
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review

Description User image Dave Townsend [:mossop] 2011-02-01 10:00:08 PST
Currently in Firefox 3.5 and 3.6 we only include the "blocklisted" state if an item is hardblocked.
In Firefox 4.0 we include the "blocklisted" state if an item is soft or hard blocked.

This causes confusing data and for the branches makes it impossible to verify that blocklist changes are having any effect. We should split it and send "blocklisted" for hard blocks only and "softblocked" for soft blocked items.
Comment 1 User image Dave Townsend [:mossop] 2011-02-09 16:59:02 PST
Created attachment 511242 [details] [diff] [review]
patch rev 1

Just for Firefox 4 for now this makes the changes. Turns out there were no tests for this so I added those. I think it's important to take this and bring us back into parity with 3.6 for stats.
Comment 2 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2011-02-10 01:32:50 PST
Comment on attachment 511242 [details] [diff] [review]
patch rev 1

Makes sense.
Comment 3 User image Dave Townsend [:mossop] 2011-02-10 10:25:37 PST
For drivers, this is really safe code that fixes a regression from 3.6 that generates us confusing stats when we try to evaluate the pickup rate of the blocklist.
Comment 4 User image Dave Townsend [:mossop] 2011-02-10 10:35:56 PST
Landed: http://hg.mozilla.org/mozilla-central/rev/53d06f07a4d2
Comment 5 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2011-02-11 19:19:25 PST
Dave, what's the best way to verify this fix at least for the softblocked items? In which query will the softblocked items be contained in?
Comment 6 User image Dave Townsend [:mossop] 2011-02-15 09:41:07 PST
(In reply to comment #5)
> Dave, what's the best way to verify this fix at least for the softblocked
> items? In which query will the softblocked items be contained in?

It shows up in the update ping to AMO or the %ITEM_STATUS% part of a custom updateURL
Comment 7 User image Dave Townsend [:mossop] 2011-03-07 11:59:14 PST
Created attachment 517484 [details] [diff] [review]
branch patch

This adds the softblocked status to the 1.9.2 branch and adds the same tests.
Comment 8 User image Dave Townsend [:mossop] 2011-03-07 12:45:47 PST
Comment on attachment 517484 [details] [diff] [review]
branch patch

I'd like approval to land this on the branches to ensure that we get good feedback from changes to the blocklist. It is a pretty low risk patch and adds more tests than we had previously for this status reporting.
Comment 9 User image Daniel Veditz [:dveditz] 2011-03-14 10:27:19 PDT
(In reply to comment #3)
> For drivers, this is really safe code that fixes a regression from 3.6 that

If it's a regression from 3.6 why do we need this patch in 3.6? Or if you meant a regression introduced _in_ 3.6 it's still not clear why you want this patch in 3.5.
Comment 10 User image Dave Townsend [:mossop] 2011-03-14 11:51:03 PDT
(In reply to comment #9)
> (In reply to comment #3)
> > For drivers, this is really safe code that fixes a regression from 3.6 that
> 
> If it's a regression from 3.6 why do we need this patch in 3.6? Or if you meant
> a regression introduced _in_ 3.6 it's still not clear why you want this patch
> in 3.5.

Neither, that comment was for the 2.0 drivers where we changed the behaviour.

Firefox 3.6 and 3.5 tell AMO when they know an add-on that a user has installed is hard blocked but not when it is soft blocked (which 4.0 does do now). We have been using the stats from these pings in the past to verify the rollout of the blocklist updates and without the soft blocked information we can't verify that kind of block is rolling out as we expect.
Comment 11 User image Daniel Veditz [:dveditz] 2011-03-16 10:44:41 PDT
Comment on attachment 517484 [details] [diff] [review]
branch patch

Approved for 1.9.2.16 and 1.9.1.18, a=dveditz for release-drivers

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