1026853 - Experiment is displayed as "pending removal" in detailed view

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

In the Add-ons Manager, Experiments tab: experiments are marked with the "striped" background which means they are marked for removal. Also, if you double click on it, you enter into the detailed view which also displays text telling the user about the pending removal.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: detailview.png

Comment 2

[Georg Fritzsche [:gfritzsche]]

I'm not sure if we mind the striped background in the addon list view, but we definitely need to hide the "pending notification" in the detail view around here:
http://hg.mozilla.org/mozilla-central/annotate/37f08ddaea48/toolkit/mozapps/extensions/content/extensions.css#l243

Due to how we integrate with the addon manager, all experiment addons have "pending disable", which is why we hide the pending notification elements of the addon manager UI. Obviously i missed a spot here.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix pending in detail view

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8447905 [details] [diff] [review]
Fix pending in detail view

r- on the basis that this is an empty patch.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix pending in detail view

Post-lunch this patch looks a little less empty.

Comment 6

[Marco Mucci [:MarcoM]]

Added to Iteration 33.2

Comment 7

[Marco Mucci [:MarcoM]]

Added to Iteration 33.2

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8447936 [details] [diff] [review]
Fix pending in detail view

Review of attachment 8447936 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> Due to how we integrate with the addon manager, all experiment addons have
> "pending disable", which is why we hide the pending notification elements of
> the addon manager UI. Obviously i missed a spot here.

Wait, what? Why? That's new to me, and seems broken and the thing we should fix.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

*yoink!* Patch coming up, following IRC discussion on this.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Comment 11

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8448623 [details] [diff] [review]
Patch v2

Review of attachment 8448623 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +6645,5 @@
>        // operations
>        return AddonManager.PENDING_UNINSTALL;
>      }
>  
> +    // Extensions have an intentional inconsistancy between what the DB says is

nit: s/inconsistancy/inconsistency/

@@ +6653,5 @@
> +      if (aAddon.active && isAddonDisabled(aAddon))
> +        pending |= AddonManager.PENDING_DISABLE;
> +      else if (!aAddon.active && !isAddonDisabled(aAddon))
> +        pending |= AddonManager.PENDING_ENABLE;
> +    }

I wish we could refactor AddonInternal so that there were separate implementations for theme / experiment / extension / ... so that we don't have to spread "if addon.type ..." all over the XPI provider. (I'm a http://www.antiifcampaign.com/ sympathiser...)

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Irving Reid from comment #11)
> I wish we could refactor AddonInternal so that there were separate
> implementations for theme / experiment / extension / ... so that we don't
> have to spread "if addon.type ..." all over the XPI provider. (I'm a
> http://www.antiifcampaign.com/ sympathiser...)

I've been thinking the same thing. I'd already filed bug 986306, but doing it via refactoring into separate objects with a base implementation would also work.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/caefe18d7ffe

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/caefe18d7ffe

Comment 15

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification.

Comment 16

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Kamil can you handle this since it's related to experiments?

Comment 17

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:
* firefox-33.0a1.en-US.win32 [BuildID: 20140707030202]
** http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-07-03-02-02-mozilla-central/

- ensured that active experiment tabs under about:addons are not using stripped background(s)
- ensured that the detailed view doesn't display a message indicating that the experiment is going to be removed on restart
- ensured both "Learn More" & "Telemetry Settings" buttons are still working
- ensured that the "more" button on the experiment tab takes the user into detailed view
- ensured that double clicking on an active/removed experiment under about:addons displays the detailed view
- ensured that there's no UI/UX issues due to removing the stripped background
- moved the machines time ahead by one day and ensured that the "x days remaining" is being counted correctly
- ensured that the "x days remaining" from the experiment tab under about:addons matches the information displayed under the detailed view
- ensured that the experiment expires correctly
- ensured that the experiment is removed when "Remove" on either the experiment tab or on the detailed view is selected
- ensured that the "undo" button is working correctly when removing an experiment
- ensured that the "undo" message is using the correct stripped background (pending removal)
- ensured that the background for the tabs/detailed view are grayed out when the experiment is removed/expired

Comment 18

[Kamil Jozwiak [:kjozwiak]]

Should this be uplifted to beta?? We currently have the search experiment on beta and are about to land the translation experiments for the vi, pl and tr locales.

I ran into this when I was testing the new translation experiment in bug #1041598

Comment 19

[Georg Fritzsche [:gfritzsche]]

Benjamin is away.
Too bad we missed Aurora uplift. I'm not sure if this is something we'll still take on beta given the risk/win here.

[Tracking Requested - why for this release]: Affected release and we ended up doing a few experiments on 32.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8448623 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: Telemetry experiments.
[User impact if declined]: Experiments incorrectly/misleadingly shown as "pending removal" in Addon Manager.
[Describe test coverage new/current, TBPL]: Baked fine for a while on m-c with active experiments. Automated test coverage.
[Risks and why]: Low-risk - only simple branching to avoid setting the "pending disable" state on experiments.
[String/UUID change made/needed]: None.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8448623 [details] [diff] [review]
Patch v2

Given that we're currently running 2 experiments on beta, I'm going to say beta+. However, I would like to see this land before beta2, which goes to build early Monday, July 28. Please land before July 28.

Comment 22

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ecdde13a0aaa

Comment 23

[Kamil Jozwiak [:kjozwiak]]

For each of the experiments that are currently on beta, I went through the test cases in comment # 17 and ensured that the original issue wasn't occurring:

Search Experiment: [Passed]
Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/32.0b6/win32/en-US/

OOPP container unload timeout tester: [Passed]
Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/32.0b6/win32/en-US/

Automatic Translation - PL: [Passed]
Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/32.0b6/win32/pl/

Automatic Translation - TR: [Passed]
Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/32.0b6/win32/tr/

Automatic Translation - VI: [Passed]
Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/32.0b6/win32/vi/