Closed Bug 1026853 Opened 5 years ago Closed 5 years ago

Experiment is displayed as "pending removal" in detailed view

Categories

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

defect
Not set

Tracking

(firefox31 unaffected, firefox32 verified, firefox33 verified)

VERIFIED FIXED
Firefox 33
Tracking Status
firefox31 --- unaffected
firefox32 --- verified
firefox33 --- verified

People

(Reporter: Felipe, Assigned: Unfocused)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached image detailview.png
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.
Flags: firefox-backlog+
Whiteboard: p=3
Points: --- → 3
Whiteboard: p=3
Assignee: nobody → georg.fritzsche
Attached patch Fix pending in detail view (obsolete) — Splinter Review
Attachment #8447905 - Flags: review?(bmcbride)
Comment on attachment 8447905 [details] [diff] [review]
Fix pending in detail view

r- on the basis that this is an empty patch.
Attachment #8447905 - Flags: review?(bmcbride) → review-
Attached patch Fix pending in detail view (obsolete) — Splinter Review
Post-lunch this patch looks a little less empty.
Attachment #8447905 - Attachment is obsolete: true
Attachment #8447936 - Flags: review?(bmcbride)
Added to Iteration 33.2
Status: NEW → ASSIGNED
Iteration: --- → 33.2
Whiteboard: [qa+]
Added to Iteration 33.2
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.
Attachment #8447936 - Flags: review?(bmcbride) → review-
*yoink!* Patch coming up, following IRC discussion on this.
Assignee: georg.fritzsche → bmcbride
Attached patch Patch v2Splinter Review
Attachment #8447936 - Attachment is obsolete: true
Attachment #8448623 - Flags: review?(irving)
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...)
Attachment #8448623 - Flags: review?(irving) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/caefe18d7ffe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Hi Florin, can a QA contact be assigned for verification.
Flags: needinfo?(florin.mezei)
Kamil can you handle this since it's related to experiments?
Flags: needinfo?(florin.mezei)
QA Contact: kamiljoz
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
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
QA Whiteboard: [qa!]
Whiteboard: [qa!]
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
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
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.
Attachment #8448623 - Flags: approval-mozilla-beta?
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.
Attachment #8448623 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa!] → [qa+]
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/
QA Whiteboard: [qa+] → [qa!]
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.