Closed Bug 1353194 Opened 7 years ago Closed 7 years ago

Streamline addon update check at startup for 57

Categories

(Toolkit :: Add-ons Manager, enhancement, P1)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 + verified

People

(Reporter: phlsa, Assigned: aswan)

References

Details

(Whiteboard: [photon-onboarding], triaged)

Attachments

(5 files)

Bug 1353194 Streamline the startup extension compatibility check
59 bytes, text/x-review-board-request
kmag
: review+
Details
CheckAddonCompat57_v2.gif
46.45 KB, image/gif
Details
Screen Shot 2017-09-05 at 12.30.13 PM.png
177.26 KB, image/png
Details
windows screenshot
15.91 KB, image/png
Details
Bug 1353194 Remove a bunch of tests of the old startup update check
59 bytes, text/x-review-board-request
kmag
: review+
Details 
With the full-on switch to webextensions, a lot of older add-ons are going to become incompatible with Firefox 57. That means that the add-on compatibility checker after update will trigger for anyone who has a legacy add-on installed.

Since we want to make onboarding for this version extremely smooth, we should remove or disable the checker for this update and default to disabling legacy add-ons.

If we are concerned about users who don't want to update because they can't lose those add-ons, we could instead offer a download of ESR in the add-ons manager to get those old add-ons working again.

Jeff, any additional thoughts on this?
Flags: needinfo?(jgriffiths)
Component: Installer → Application Update
Product: Firefox → Toolkit
(In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #0)
> With the full-on switch to webextensions, a lot of older add-ons are going
> to become incompatible with Firefox 57. That means that the add-on
> compatibility checker after update will trigger for anyone who has a legacy
> add-on installed.
> 
> Since we want to make onboarding for this version extremely smooth, we
> should remove or disable the checker for this update and default to
> disabling legacy add-ons.

So agreed, we should definitely *not* run the compat checker and instead handle addon compat in the onboarding experience.
 
> If we are concerned about users who don't want to update because they can't
> lose those add-ons, we could instead offer a download of ESR in the add-ons
> manager to get those old add-ons working again.
>
> Jeff, any additional thoughts on this?

I'm not sure what the right thing to do here is, I think we'd need to explore this more as part of onboarding.
Flags: needinfo?(jgriffiths)
The add-ons compatibility check performed by app update was removed in bug 1262880.

The compatibility check this bug refers to happens on application version change (both upgrade and downgrade) and is entirely add-ons mgr code.
Component: Application Update → Add-ons Manager
Whiteboard: [photon]
It's not clear from this bug what the expected outcome is. Philipp, could you clarify what the end result should be? We've been discussing this as part of the add-ons project, and I'd like to understand what the recommended user experience is. While we may not want to draw attention to it on first run, we would like to make sure that users are made aware of options available to them when add-ons are disabled. 

What, specifically, is being considered for removal here?
Flags: needinfo?(philipp)
not commenting on tech/direction of bug - just adding under add-on transition UI bug for tracking
Blocks: 1181835
It's specifically and exclusively about the removal/disabling of this wizard UI:
http://cdn.ghacks.net/wp-content/uploads/2012/07/firefox-compatibility-check.jpg
(sorry, I didn't find a more up-to-date screen shot)

It pops up automatically before Firefox starts up after an update. The user has to go through the process in order to get to the browser.
This is not about the add-on manager in the browser.

I'm not sure if this is the UI that Rob mentioned was removed. I'm fairly sure that I've had it pop up at some point earlier this year and the bug he linked to was fixed almost a year ago...
Flags: needinfo?(philipp) → needinfo?(robert.strong.bugs)
It isn't the same UI. The UI you linked to has the BrandShortName Update string in the UI which is confusing since it isn't just about update and it really is add-ons manager code and showed whenever the application version changed. This happened (I think it still does but add-ons manager devs would know better) during the following conditions on first run for all users whether they performed the update or not as long as they have add-ons:
1. after an app update
2. after installing a newer version of the app
3. after installing an older version of the app
4. using a profile with a different version of the app it was used for previously

2, 3, and 4 could be combined but I wanted to emphasize the conditions since there are slight variations in the details.

The UI is located at
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/update.xul

The bug I fixed is for UI that was shown before downloading an update and not first run and that UI wasn't displayed if the add-ons had updates available.
Flags: needinfo?(robert.strong.bugs)
andy investigating 57 plan for this area
Assignee: nobody → amckay
Priority: -- → P2
Whiteboard: [photon] → [photon], triaged
I think rhelmer is the current owner of this ui so needinfo'ing him so he is aware of this bug.
Flags: needinfo?(rhelmer)
I agree that we don't want this in-your-face UI for legacy add-ons on 57+, since there is no action the user can take to enable them. Since WebExtensions can optionally set strict min/max version, we should consider leaving the UI but not display it for legacy add-ons.

I agree with Jeff in comment 1 that we need an on-boarding strategy - from a technical standpoint I'd suggest leaving add-ons installed but app disabled, so if add-on authors upload a replacement WE for an add-on ID post-57 then it'll start working again. 

This whole flow might be very confusing for users though, some kind of notification might be warranted but I'm not sure what it would be.

I don't think we should promote ESR to users on the release channel in any case.
Flags: needinfo?(rhelmer)
Whiteboard: [photon], triaged → [photon-onboarding], triaged
Blocks: 1351616
No longer blocks: photon-onboarding
Priority: P2 → P3
There is no plan to change the Firefox 57 set of add-ons by removing legacy add-ons. They'll stay around, but be disabled I think we should just remove this dialog by 57.

:aswan we could put this behind the flag from bug 1336576 and then remove it later.
Assignee: amckay → aswan
Blocks: 1336576
After a conversation on IRC, I'm going to try and catch Markus and Michael to talk about this in SF next week and will update this after.
Status: NEW → ASSIGNED
UX design is here:
https://docs.google.com/document/d/1YFF06bEoqH7yUojXKWw9KvK8XyHoIQSuyrPV8NDY5vU/edit
Priority: P3 → P1
Summary: Remove or disable add-on compatibility checker for Firefox 57 → Streamline addon update check at startup for 57
One question for UX and one more general issue.
First, for UX: can we nail down the times in the design document?  The proposals currently are that we should the dialog after 1 second and add the cancel button after 30 seconds.  The first seems reasonable to me but if a user wants to start using the browser right away and skip the update process, 30 seconds seems like a very long time to make them wait to be able to do so.

The broader concern is that I don't think we're going to be able to do much in the way of automated testing of this flow (like we don't appear to have automated testing of the existing update UI that this will replace).  Even manual testing is going to be tedious if an app version change is necessary to trigger this flow.  Ccing :Mossop hoping for clever ideas from him (or anybody else already cc'ed) on this front.
(In reply to Andrew Swan [:aswan] from comment #15)
> The broader concern is that I don't think we're going to be able to do much
> in the way of automated testing of this flow (like we don't appear to have
> automated testing of the existing update UI that this will replace).  Even
> manual testing is going to be tedious if an app version change is necessary
> to trigger this flow.  Ccing :Mossop hoping for clever ideas from him (or
> anybody else already cc'ed) on this front.

We have some basic automated testing, xpcshell tests verify that the dialog gets opened and I think there are some tests of the UI itself in a minimal way.
I would suggest writing the UI in such a way that you pass it the add-on objects for it to operate on, that way in tests you can pass it mock objects instead.
Blocks: 1268708
(In reply to Andrew Swan [:aswan] from comment #15)
> First, for UX: can we nail down the times in the design document?  The
> proposals currently are that we should the dialog after 1 second and add the
> cancel button after 30 seconds.  The first seems reasonable to me but if a
> user wants to start using the browser right away and skip the update
> process, 30 seconds seems like a very long time to make them wait to be able
> to do so.

Looking at research done around tolerable wait time, studies seam to vary widely, but it seams to be somewhere between 12-30s could be acceptable if progress is provided.

As this is a one time process and we risk significantly worsening the users experience if we can not updated the extensions, I think we should take 30seconds to ensure success before we offer an alternative route. (as I expect users would take that route not knowing all the consequences.)

Note: for most people this will take considerably less time than 30s and to them we can provide a much cleaner UI that they are already used to from Firefox updates. Show showing the cancel to early will risk worsening their experience too.
I'm not sure if we've actually gotten sign-off on the copy here.  The strings in the current patch are:

addonUpdateHeader=%S Update
addonUpdateMessage=%S is updating your extensions and will start in a few moments...
addonUpdateCancelMessage=This will take a little longer than expected
addonUpdateCancelButton=Stop update and launch %S

In all cases, %S is replaced with the name of the browser (ie, typically Firefox, possibly Nightly).
Michelle, can you review and sign off on those strings?

Also, the layout of the dialog is going to depend a bunch of the length of the localized versions of these strings and unfortunately its going to be very difficult for translators to see some of them (given the 30 second requirement discussed above).  Not sure what to do about that...
Flags: needinfo?(mheubusch)
:aswan,

So I tried,

* Installed Adblock Plus on my profile
* Opened the same profile using one of the test builds from comment 21

I was expecting to see the compatibility checker but the browser just loaded instantly. 

Is that expected?
ABP isn't a webextension so all that will happen at startup is a check to find out that there is no compatible update.  That should happen very fast.
See the doc linked in comment 14 for a more thorough explanation of when the dialog appears.
Also, you'll need to do the initial install with a pre-57 build, we don't do any extra startup checks if the version number doesn't change.
Adding tracking flags for this one, since it was suggested in at least one of the duplicate bugs.
status-firefox57: --- → affected
tracking-firefox57: --- → +
(In reply to Andrew Swan [:aswan] from comment #20)
> Also, the layout of the dialog is going to depend a bunch of the length of
> the localized versions of these strings and unfortunately its going to be
> very difficult for translators to see some of them (given the 30 second
> requirement discussed above).  Not sure what to do about that...

If having 2 lines of text doesn't break the ui, I think it should not be a problem.
I am hoping with Michelle's help we might get a shorter string on the button.
Final strings:

addonUpdateHeader=%S Update
addonUpdateMessage=%S is updating your extensions  . . . 
addonUpdateCancelMessage=Still updating. Want to wait? 
addonUpdateCancelButton=Stop update and launch %S
Flags: needinfo?(mheubusch)
Design with updated strings.
You've got different text in different sizes, can you give me the exact styles each element should use?
Flags: needinfo?(mjaritz)
(In reply to Andrew Swan [:aswan] from comment #30)
> You've got different text in different sizes, can you give me the exact
> styles each element should use?

Hi Andrew,
please use the same styles as the Firefox Update and the current dialog are using:
The title bar style should be the same as the Firefox Updater,
the body style should be the same as the Firefox Updater,
the buttons style should be the same as in the current add-on compatibility checker,
the 30s message next to the button should use the same style as the button.
Flags: needinfo?(mjaritz)
Depends on: 1396578
The updater is a native application, it uses native APIs to display the update dialog and from what I can tell, it doesn't apply any particular styles, is just uses underlying platform defaults for fonts etc.  Trying to track down exact font details and other styling information to match them in this update dialog sounds like a wild goose chase that would put this (plus a bunch of other work assigned to me) at significant risk for 57.
I've attached a screenshot of my most recent work-in-progress and I'll push the patches in a moment.  Can we do at most one more iteration on the visuals and then just land this?
Flags: needinfo?(mjaritz)
(In reply to Andrew Swan [:aswan] from comment #35)
> The updater is a native application, it uses native APIs to display the
> update dialog and from what I can tell, it doesn't apply any particular
> styles, is just uses underlying platform defaults for fonts etc.  Trying to
> track down exact font details and other styling information to match them in
> this update dialog sounds like a wild goose chase that would put this (plus
> a bunch of other work assigned to me) at significant risk for 57.

I assumed you would be using the same window / native APIs to build this. 
If not, I guess we have to live with what is possible in your environment.

> I've attached a screenshot of my most recent work-in-progress and I'll push
> the patches in a moment.  Can we do at most one more iteration on the
> visuals and then just land this?
Please move the "Nightly Update" string into the title bar of the window.
Please make the window height fit the content. (Same spacing then left and right)
And can you please share a screenshot from Windows, to make sure it looks ok there as well. 
Most people will see this on windows.

With that we should be good.
Flags: needinfo?(mjaritz)
Attached image windows screenshot — 

    
    

    
  
Slightly out of date windows screenshot attached (I changed the styles to force the "Still updating" message be vertically aligned with the button since the screenshot was taken).

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8893991 -
        Flags: review?(kmaglione+bmo)
Flags: qe-verify+
QA Contact: jwilliams

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #39)
> Slightly out of date windows screenshot attached (I changed the styles to
> force the "Still updating" message be vertically aligned with the button
> since the screenshot was taken).

Please also align the button and the progress bar to end at the same x position and add some space below the button, so that all sides have equal spacing to the sides of the window. Thanks.

    
    

    
      
  

    
    

    
  
Comment on attachment 8893991 [details]
Bug 1353194 Streamline the startup extension compatibility check

https://reviewboard.mozilla.org/r/165062/#review182524

::: toolkit/mozapps/extensions/content/update.html:3
(Diff revision 7)
> +<!DOCTYPE html>
> +<html>
> +  <head>

Nit: `<meta charset="utf-8">`

::: toolkit/mozapps/extensions/content/update.html:5
(Diff revision 7)
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <script src="update.js"></script>
> +    <link rel="stylesheet"  href="chrome://mozapps/content/extensions/update.css"/>

Nit: No `/>` in HTML, only XHTML.

::: toolkit/mozapps/extensions/content/update.js:1
(Diff revision 7)
> -// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
> +Components.utils.import("resource://gre/modules/Services.jsm");

Nit: "use strict";

::: toolkit/mozapps/extensions/content/update.js:21
(Diff revision 7)
> -// timeout (in milliseconds) to wait for response to the metadata ping
> -const METADATA_TIMEOUT    = 30000;
> -
> -Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "AddonManagerPrivate", "resource://gre/modules/AddonManager.jsm");
> +
> +window.addEventListener("DOMContentLoaded", e => {
> +  document.getElementById("message").textContent = messageText;
> +  document.getElementById("cancel-message").textContent = cancelText;
> +  document.getElementById("cancel-btn").textContent = cancelButtonText;
> +  window.sizeToContent();

Nit: This technically shouldn't be called before "load", or it may hapen before styles are applied.

::: toolkit/mozapps/extensions/content/update.js:23
(Diff revision 7)
> -
> -Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "AddonManagerPrivate", "resource://gre/modules/AddonManager.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "AddonRepository", "resource://gre/modules/addons/AddonRepository.jsm");
> +  document.getElementById("message").textContent = messageText;
> +  document.getElementById("cancel-message").textContent = cancelText;
> +  document.getElementById("cancel-btn").textContent = cancelButtonText;
> +  window.sizeToContent();
> +
> +  let observer = new MutationObserver(() => window.sizeToContent());

This... is going to be *very* expensive if you make multiple changes the same time. Can you please do it explicitly after you make changes you expect to change the size, instead?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:783
(Diff revision 7)
> + *
> + * @param {Addon} addon The addon to check
> + *
> + * @returns {boolean} Whether the addon should be disabled for being legacy
> + */
> +function isDisabledLegacy(addon) {

Thank you.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2182
(Diff revision 7)
> +          for (let id of started) {
> +            startedAddonIDs.add(id);
> +          }

Seems like `startedAddonIDs = new Set(started)` would work just as well, but *shrug*

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2216
(Diff revision 7)
> +          if (startedAddonIDs.has(addon.id)) {
> +            continue;
> +          }

Can we store whether the add-on has started in `activeAddons` instead? This seems a bit shaky... Probably just checking whether it has an `activeAddons` entry should be enough.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2439
(Diff revision 7)
>     * @returns False if no update check is needed, otherwise an array of add-on
> -   *          IDs to check for updates. Array may be empty if no add-ons can be/need
> +   *          IDs to check for updates.

This is so stupid...

Anyway, if we're going to keep this weird signature, can you add `{string[]|bool}` to the @returns tag?

But... really, can we just fix this to have a sensible name and signature, and return `null` rather than `false` when no update check is needed?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2471
(Diff revision 7)
> -   *
> -   * @param  aAddonIDs
> +   * This runs during startup when some addons have become disabled but
> +   * legacy extensions are disabled.  In this case, we just do a quiet

This is a bit hard to parse. Maybe something like "when an app update has made some add-ons incompatible, and legacy add-on support is disabled"?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2475
(Diff revision 7)
> -   * Shows the "Compatibility Updates" UI.
> -   *
> -   * @param  aAddonIDs
> -   *         Array opf addon IDs that were disabled by the application update, and
> -   *         should therefore be checked for updates.
> -   */
> +   * Perform startup check for updates of legacy extensions.
> +   * This runs during startup when some addons have become disabled but
> +   * legacy extensions are disabled.  In this case, we just do a quiet
> +   * update check.
> +   *
> +   * @param {array<string>} ids The ids of the addons to check for updates.

Nit: `Array<string>` or `string[]`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2477
(Diff revision 7)
> -   * @param  aAddonIDs
> -   *         Array opf addon IDs that were disabled by the application update, and
> -   *         should therefore be checked for updates.
> -   */
> -  showUpgradeUI(aAddonIDs) {
> -    logger.debug("XPI_showUpgradeUI: " + aAddonIDs.toSource());
> +   * legacy extensions are disabled.  In this case, we just do a quiet
> +   * update check.
> +   *
> +   * @param {array<string>} ids The ids of the addons to check for updates.
> +   *
> +   * @returns {set<string>} The ids of any addons that were updated.  These

Nit: `Set<string>`. Sets aren't a primitive type, so there's no lower-case variant.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2492
(Diff revision 7)
> +    let progressByID = new Map();
> +    function setProgress(id, val) {
> +      progressByID.set(id, val);
> +      updateProgress(Array.from(progressByID.values()).reduce((a, b) => a + b) / progressByID.size);
> +    }

If you wanted to be clever, you could do something like:

    let progressByID = new class extends Map {
      set(id, value) {
        super.set(id, value);
        this.updateProgress(Array.from(this.values(), ...));
      }
    };

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2502
(Diff revision 7)
> +
> +    // Do an update check for one addon and try to apply the update if
> +    // there is one.  Progress for the check is arbitrarily defined as
> +    // 10% done when the update check is done, between 10-90% during the
> +    // download, then 100% when the update has been installed.
> +    async function checkOne(id) {

Nit: Can we make this an arrow function to avoid footguns?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2544
(Diff revision 7)
> +            finish();
> +          },
> +        });
> +      });
> +      install.install();
> +      return installPromise; // eslint-disable-line consistent-return

Nit: Can get rid of the eslint override with `await installPromise` or `return undefined` for the other return.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2572
(Diff revision 7)
> +      timer = setTimeout(() => {
> +        cancelDiv.removeAttribute("style");
> +      }, SHOW_CANCEL_DELAY - SHOW_DIALOG_DELAY);
> +    }, SHOW_DIALOG_DELAY);
> +
> +    Services.tm.spinEventLoopUntil(() => finished);

Well, that's gross... but what can you do.

        Attachment #8893991 -
        Flags: review?(kmaglione+bmo) → review+

    
  
Depends on: 1398217

    
    

    
      
  

    
    

    
  
Comment on attachment 8893991 [details]
Bug 1353194 Streamline the startup extension compatibility check

https://reviewboard.mozilla.org/r/165062/#review182524

> Can we store whether the add-on has started in `activeAddons` instead? This seems a bit shaky... Probably just checking whether it has an `activeAddons` entry should be enough.

Just checking whether it has an `activeAddons` entry isn't sufficient since there are code paths that call the install method before startup (I know you've removed some of those but it still happens for system addons and it happens extensively in tests).  But adding a new property in the `activeAddons` to indicate whether the addon has been started or not ought to work, I'll try that in a bit.

    
    

    
      
  


  

    
    

    
      
  

    
  

        Attachment #8906407 -
        Flags: review?(kmaglione+bmo)

    
    

    
  
Comment on attachment 8893991 [details]
Bug 1353194 Streamline the startup extension compatibility check

https://reviewboard.mozilla.org/r/165062/#review183044

lgtm

    
    

    
  
Comment on attachment 8906407 [details]
Bug 1353194 Remove a bunch of tests of the old startup update check

https://reviewboard.mozilla.org/r/178112/#review183046

\o/

        Attachment #8906407 -
        Flags: review?(kmaglione+bmo) → review+

    
    

    
  
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3c293ceedf3
Remove a bunch of tests of the old startup update check r=kmag
https://hg.mozilla.org/integration/autoland/rev/246b6aae8157
Streamline the startup extension compatibility check r=kmag

    
    

    
  
(In reply to Markus Jaritz [:designakt] (UX) from comment #42)
> Please also align the button and the progress bar to end at the same x
> position and add some space below the button, so that all sides have equal
> spacing to the sides of the window. Thanks.

I fixed the top-bottom spacing but the progress bar (on Windows only) is a mess and I simply don't have the time to fuss with it right now.  Feel free to file a follow-up and perhaps somebody can work on it during the beta cycle.  The short of it is that native Windows progress bars appear to get a border by default which throws off all the spacings.  Removing the border causes the progress bar to be displayed as a simple blue rectangle instead of a native widget.

    
    

    
      
  

    
    

    
  
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a70f67c7c65
Remove a bunch of tests of the old startup update check r=kmag
https://hg.mozilla.org/integration/autoland/rev/005cddf9e020
Streamline the startup extension compatibility check r=kmag
Depends on: 1399092

    
    

    
  
I have verified that this issue has been fixed on today's nightly.
Status: RESOLVED → VERIFIED

    
    

    
  
I can confirm this issue is not reproducible on Fx 57.0b8.

          status-firefox57:
          fixed → verified
Flags: qe-verify+

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: