Include time left in experiment in addon-manager

VERIFIED FIXED in Firefox 31

Status

()

defect
P2
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: gfritzsche)

Tracking

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox31 verified)

Details

(Whiteboard: p=8 s=it-31c-30a-29b.3 [qa!])

Attachments

(6 attachments, 17 obsolete attachments)

503.53 KB, image/png
Details
513.79 KB, image/png
Details
507.89 KB, image/png
Details
1.16 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
2.62 KB, patch
Details | Diff | Splinter Review
21.19 KB, patch
Details | Diff | Splinter Review
Reporter

Description

5 years ago
+++ 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.
Whiteboard: p=0
Whiteboard: p=0 → p=8

Updated

5 years ago
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
Whiteboard: p=8 → p=8 s=it-31c-30a-29b.2
Assignee

Comment 1

5 years ago
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.
Attachment #8400758 - Flags: feedback?(bmcbride)
QA Contact: kamiljoz
Whiteboard: p=8 s=it-31c-30a-29b.2 → p=8 s=it-31c-30a-29b.2 [qa+]
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
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.
Attachment #8400758 - Flags: feedback?(bmcbride) → feedback+
Assignee

Comment 3

5 years ago
Posted image Current AM Experiments look (obsolete) —
This is how it looks with the changes i have now.
Assignee

Comment 4

5 years ago
Attachment #8400758 - Attachment is obsolete: true
Attachment #8401342 - Flags: review?(bmcbride)
Assignee

Comment 5

5 years ago
Attachment #8401343 - Flags: review?(bmcbride)
Assignee

Comment 6

5 years ago
Remaining work:
* Refresh AM experiments UI on changes
* tests
Assignee

Comment 7

5 years ago
(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.
Reporter

Comment 8

5 years ago
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.
Attachment #8401342 - Flags: review?(bmcbride) → review+
Assignee

Comment 9

5 years ago
Attachment #8401343 - Attachment is obsolete: true
Attachment #8401343 - Flags: review?(bmcbride)
Attachment #8401902 - Flags: review?(bmcbride)
Assignee

Updated

5 years ago
Blocks: 992258
Assignee

Comment 10

5 years ago
Moved the refresh issue out to bug 992258 as it doesn't block the changes here.
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.
Attachment #8401902 - Flags: review?(bmcbride) → review-
Assignee

Comment 12

5 years ago
(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.
Assignee

Updated

5 years ago
Depends on: 990111
Assignee

Comment 13

5 years ago
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.
Assignee

Comment 14

5 years ago
Posted image Current AM Experiments look #2 (obsolete) —
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.
Attachment #8401288 - Attachment is obsolete: true
Assignee

Comment 15

5 years ago
Posted image Screenshot: List view (obsolete) —
Attachment #8402803 - Attachment is obsolete: true
Assignee

Comment 16

5 years ago
Posted image Screenshot: Detail view, active (obsolete) —
Assignee

Comment 17

5 years ago
Posted image Screenshot: previous (obsolete) —
The experiment-detail view needs padding. Maybe it needs a detail-view-container element so that it gets the normal 2em padding on that?
Assignee

Comment 19

5 years ago
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.
Attachment #8401902 - Attachment is obsolete: true
Assignee

Comment 20

5 years ago
(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 :-/
Assignee

Comment 21

5 years ago
Posted image Screenshot: Detail view, active (obsolete) —
Fixed margin issue.
Attachment #8403289 - Attachment is obsolete: true
Assignee

Comment 22

5 years ago
Posted image Screenshot: Detail view, previous (obsolete) —
Fixed margin issue.
Attachment #8403291 - Attachment is obsolete: true
Assignee

Comment 23

5 years ago
Fixed margin issue, still no idea about the vertical layout (green activity circle vs. neighbouring labels).
Attachment #8403302 - Attachment is obsolete: true
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/
Assignee

Comment 25

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

Comment 26

5 years ago
Posted patch Shortcut for layout testing (obsolete) — Splinter Review

Comment 28

5 years ago
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.
Assignee

Comment 29

5 years ago
Thanks for the help here, this looks passable now.
Attachment #8403288 - Attachment is obsolete: true
Assignee

Comment 30

5 years ago
Attachment #8403370 - Attachment is obsolete: true
Assignee

Comment 31

5 years ago
Attachment #8403371 - Attachment is obsolete: true
Assignee

Comment 32

5 years ago
Expanded hiding undesired elements to the detail view.
Attachment #8403948 - Flags: review?(bmcbride)
Assignee

Updated

5 years ago
Attachment #8401342 - Attachment is obsolete: true
Assignee

Comment 33

5 years ago
Attachment #8403372 - Attachment is obsolete: true
Attachment #8403950 - Flags: review?(bmcbride)
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.
Attachment #8403948 - Flags: review?(bmcbride) → review+
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()
Attachment #8403950 - Flags: review?(bmcbride) → review+
Whiteboard: p=8 s=it-31c-30a-29b.2 [qa+] → p=8 s=it-31c-30a-29b.3 [qa+]
Assignee

Comment 36

5 years ago
Review comments addressed.
Attachment #8403948 - Attachment is obsolete: true
Attachment #8410337 - Flags: review+
Assignee

Comment 37

5 years ago
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.
Attachment #8403950 - Attachment is obsolete: true
Assignee

Comment 38

5 years ago
Shortcut for possible help here.
Attachment #8403893 - Attachment is obsolete: true
Assignee

Comment 40

5 years ago
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.
Attachment #8410341 - Attachment is obsolete: true
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.
Assignee

Comment 42

5 years ago
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
Assignee

Updated

5 years ago
Blocks: 1000114
https://hg.mozilla.org/mozilla-central/rev/bd993b75b61f
https://hg.mozilla.org/mozilla-central/rev/18c0f7152372
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1000796
Hey Kamil, will you have time to verify this bug before the desktop iteration ends this Monday?
Flags: needinfo?(kamiljoz)
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
Whiteboard: p=8 s=it-31c-30a-29b.3 [qa+] → p=8 s=it-31c-30a-29b.3 [qa!]
Assignee

Comment 47

5 years ago
Fixing status flag as this landed on 31.
You need to log in before you can comment on or make changes to this bug.