986677 - Include time left in experiment in addon-manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #973992 +++

We want the Addon Manager to show the time left in an active experiment. This is a follow-up from bug 973992.

This will likely either involve XBL or a gross hack to put the time in an existing add-on property, such as description.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Addon manager UI fixes & adding experiments state and time

Blair, i'm running too short on time today, so please excuse the messy patch (left-over dump()s and short-cuts in Experiments.jsm).
Is the approach taken in toolkit/mozapps/extensions/ acceptable?

Note: This is based on gps' latest telex/addonmanager bookmark.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8400758 [details] [diff] [review]
Addon manager UI fixes & adding experiments state and time

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

Yep, looks good.

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
@@ +98,5 @@
>  #LOCALIZATION NOTE (details.notification.upgrade) %1$S is the add-on name, %2$S is brand name
>  details.notification.upgrade=%1$S will be updated after you restart %2$S.
>  
> +#LOCALIZATION NOTE (details.experiment.time.remaining) %1$S is the number of days from now that the experiment will remain active.
> +details.experiment.time.remaining=%1$S days remaining.

Don't forget to use PluralForm with these strings.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Current AM Experiments look

This is how it looks with the changes i have now.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix the visibility of some elements that are irrelevant for experiments.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Add experiment state and time remaining to the UI.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Remaining work:
* Refresh AM experiments UI on changes
* tests

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> Created attachment 8401343 [details] [diff] [review]
> Add experiment state and time remaining to the UI.

Note: I'm not really happy about the way this puts endDate on the Addon from extensions.js for active experiment addons, but i haven't found a better way to get that information in so far.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8401343 [details] [diff] [review]
Add experiment state and time remaining to the UI.

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

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2566,5 @@
>        var elements = [];
>  
> +      for (let addonItem of aAddonsList) {
> +        if (addonItem.type == "experiment" && addonItem.isActive) {
> +          let experiment = Experiments.instance().getActiveExperiment();

This will raise if running in an application that doesn't have the Experiments module. You need to trap the import failure or conditionally do it if the Cc entry is there.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Add experiment state and time remaining to the UI.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Moved the refresh issue out to bug 992258 as it doesn't block the changes here.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8401902 [details] [diff] [review]
Add experiment state and time remaining to the UI.

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

You're missing changes to the details view (double click an add-on item, or click the More button). See id=details-view in extensions.xul and gDetailsView in extensions.js

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2569,5 @@
> +        if (addonItem.type == "experiment" && addonItem.isActive
> +            && "@mozilla.org/browser/experiments-service;1" in Cc) {
> +          let experiment = Experiments.instance().getActiveExperiment();
> +          if (experiment) {
> +            addonItem.endDate = experiment.endDate;

Hm, I don't like the idea of UI code modifying the Addon object itself. Better to set this as an attribute on the node returned from createItem (or even better, do that in createItem).

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +804,5 @@
>              <xul:label anonid="date-updated" class="date-updated"
>                         unknown="&addon.unknownDate;"/>
>            </xul:hbox>
> +          <xul:hbox class="experiment-detail-container">
> +            <xul:label anonid="experiment-state" class="experiment-state"/>

Wasn't the mockup showing this to be green for active, and <some other color> for complete?

@@ +1358,5 @@
>  
>            this._debugBtn.disabled = this._debugBtn.hidden = !debuggable
> +
> +          if (this.mAddon.type == "experiment") {
> +            let prefix = "details.experiment.";

Strings with the "details." prefix should only be used in the details view. You need two sets of strings, because the context is slightly different (and there is different space allowances) - once for the list items, one for the details view.

@@ +1365,5 @@
> +            let stateKey = prefix + "state." + (active ? "active" : "complete");
> +            this._experimentState.value = gStrings.ext.GetStringFromName(stateKey);
> +
> +            let now = new Date().getTime();
> +            let end = this.mAddon.endDate;

Obviously we need some testing for this, because at the moment you're only setting .endDate for the current active experiment.

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #11)
> ::: toolkit/mozapps/extensions/content/extensions.js
> @@ +2569,5 @@
> > +        if (addonItem.type == "experiment" && addonItem.isActive
> > +            && "@mozilla.org/browser/experiments-service;1" in Cc) {
> > +          let experiment = Experiments.instance().getActiveExperiment();
> > +          if (experiment) {
> > +            addonItem.endDate = experiment.endDate;
> 
> Hm, I don't like the idea of UI code modifying the Addon object itself.
> Better to set this as an attribute on the node returned from createItem (or
> even better, do that in createItem).



> ::: toolkit/mozapps/extensions/content/extensions.xml
> @@ +804,5 @@
> >              <xul:label anonid="date-updated" class="date-updated"
> >                         unknown="&addon.unknownDate;"/>
> >            </xul:hbox>
> > +          <xul:hbox class="experiment-detail-container">
> > +            <xul:label anonid="experiment-state" class="experiment-state"/>
> 
> Wasn't the mockup showing this to be green for active, and <some other
> color> for complete?

I am short-cutting a little here so we can actually get something landed. I am no UI person and happy to take suggestions what to put in there, but i can't built an icon here which seems needed to cover what the mockup does.

> @@ +1365,5 @@
> > +            let now = new Date().getTime();
> > +            let end = this.mAddon.endDate;
> 
> Obviously we need some testing for this, because at the moment you're only
> setting .endDate for the current active experiment.

Not obvious: the PreviousExperimentsProvider has .endDate for non-active experiments.

Comment 13

[Georg Fritzsche [:gfritzsche]]

It looks like we don't have an icon yet for the green/grey circle in the mockup.
Sensible options for the greenish icon right now:
* bullet character, css style
* SVG circle

I'll delegate having a proper icon to a follow-up.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Attachment: Current AM Experiments look #2

Using SVG for now seems to behave decently. I'm not sure how to convince it to properly align with the text and also not sure if it matters too much for now.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Screenshot: List view

Comment 16

[Georg Fritzsche [:gfritzsche]]

Attachment: Screenshot: Detail view, active

Comment 17

[Georg Fritzsche [:gfritzsche]]

Attachment: Screenshot: previous

Comment 18

[Benjamin Smedberg]

The experiment-detail view needs padding. Maybe it needs a detail-view-container element so that it gets the normal 2em padding on that?

Comment 19

[Georg Fritzsche [:gfritzsche]]

Attachment: Add experiment state and time remaining to the UI.

This should be nearly done.
The remaining issue is that i can't get the activity indicator to align vertically with the labels in both the list and the detail view.

Comment 20

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> The experiment-detail view needs padding. Maybe it needs a
> detail-view-container element so that it gets the normal 2em padding on that?

Ah, hm, that must be one of the hidden detail-containers that had a margin-bottom :-/

Comment 21

[Georg Fritzsche [:gfritzsche]]

Attachment: Screenshot: Detail view, active

Fixed margin issue.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Attachment: Screenshot: Detail view, previous

Fixed margin issue.

Comment 23

[Georg Fritzsche [:gfritzsche]]

Attachment: Add experiment state and time remaining to the UI.

Fixed margin issue, still no idea about the vertical layout (green activity circle vs. neighbouring labels).

Comment 24

[Benjamin Smedberg]

If you make both the svg and the other text inline-block and set vertical-align: middle on both of them, it works:

http://jsfiddle.net/4LHmg/1/

Comment 25

[Georg Fritzsche [:gfritzsche]]

Hm, thanks Benjamin, but the bullet in the detail-view is still behaving differently.

Comment 26

[Georg Fritzsche [:gfritzsche]]

Attachment: Shortcut for layout testing

Comment 27

[Georg Fritzsche [:gfritzsche]]

Patches to import on top of fx-team for local builds:

https://reviewboard.allizom.org/r/54/diff/raw/
attachment 8401342 [details] [diff] [review]
attachment 8403372 [details] [diff] [review]
attachment 8403893 [details] [diff] [review]

Comment 28

[:Gijs (he/him)]

The only thing that needs to happen for the alignment to work is to remove the align="start" attribute on detail-experiment-container.

Confusingly, in XUL, "align" is about the alignment *in the direction orthogonal to the box*. So, in an hbox, it's about vertical alignment, in a vbox, about horizontal alignment. For aligning elements in the direction of the box, use the "pack" attribute.

Comment 29

[Georg Fritzsche [:gfritzsche]]

Attachment: Screenshot: List view

Thanks for the help here, this looks passable now.

Comment 30

[Georg Fritzsche [:gfritzsche]]

Attachment: Screenshot: Detail view, active

Comment 31

[Georg Fritzsche [:gfritzsche]]

Attachment: Screenshot: Detail view, previous

Comment 32

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix the visibility of some elements that are irrelevant for experiments.

Expanded hiding undesired elements to the detail view.

Comment 33

[Georg Fritzsche [:gfritzsche]]

Attachment: Add experiment state and time remaining to the UI.

Comment 34

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8403948 [details] [diff] [review]
Fix the visibility of some elements that are irrelevant for experiments.

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

(Sorry for the delay - finally home after conferences and travel.)

r+ with a couple of tweaks...

::: toolkit/mozapps/extensions/content/extensions.css
@@ +238,5 @@
> +#addons-page .view-pane[type="experiment"] .warning,
> +#addons-page .view-pane[type="experiment"] .pending,
> +#addons-page .view-pane[type="experiment"] .disabled-postfix,
> +#addons-page .view-pane[type="experiment"] .update-postfix,
> +#addons-page .view-pane[type="experiment"] .version,

Nit: While you're tweaking this file already (see below), better to remove the "#addons-page" part from these selectors - it's just going to to make the selectors work slower, without any added benefit.

@@ +242,5 @@
> +#addons-page .view-pane[type="experiment"] .version,
> +#detail-view[type="experiment"] .alert-container,
> +#detail-view[type="experiment"] .error,
> +#detail-view[type="experiment"] .warning,
> +#detail-view[type="experiment"] .pending,

#detail-view is in fact a .view-pane, so you've introduced redundant selectors in this revision of this patch - at least for .error, .warning, and .pending.

Comment 35

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8403950 [details] [diff] [review]
Add experiment state and time remaining to the UI.

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

r+ with a few small fixups.

On the assumption we need this landed ASAP (not helped my my untimely conference/travel schedule and health issues), can you file a bug to retroactively add some tests for this?

::: browser/experiments/Experiments.jsm
@@ +2102,5 @@
> +   * The end-date of the experiment, required for the Addon Manager UI.
> +   */
> +
> +   get endDate() {
> +     return this._endDate;

Ah-ha!

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
@@ +97,5 @@
>  details.notification.uninstall=%1$S will be uninstalled after you restart %2$S.
>  #LOCALIZATION NOTE (details.notification.upgrade) %1$S is the add-on name, %2$S is brand name
>  details.notification.upgrade=%1$S will be updated after you restart %2$S.
>  
> +#LOCALIZATION NOTE (details.experiment.time.daysRemaining) %1$S is the number of days from now that the experiment will remain active (detail view).

You're referencing "%1$S" in all these comments, but all the strings are actually using "#1".

::: toolkit/mozapps/extensions/content/extensions.css
@@ +250,5 @@
>  }
> +
> +.view-pane:not([type="experiment"]) .experiment-container,
> +.view-pane:not([type="experiment"]) #detail-experiment-container {
> +  display: none;

Everything *except this rule* doesn't belong in content - they should instead be added in /toolkit/themes/[windows|osx|linux]/mozapps/extensions/extensions.css

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2904,5 @@
> +      let stateKey = prefix + "state." + (active ? "active" : "complete");
> +      let node = document.getElementById("detail-experiment-state");
> +      node.value = gStrings.ext.GetStringFromName(stateKey);
> +
> +      let now = new Date().getTime();

Don't need to construct a new Date object for this, you can/should just use: Date.now()

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1369,5 @@
> +
> +            let stateKey = prefix + "state." + (active ? "active" : "complete");
> +            this._experimentState.value = gStrings.ext.GetStringFromName(stateKey);
> +
> +            let now = new Date().getTime();

Ditto, Date.now()

Comment 36

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix the visibility of some elements that are irrelevant for experiments.

Review comments addressed.

Comment 37

[Georg Fritzsche [:gfritzsche]]

Attachment: Add experiment state and time remaining to the UI.

Review comments addressed.
However, the styling stops working when i move the rules from
  toolkit/mozapps/extensions/content/extensions.css
to
  toolkit/themes/osx/mozapps/extensions/extensions.css
... with most notably the bullet (activity indicator) coloring missing now.

Comment 38

[Georg Fritzsche [:gfritzsche]]

Attachment: Shortcut for CSS testing

Shortcut for possible help here.

Comment 39

[Georg Fritzsche [:gfritzsche]]

Patch stack (based on fx-team or probably m-c):
* attachment 8410342 [details] [diff] [review]
* attachment 8410337 [details] [diff] [review]
* attachment 8410341 [details] [diff] [review]
* attachment 8410356 [details] [diff] [review]

Comment 40

[Georg Fritzsche [:gfritzsche]]

Attachment: Add experiment state and time remaining to the UI.

felipe noticed a namespace issue here.

Removing the default namespace here makes the .experiment-bullet/ rule work: http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/mozapps/extensions/extensions.css#9

Adding namespace handling like in this patch version (in themes/osx only) doesn't fix the issue though.

Comment 41

[Benjamin Smedberg]

According to dbaron the CSS default namespace is unlikely to be performance-sensitive in this case, so we should just remove it and move on.

Comment 42

[Georg Fritzsche [:gfritzsche]]

I was hesitant to remove that as i fear this silently breaking other rules.
It seems fine on OS X, still need to check on it on the other platforms though (and needs some special QA attention there on not breaking other Addon Manager UI parts).

https://tbpl.mozilla.org/?tree=Try&rev=b4ca6e7f77e0

Comment 43

[Georg Fritzsche [:gfritzsche]]

Good on try and when checking the UI locally on OS X, Win, Linux:
https://hg.mozilla.org/integration/fx-team/rev/bd993b75b61f
https://hg.mozilla.org/integration/fx-team/rev/18c0f7152372

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd993b75b61f
https://hg.mozilla.org/mozilla-central/rev/18c0f7152372

Comment 45

[Marco Mucci [:MarcoM]]

Hey Kamil, will you have time to verify this bug before the desktop iteration ends this Monday?

Comment 46

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the latest Nightly build from the following location:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-25-03-02-09-mozilla-central/

Because the current experiment is being distributed to 25% of the install base, it's difficult to test using the "official" Mozilla staging server so I setup my own here:
- http://kamiljozwiak.com/firefox-manifest.json

- Ensured that the current UI matches the screenshot examples under the tickets attachments (Windows, OSX)
- Ensured that the experiment indicates that it's currently "Active" with the status bullet being green
- Ensured that once the experiment is active, the "Active" column appears as "true" under about:support
- Ensured that when the experiment is active, the "experiments.activeExperiment" preference is set as "true"
- Ensured that the experiments "Active" matches the "End Date" being listed under about:support
- Moved the date on the computer forward and ensured that the experiment correctly displayed the remaining time
- Ensured that once the experiment has expired, the "Active" column appears as "false" under about:support
- Ensured that the experiment appears greyed out when expired, ensured that the status bullet also appeared grey
- Ensured that once the experiment has expired, the "experiments.activeExperiment" preference is set as "false" when there's no other experiments enabled
- Ensured that the add-on manager correctly displays approximate time the experiment expired
- Ensured that the "Preferences" and "Remove" buttons are working under the add-on manager
- Ensured that the context menu when right clicking on an experiment worked without any issues

Comment 47

[Georg Fritzsche [:gfritzsche]]

Fixing status flag as this landed on 31.