599290 - Update SoftwareUpdateAPI to support complete relocation to the about dialog

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With the landing of bug 596813 the complete software update interaction is now handled in the about dialog. That requires one more round of updates for our Mozmill tests. I'm unlikely be able to do this in the next days. But lets see.

What has to be done:

1. To maintain the ability to execute update checks for older beta versions we should have two separate versions of the API. We should decide on the buildid which version has to be used. Once we have released Firefox 4.0 we can remove the orphaned API.

2. Make sure what has to be updated for getting elements and interacting with the update steps. Robert, could you give me a quick overview about the differences between the update dialog and the new ui?

I hope that there is not that much work to do and we still have a chance to get it implemented before Beta 7. Otherwise we will have to run update tests manually for Beta 7. :/

Comment 1

[Robert Strong (they/them - no direct email)]

I am going to work on mochitests for the UI which will cover all of the use cases besides update being disabled administratively and the manual update case which required that the user doesn't have write access.

The steps for the most part are defined by beltzner in bug 596813 comment #18 with the exception I stated in bug 596813 comment #51. The element id's can be seen in the final patch in bug 596813.

You can still test with the normal update ui with the following code which will check for updates and this will test the one thing we can't test on the tinderbox which is the update servers and the update snippets and mars which are provided by releng.

Components.classes["@mozilla.org/updates/update-prompt;1"].createInstance(Components.interfaces.nsIUpdatePrompt).checkForUpdates();

Since the mochitests can test the ui it would probably be a better use of time to just use the code snippet above. To test that locales haven't messed up a string you can disable the automatic update check, open the about dialog, and verify there are no dtd errors.

Comment 2

[Robert Strong (they/them - no direct email)]

btw: the about dialog will also widen if a locale has a string longer than the space available. It might be a good thing to test the about dialog's width to check for this.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Robert. For how long will the old updates ui remain in the product? 

(In reply to comment #2)
> btw: the about dialog will also widen if a locale has a string longer than the
> space available. It might be a good thing to test the about dialog's width to
> check for this.

Any l10n aspect is not part of this API but will hopefully be implemented soon by the work on bug 562084. So far I only have to make sure we can execute our update tests. As long as the above method works I will use it as a workaround for now.

Comment 4

[Robert Strong (they/them - no direct email)]

(In reply to comment #3)
> Thanks Robert. For how long will the old updates ui remain in the product? 
There are no plans to remove it and the flow beltzner describes in bug 596813 comment #18 actually requires it.

> (In reply to comment #2)
> > btw: the about dialog will also widen if a locale has a string longer than the
> > space available. It might be a good thing to test the about dialog's width to
> > check for this.
> 
> Any l10n aspect is not part of this API but will hopefully be implemented soon
> by the work on bug 562084. So far I only have to make sure we can execute our
> update tests. As long as the above method works I will use it as a workaround
> for now.
It had better work and it definitely works for me. :)

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Lets try to get the workaround landed today...

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Temporary workaround [checked-in]

This patch makes sure that we use the old update ui also on Windows and Linux.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I will wait with the complete reorganization until we have a decision on bug 599945.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 478849 [details] [diff] [review]
Temporary workaround [checked-in]

Landed temporary fix on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/f5c1b50d1c5d

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Now that bug 599480 is fixed, we only support the update check inside the about window on all platforms. We should update our shared module and find a way to safely integrate it into our repository, so that we are still able to update older beta versions for release testing.

Comment 10

[Robert Strong (they/them - no direct email)]

btw: after Firefox 4.0 toolkit -> app update will provide a generic widget for this that can be included in any app provided ui for checking for updates (bug 599574).

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks for this information. I will follow the bug. Once the current shared module has been updated, it should be easy to adapt it to the upcoming update XBL binding.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bustage fix for OS X [checked-in]

Another necessary fix to re-enable updates on OS X due to the relocation into the about dialog. The menu entry isn't available anymore.

Comment 13

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 490504 [details] [diff] [review]
Bustage fix for OS X [checked-in]

Henrik, change itself looks fine syntactically but the logic no longer matches the comment above. The new behavior will look in the about box for updates on any platform; the comment says it only does this for windows/linux.

I'm also a little confused because my Mac version of 4.0b7 doesn't have update in the about window and does have the separate menu option, so this code wouldn't work on it. Did you mean to be checking for 4.0b8pre on Mac instead?

Please clarify the logic/version question here. Also, please update the comment for truth. With those changes (and in the interest of a cleaner test run tomorrow) r=me.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #13)
> I'm also a little confused because my Mac version of 4.0b7 doesn't have update
> in the about window and does have the separate menu option, so this code
> wouldn't work on it. Did you mean to be checking for 4.0b8pre on Mac instead?

Your Mac version of 4.0b7 definitely has the updater in the about window. Before bug 599480 has been fixed we simply had two different paths in getting an update. This patch has been proven to work with 4.0b7 and older 4.0b builds.

Updated the comments as mentioned before the check-in:
http://hg.mozilla.org/qa/mozmill-tests/rev/8651f67b5030

Comment 15

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Must only appear if there's an update available then--the about dialog on mine is updater-free right now. :) Thanks for the clarification and the change.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #15)
> Must only appear if there's an update available then--the about dialog on mine
> is updater-free right now. :) Thanks for the clarification and the change.

Then it can't be a real b7 build. Bug 596813 has been landed for b7 on all platforms.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks like we have to take action on this bug ASAP. As what I have seen while working on bug 604370 we fail in downloading the partial update, because the about window starts the download and at the same time the old ui, which we are still using, does the same. As the reason the partial update is marked as failed and a complete update gets downloaded. :(

Comment 18

[Robert Strong (they/them - no direct email)]

You should be able to use the code in comment #1 without opening the about window to test downloading / applying / etc.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Don't open about dialog to prevent failing partial updates [checked-in]

That's indeed a workaround but now we don't test anything of the new ui. This patch will fix it temporarily but I will have to update the whole shared module as soon as I can.

Comment 20

[Robert Strong (they/them - no direct email)]

Bug 599233 should cover that.

Comment 21

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 491147 [details] [diff] [review]
Don't open about dialog to prevent failing partial updates [checked-in]

Looks OK. r+. 

Do we have a bug where we can remind ourselves to flip it back later? I don't want the dead commented-out code to live forever unmaintained.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #21)
> Do we have a bug where we can remind ourselves to flip it back later? I don't
> want the dead commented-out code to live forever unmaintained.

Yes, we have bug 599290 for exactly this purpose. :) Once I'm able to update the shared module this code will completely go away.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 491147 [details] [diff] [review]
Don't open about dialog to prevent failing partial updates [checked-in]

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/6585aa4bca81

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Not working on this bug right now until the API refactor has been finished.

Comment 26

[Daniela Petrovici]

Attachment: patch v1.0

This is not finished yet, but I wanted to get the feedback before continuing to see if I am on the right track

I have changed the testDirectUpdate tests to use the about-dialog library, but there are still items that need to be added there, too.

Regarding the testFallbackUpdate tests, I am not sure how to force the fallback using the new UI. Should we just use the old library from software-update.js -> forceFallback() method?

Comment 27

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 727662 [details] [diff] [review]
patch v1.0

If a patch is not ready please only ask for review. I would like Henrik's thoughts on this, but I think it would be better to create a ui lib for the about dialog, and update the software-update library to use that.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Also see bug 600500 for upcoming changes. Not sure yet when it will land. So we should not consider it for inclusion in this bug but more as a follow-up.

(In reply to Dave Hunt (:davehunt) from comment #27)
> If a patch is not ready please only ask for review. I would like 

I'm sure Dave wanted to say info-needed or feedback by a code-check here. Otherwise I agree to Dave's comment. I will have a look at it soon.

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #28)
> Also see bug 600500 for upcoming changes. Not sure yet when it will land. So
> we should not consider it for inclusion in this bug but more as a follow-up.
> 
> (In reply to Dave Hunt (:davehunt) from comment #27)
> > If a patch is not ready please only ask for review. I would like 
> 
> I'm sure Dave wanted to say info-needed or feedback by a code-check here.
> Otherwise I agree to Dave's comment. I will have a look at it soon.

Indeed :)

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 727662 [details] [diff] [review]
patch v1.0

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

As we have pointed out earlier this has to be split-up into different modules. I thought it was clear when we talked about it in our ask an expert meeting and I clearly expressed that. So please split up into an ui and backend module.

::: lib/about-dialog.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include required modules
> +var observerService = Cc["@mozilla.org/observer-service;1"].
> +                      getService(Ci.nsIObserverService);

You will have to make use of the services.jsm module here.

::: tests/update/testDirectUpdate/test3.js
@@ -35,5 @@
> -
> -    var lastUpdateType = persisted.updates[persisted.updateIndex].type;
> -    expect.notEqual(update.updateType, lastUpdateType,
> -                    "No more update of the same type offered.");
> -  }

Why the removal of all those lines? Those are important and have to be kept.

Comment 31

[Daniela Petrovici]

Attachment: patch v2.0

Modified patch so that we now have a UI library: about-dialog.js and the software-update.js library is the backend module that is using the UI library. 

I have tested by updating from Nightly 20.0a1 (with incompatible addons) to Nightly 24.0a1 and from Nightly 24.0a1 from 13th of May (en-US and different locales) and it is working on MAC, Windows and Linux.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 750407 [details] [diff] [review]
patch v2.0

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

Thank you Daniela for the next version of the patch. I went through all files and made my comments inline. In general those changes suffer from a big problem. You bind the about window below the software-update module. That should not be the case. The about window is totally isolated from any other ui interaction of the old software update window. Most likely we need a real backend module for helpers and the interaction with the update service.

I wish that we would have agreed on that before you started the work on this bug. I think in the future it will be good if the assignee could tell us how he/she would implement something. Starting do directly work on a patch is not often the best idea. With that we could save us a lot of time.

::: lib/about-dialog.js
@@ +10,5 @@
> +const STATE_DOWNLOADING = "Downloading";
> +const STATE_DOWNLOAD_FAILED = "DownloadFailed";
> +const STATE_INITIAL = "Initial";
> +const STATE_MANUAL_UPDATE = "ManualUpdate";
> +const STATE_NO_UPDATES_FOUND = "NoUpdatesFound";

Why are those constants defined twice, in this module and the software-update.js one?

@@ +12,5 @@
> +const STATE_INITIAL = "Initial";
> +const STATE_MANUAL_UPDATE = "ManualUpdate";
> +const STATE_NO_UPDATES_FOUND = "NoUpdatesFound";
> +
> +function aboutDialog() {

Allow to pass in a controller here which can be directly used for the window.

@@ +14,5 @@
> +const STATE_NO_UPDATES_FOUND = "NoUpdatesFound";
> +
> +function aboutDialog() {
> +  this._controller = null;
> +  this._mainController = null;

Why do we need the second controller?

@@ +40,5 @@
> +    var indexTotalToDownload = downloadStatusValue.lastIndexOf(" ");
> +    var percentageDownloaded = downloadStatusValue.substring(0, indexDownloaded);
> +    var sizeToDownload = downloadStatusValue.substring(indexDownloaded+2, indexTotalToDownload);
> +
> +    var progress = sizeToDownload - percentageDownloaded;

Is there no sub node exists anymore which could be used to directly retrieve the current progress?

@@ +89,5 @@
> +      case "7":
> +        state = STATE_NO_UPDATES_FOUND;
> +        break;
> +      case "8":
> +        state = STATE_MANUAL_UPDATE;

I think we should make use of a dictionary here so that we can safe us from this switch statement:

var STATE = { 1: 'foo', 2: 'bar' };

@@ +165,5 @@
> +   */
> +  open : function aboutDialog_open(aController) {
> +    this._mainController = aController;
> +    aController.mainMenu.click("#aboutName");
> +    this._controller = utils.handleWindow("type", "Browser:About", undefined, false);

I would move this into the test itself and let the whole about window be handled by the handleWindow method. That way you would not have to duplicate the close() code.

::: lib/software-update.js
@@ +10,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  // Include required modules
>  var { assert, expect } = require("assertions");
> +var aboutDialog = require("about-dialog");

A backend module should never include an ui module. It should always happen the other way around.

@@ +25,5 @@
> +const STATE_INITIAL = "Initial";
> +const STATE_MANUAL_UPDATE = "ManualUpdate";
> +const STATE_NO_UPDATES_FOUND = "NoUpdatesFound";
> +
> +const TIMEOUT_UPDATE_APPLYING  = 120000;

Why have you changed the timeout here?

@@ -197,5 @@
> -//      var patch1URL = this.activeUpdate.getPatchAt(1).URL;
> -       // Test that the update snippet created by releng doesn't have the same
> -       // url for both patches (bug 514040).
> -//      assert.notEqual(patch0URL, patch1URL, "Update snippets have different URLs");
> -//    }

Why have those lines been removed?

@@ +276,5 @@
>    /**
>     * Check if updates have been found
>     */
>    get updatesFound() {
> +    return this._aboutDialog.updateState !== STATE_NO_UPDATES_FOUND;

As said above, wrong dependency.

@@ +324,5 @@
> +  checkAboutDialog : function softwareUpdate_checkAboutDialog(aController) {
> +    this._aboutDialog.open(aController);
> +    var button = this._aboutDialog.getElement({type:"updateButton"});
> +    expect.ok(!button.getNode().hidden,
> +              "The update button is always visible even after an update.");

Same here and also for other places I don't want to mention it again.

Comment 33

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #32)
> I wish that we would have agreed on that before you started the work on this
> bug. I think in the future it will be good if the assignee could tell us how
> he/she would implement something. Starting do directly work on a patch is
> not often the best idea. With that we could save us a lot of time.
Implementation is below. Please tell me if this is ok with you:
- keep software-update.js library as a backend module
- about-dialog.js would be the UI library added in lib/ui
- have a get softwareUpdate() method that will return the softwareUpdate object so that we can call the methods that use the update service directly in the tests. It would be:
update = new aboutDialog.aboutDialog();
and use as: update.softwareUpdate.allowed

Although I think that it would be better to create wrappers for these methods in the about-dialog.js so that the tests use only the UI library.
- keep the old UI separate from the new UI. Methods that use the old UI (like downloading when there are incompatible addons) would still be added in the software-update while methods using the new UI will be in the about-dialog.js file.

Comment 34

[Daniela Petrovici]

Forgot to add the need info flag. Please let me know if the above implementation would be ok.

Comment 35

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'd like to defer to Henrik for feedback on the implementation.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Daniela Petrovici from comment #33)
> - keep software-update.js library as a backend module

That's fine if we only keep that stuff in the file which operates on the API and file system level. Which methods and variables will remain?

> - about-dialog.js would be the UI library added in lib/ui

Right. But I would like to see more details about what's needed here. That's way too less implementation details. Also what about the old dialog? Where will this code be placed in?

> - have a get softwareUpdate() method that will return the softwareUpdate
> object so that we can call the methods that use the update service directly
> in the tests. It would be:
> update = new aboutDialog.aboutDialog();
> and use as: update.softwareUpdate.allowed

Where is that method places? Sorry, but I miss the connection here.

> Although I think that it would be better to create wrappers for these
> methods in the about-dialog.js so that the tests use only the UI library.

Which methods are you referring here to?

> - keep the old UI separate from the new UI. Methods that use the old UI
> (like downloading when there are incompatible addons) would still be added
> in the software-update while methods using the new UI will be in the
> about-dialog.js file.

As mentioned above, we need two and not only one ui module.

Comment 37

[Daniela Petrovici]

I will take out from software-update the UI methods and create two UI libraries: about-dialog.js containing methods for the new UI and wizard-pages.js – containing methods for the old UI

The variables that will remain in software-update.js are the ones used by the methods in there (please see below).

In *about-dialog.js* we will have:
- New methods added in about-dialog.js: get downloadProgress, getElements, get updateState, get softwareUpdate – the latter is a wrapper for the backend module.
- Moved and modified from software-update.js: openDialog, waitForDownloadFinished, updatesFound, checkAboutDialog, closeDialog, download, waitForCheckFinished

 In *wizard-page.js* we will have:
- Moved from software-update.js: waitForWizardPage, currentPage, closeDialog, download – for when the old UI is used (e.g. incompatible addons are present), getElement, waitForCheckFinished, waitForDialogOpen (since we need this for the fallback tests and it uses the old UI), waitForDownloadFinished (in case of incompatible addons), WIZARD_PAGES

*Remaining methods in software-update.js*
- Methods that calls API: get ABI, get activeUpdate, get allowed, get buildInfo, get channel, get isCompleteUpdate, get patchInfo, get OSVersion, get stagingDirectory, get updateType, assertUpdateApplied, forceFallback, getDtds, getUpdateURL.

The tests will be changed to make use of both UI libraries.

Comment 38

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Henrik is on PTO until the 8th July. This sounds like a reasonable approach to me.

Comment 39

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

Made changes per previous comment.

(In reply to Henrik Skupin (:whimboo) from comment #32)
> Comment on attachment 750407 [details] [diff] [review]
> patch v2.0
> Is there no sub node exists anymore which could be used to directly retrieve
> the current progress?
We were not using this method / need it so I have removed it from the library.

Comment 41

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.1

I updated my patch so it will still apply after latest changes and I bring new reports:

Reports
http://mozmill-crowd.blargon7.com/#/update/report/1039ea48a9d69a5a1cc4fd228c8551a2
http://mozmill-crowd.blargon7.com/#/update/report/1039ea48a9d69a5a1cc4fd228c856f69
http://mozmill-crowd.blargon7.com/#/update/report/1039ea48a9d69a5a1cc4fd228c7bd49d
http://mozmill-crowd.blargon7.com/#/update/report/1039ea48a9d69a5a1cc4fd228c7bcc2d
http://mozmill-crowd.blargon7.com/#/update/report/1039ea48a9d69a5a1cc4fd228c7ba2fe
http://mozmill-crowd.blargon7.com/#/update/report/1039ea48a9d69a5a1cc4fd228c7ba112

Comment 42

[Andrei Eftimie]

Comment on attachment 807722 [details] [diff] [review]
patch v3.1

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

We still have some things to address here.

I'm still taking in all architecture changes, overall it seems ok, but I will need more time to digest it properly.
I might come back with feedback regarding that.

Until then, there are some things to address.

::: lib/software-update.js
@@ +240,5 @@
>      updateStatus.append("update.status");
>  
> +    // It takes a long time to apply the update and a situation might arise where
> +    // "applied" status overwrites the "failed: 6" string
> +    assert.waitFor(function () {

nit: space between function and brackets

@@ +242,5 @@
> +    // It takes a long time to apply the update and a situation might arise where
> +    // "applied" status overwrites the "failed: 6" string
> +    assert.waitFor(function () {
> +        return this.activeUpdate !== null && this.activeUpdate.state.contains("applied");
> +      }, "The update has been applied", TIMEOUT_UPDATE_APPLYING, undefined, this);

Can we bind this to self prior to this call and drop the last 2 parameters

::: lib/ui/about-dialog.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var observerService = Cc["@mozilla.org/observer-service;1"].
> +                      getService(Ci.nsIObserverService);

We can use Services.jsm for the observerService

@@ +22,5 @@
> +  "manualUpdate",
> +  "unsupportedSystem"
> +];
> +
> +

nit: 2 new lines

@@ +63,5 @@
> +   *
> +   * @param {MozMillController} aController
> +   *        Controller of the parent window from which we open the about dialog
> +   */
> +  checkAboutDialog : function softwareUpdate_checkAboutDialog(aController) {

Shouldn't we use either:

1) use the methods we already have to open and close the aboutDialog:
this.open();
[check the button];
this.close();

or 

2) Handle the opening and closing in the test itself, and only do the [check the button] code in this method.

At the moment this is half-baked, we open the AboutDialog regardless of its state, and we don't close it afterwards.

In how we handle the tests themselves, I'm inclined to go with 2)

@@ +127,5 @@
> +   * Download the update of the given channel and type
> +   *
> +   * @param {String} aChannel
> +   *        Update channel to use
> +   * @param {Boolean} aWaitForFinish

Mark optional parameters in JSDOC, and set their default values.
Please also check other JSDOC comments.

@@ +199,5 @@
> +   *
> +   * @param {MozMillController} aController
> +   *        Controller of the parent window
> +   */
> +  open: function aboutDialog_open(aController) {

I think we might also want to include the opening action in this method:
> controller.mainMenu.click("#aboutName");

Since:
> aboutDialog.open(controller); 

Implies an action, not just an observer.

@@ +210,5 @@
> +   *
> +   *  @returns {Boolean}
> +   *           True if updates were found
> +   */
> +  updatesFound: function aboutDialog_updateFound() {

We might make this a getter as it initially was

::: lib/ui/wizard-pages.js
@@ +216,5 @@
> +   *
> +   * @param {number} timeout
> +   */
> +  waitForCheckFinished : function softwareUpdate_waitForCheckFinished(aTimeout) {
> +    var timeout = aTimeout ? aTimeout : TIMEOUT_UPDATE_CHECK;

This will fail with a timeout of 0 (but then again, we will probably never want a zero timeout).

However this can be shorter:
> var timeout = aTimeout || TIMEOUT_UPDATE_CHECK;

There are multiple instances like this.

@@ +220,5 @@
> +    var timeout = aTimeout ? aTimeout : TIMEOUT_UPDATE_CHECK;
> +
> +    assert.waitFor(function() {
> +      return this.currentPage != WIZARD_PAGES.checking;
> +    }, "Check for updates has been completed.", timeout, null, this);

Please reference this as self before the function and take out the last 2 parameters.

@@ +232,5 @@
> +    this._wizard = this.getElement({type: "wizard"});
> +
> +    assert.waitFor(function () {
> +      return this.currentPage !== WIZARD_PAGES.dummy;
> +    }, "Dummy wizard page has been made invisible", undefined, undefined, this);

Same thing with this and self

::: tests/update/testFallbackUpdate/test2.js
@@ +34,2 @@
>  
> +  aboutDialog.checkAboutDialog(controller);

nit: leave a newline after this check

@@ +36,2 @@
>    // Open the software update dialog and wait until the check has been finished
> +  controller.mainMenu.click("#aboutName");

I don't think we need this here, we already call it in aboutDialog.checkAboutDialog() and we don't close it there.

@@ +43,2 @@
>  
> +  assert.waitFor( function () {

nit: extra spaces

::: tests/update/testFallbackUpdate/test4.js
@@ +48,1 @@
>                      "No more update of the same type offered.");

I think we can improve the wording here a bit:
"No other update [...]"

Comment 43

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: p

Comment 44

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.2

Hi Andrei, I fixed the what you asked.

Comment 45

[Andrei Eftimie]

Comment on attachment 816969 [details] [diff] [review]
patch v3.2

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

::: lib/ui/about-dialog.js
@@ +70,5 @@
> +   *
> +   * @param {MozMillController} aController
> +   *        Controller of the parent window from which we open the about dialog
> +   */
> +  checkAboutDialog : function softwareUpdate_checkAboutDialog(aController) {

this should be aboutDialog_checkAboutDialog.

I'm wondering if we should also change the name... we check that the updateButton is not hidden.
Should this function be called checkAboutDialogUpdateButton ?

@@ +76,5 @@
> +    utils.handleWindow("type", "Browser:About", function (aController) {
> +      var button = new elementslib.Selector(aController.window.document, "#updateButton");
> +      expect.ok(!button.getNode().hidden,
> +                "The update button is always visible even after an update.");
> +    }, true);

Lets make this method more simple and pragmatic and DRY.

We can require the aboutDialog to be opened, and this method just check that the button is visible.
Just call this method in the tests after we call open()

This way we don't need the aController, and we can get the button with this.getElements

@@ +84,5 @@
> +   * Checks if there is an "Check For Updates" button and then click on it
> +   */
> +  checkForUpdates: function aboutDialog_checkForUpdates() {
> +    var updateBtn = this.getElements({type: "updateButton"});
> +    assert.ok(this._controller, "About Dialog has been opened");

This check should go before getting the button, that call will fail if we don't have the dialog open

@@ +141,5 @@
> +   * @param {Number} aTimeout
> +   *        Timeout the download has to stop
> +   *        [optional - default: 600s]
> +   */
> +  download: function aboutDialogDownload(aChannel, aWaitForFinish, aTimeout) {

The name should probably be aboutDialog_download

@@ +142,5 @@
> +   *        Timeout the download has to stop
> +   *        [optional - default: 600s]
> +   */
> +  download: function aboutDialogDownload(aChannel, aWaitForFinish, aTimeout) {
> +    waitForFinish = aWaitForFinish ? aWaitForFinish : true;

You missed to simplify this assignment.

@@ +155,5 @@
> +          this.waitForDownloadFinished(timeout);
> +        }
> +        assert.waitFor(function () {
> +          return this.getUpdateState() === "updateButtonBox";
> +        }, "Final wizard page has been selected.", undefined, undefined, this);

Please reference self as this and get rid of the last 3 arguments.

@@ +157,5 @@
> +        assert.waitFor(function () {
> +          return this.getUpdateState() === "updateButtonBox";
> +        }, "Final wizard page has been selected.", undefined, undefined, this);
> +        break;
> +      case "updateButtonBox":

This will also be caught in the default: state.
We can remove this.

@@ +173,5 @@
> +   *
> +   * @returns {MozMillElement}
> +   *          Returns an  MozMill element
> +   */
> +  getElements: function aboutDialog_getElements(aSpec) {

We should rename this into getElement, we always return 1 element.

@@ +196,5 @@
> +   *
> +   * @returns {String}
> +   *          The Id of the actual enabled element in updateDeck
> +   */
> +  getUpdateState: function aboutDialog_getUpdateState() {

This also looks like a perfect candidate for a getter.

@@ +208,5 @@
> +   *
> +   * @param {MozMillController} aController
> +   *        Controller of the parent window
> +   */
> +  open: function aboutDialog_open(aController) {

See previous comment to enhance this method.

::: lib/ui/wizard-pages.js
@@ +234,5 @@
> +    this._wizard = this.getElement({type: "wizard"});
> +
> +    assert.waitFor(function () {
> +      return this.currentPage !== WIZARD_PAGES.dummy;
> +    }, "Dummy wizard page has been made invisible", undefined, undefined, this);

Reference self to this.
Please also check other instances.

::: tests/update/testDirectUpdate/test1.js
@@ +20,5 @@
>    persisted.updateIndex = 0;
>  
>    persisted.updates = [{
> +    build_pre : aModule.aboutDialog.softwareUpdate.buildInfo,
> +    build_post :  aModule.aboutDialog.softwareUpdate.buildInfo,

nit: 2 spaces

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 816969 [details] [diff] [review]
patch v3.2

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

::: lib/software-update.js
@@ +26,4 @@
>  /**
>   * Constructor for software update class
>   */
>  function softwareUpdate() {

Also update the class to use a capital letter at the beginning.

@@ +113,5 @@
>    get isCompleteUpdate() {
>      // Throw when isCompleteUpdate is called without an update. This should
>      // never happen except if the test is incorrectly written.
> +    assert.ok(this.activeUpdate, arguments.callee.name +
> +                                 ": isCompleteUpdate called  when activeUpdate is null!");

Remove arguments.callee.name. It's not necessary anymore given that we have a stack included.

@@ +242,5 @@
>  
> +    // It takes a long time to apply the update and a situation might arise where
> +    // "applied" status overwrites the "failed: 6" string
> +    assert.waitFor(function () {
> +        return self.activeUpdate !== null && self.activeUpdate.state.contains("applied");

I would do an assert for activeUpdate at the beginning of the method similar to above. '!== null' is also not necessary. Any real object is treated as true.

Also I don't see why we have to wait here. It's totally unrelated to this method.

@@ +259,5 @@
> +   *
> +   * @returns {String}
> +   *          Returns the "Apply Update..." button label
> +   */
> +  getApplyButtonLabel: function softwareUpdate_getapplyButtonLabel() {

This is a back-end module. Why does it still include UI related functionality?

@@ +280,1 @@
>      return dtds;

I don't see why we still need this method in the module.

::: lib/ui/about-dialog.js
@@ +5,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +var { assert, expect } = require("../assertions");
> +var utils = require("../utils");
> +var softwareUpdate = require("../software-update");

alphabetical order please

@@ +8,5 @@
> +var utils = require("../utils");
> +var softwareUpdate = require("../software-update");
> +
> +const STATE = [
> +  "updateButtonBox",

What's that for a state?

@@ +21,5 @@
> +  "manualUpdate",
> +  "unsupportedSystem"
> +];
> +
> +const TIMEOUT_UPDATE_DOWNLOAD = 600000;

Why are you increasing the timeout here? That will not work, Cosmin given the jsbridge timeout. What's the reasoning for it?

@@ +23,5 @@
> +];
> +
> +const TIMEOUT_UPDATE_DOWNLOAD = 600000;
> +
> +function aboutDialog() {

AboutDialog()

@@ +28,5 @@
> +  this._controller = null;
> +
> +  // Controller of the main window from which we open About Dialog
> +  this._mainController = null;
> +  this._softwareUpdate = new softwareUpdate.softwareUpdate();

I don't see why it has to be a "private" property with an additional getter. Lets make it public.

@@ +70,5 @@
> +   *
> +   * @param {MozMillController} aController
> +   *        Controller of the parent window from which we open the about dialog
> +   */
> +  checkAboutDialog : function softwareUpdate_checkAboutDialog(aController) {

I wonder for which purpose we have this method. Please explain.

@@ +73,5 @@
> +   */
> +  checkAboutDialog : function softwareUpdate_checkAboutDialog(aController) {
> +    this.open(aController);
> +    utils.handleWindow("type", "Browser:About", function (aController) {
> +      var button = new elementslib.Selector(aController.window.document, "#updateButton");

Why is this not included in getElements?

@@ +86,5 @@
> +  checkForUpdates: function aboutDialog_checkForUpdates() {
> +    var updateBtn = this.getElements({type: "updateButton"});
> +    assert.ok(this._controller, "About Dialog has been opened");
> +    assert.ok(updateBtn.exists(), "Check for updates button has been found");
> +    this._controller.waitThenClick(updateBtn);

Similar behavior as the method above. Andrei has already pointed that out.

@@ +98,5 @@
> +   *        or throw an Error if false
> +   *
> +   */
> +  close : function aboutDialog_close(aIgnoreFailures) {
> +    const DOM_WINDOW_DESTROYED_TOPIC = "dom-window-destroyed";

I wouldn't include all this code in this method. Do we have a windows.js module? Lets put it in there and call it closeWindow and have another method called closeAllWindows. Those can force the close action. But here this is overhead.

@@ +120,5 @@
> +      }, "About Dialog has been closed");
> +    }
> +    catch (ex) {
> +      if (!aIgnoreFailures) {
> +        throw new Error("About Dialog has been closed.");

Never throw an Error. Therefore we have the assertions module.

@@ +142,5 @@
> +   *        Timeout the download has to stop
> +   *        [optional - default: 600s]
> +   */
> +  download: function aboutDialogDownload(aChannel, aWaitForFinish, aTimeout) {
> +    waitForFinish = aWaitForFinish ? aWaitForFinish : true;

If false gets specified via the parameter it will be treated as true.

@@ +168,5 @@
> +  /**
> +   *
> +   * @param {Object} aSpec
> +   *        Information of the UI elements which should be retrieved
> +   *        Elements: type - Identifier of the element

Please define the jsdoc correctly for sub properties. This is a new module so we should do it right and not depend on other bugs.

@@ +173,5 @@
> +   *
> +   * @returns {MozMillElement}
> +   *          Returns an  MozMill element
> +   */
> +  getElements: function aboutDialog_getElements(aSpec) {

I miss the getElement() method.

@@ +176,5 @@
> +   */
> +  getElements: function aboutDialog_getElements(aSpec) {
> +    var spec = aSpec || {};
> +    var element;
> +    switch (spec.type) {

nit: empty line above please

@@ +180,5 @@
> +    switch (spec.type) {
> +      case "updateDeck":
> +        element = new elementslib.ID(this._controller.window.document, "updateDeck");
> +        break;
> +      case "updateButton":

Wrong alphabetical order of case statements.

@@ +203,5 @@
> +    return STATE[index];
> +  },
> +
> +  /**
> +   * Opens the About Dialog and handel de About Window

Not sure which language that is. :)

@@ +233,5 @@
> +   */
> +  waitForDownloadFinished : function aboutDialog_waitForDownloadFinished(aTimeout) {
> +    var self = this;
> +    assert.waitFor(function () {
> +      return  self.softwareUpdate.activeUpdate !== null &&

As above I would have a separate call to check for a valid update, or a helper method given that we make use of it in various places.

@@ +238,5 @@
> +              self.softwareUpdate.activeUpdate.state.contains("applied");
> +    }, "Download has been completed and applied", aTimeout);
> +
> +    assert.notEqual(this.getUpdateState(), "downloadFailed",
> +                    "Download has been done successfully");

Which other states could we have here? I think this check is not complete.

::: lib/ui/wizard-pages.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

What is this module for? I assume its for the old software updater? Please reflect that in the name.

Otherwise no special comments on that file given that we want to get rid of the old ui as soon as possible. I assume all its content is copy&paste?

@@ +4,5 @@
> +// Helper lookup constants for elements of the software update dialog
> +
> +var { assert } = require("../assertions");
> +var utils = require("../utils");
> +var softwareUpdate = require("../software-update");

wrong alphabetical order.

::: tests/update/testDirectUpdate/test1.js
@@ +16,4 @@
>  
>    // Prepare persisted object for update results
>    // If an update fails the post build has to be the same as the pre build.
> +  persisted.updateStagingPath = aModule.aboutDialog.softwareUpdate.stagingDirectory.path;

I wouldn't use the AboutDialog class here but a direct instance of SoftwareUpdate. The about dialog is not used in this file.

::: tests/update/testDirectUpdate/test2.js
@@ +30,5 @@
>   */
>  var testDirectUpdate_Download = function() {
>    // Check if the user has permissions to run the update
> +  assert.ok(aboutDialog.softwareUpdate.allowed,
> +            "User has permissions to update the build.");

All this is not specific to the ui, so please really use the backend module.

::: tests/update/testFallbackUpdate/test3.js
@@ +35,5 @@
>   * Test that the patch hasn't been applied and the complete patch gets downloaded
>   **/
>  function testFallbackUpdate_ErrorPatching() {
>    // The dialog should be open in the background and shows a failure
> +  wizardPages.waitForDialogOpen(controller);

So we still get the old ui, when we make an update invalid?

Comment 47

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0

(In reply to Henrik Skupin (:whimboo) from comment #46)
lso update the class to use a capital letter at the beginning.
> @@ +242,5 @@
> >  
> > +    // It takes a long time to apply the update and a situation might arise where
> > +    // "applied" status overwrites the "failed: 6" string
> > +    assert.waitFor(function () {
> > +        return self.activeUpdate !== null && self.activeUpdate.state.contains("applied");
> Also I don't see why we have to wait here. It's totally unrelated to this
> method.
Right, I removed this, we are doing the same assertion in test when we download.

> @@ +259,5 @@
> > +   *
> > +   * @returns {String}
> > +   *          Returns the "Apply Update..." button label
> > +   */
> > +  getApplyButtonLabel: function softwareUpdate_getapplyButtonLabel() {
> 
> This is a back-end module. Why does it still include UI related
> functionality?
I also removed this, I used the method when I was testing with incompatible addons, But for that i guess we should file a new bug.

> > +const STATE = [
> > +  "updateButtonBox",
> 
> What's that for a state?
Those states are actual ids, and this one is for restart/apply button.
> @@ +70,5 @@
> > +   *
> > +   * @param {MozMillController} aController
> > +   *        Controller of the parent window from which we open the about dialog
> > +   */
> > +  checkAboutDialog : function softwareUpdate_checkAboutDialog(aController) {
> 
> I wonder for which purpose we have this method. Please explain.
It was used check that we are in the right window, I removed the method and now I check that in open method after we open the About:Dialog

> @@ +98,5 @@
> > +   *        or throw an Error if false
> > +   *
> > +   */
> > +  close : function aboutDialog_close(aIgnoreFailures) {
> > +    const DOM_WINDOW_DESTROYED_TOPIC = "dom-window-destroyed";
> 
> I wouldn't include all this code in this method. Do we have a windows.js
> module? Lets put it in there and call it closeWindow and have another method
> called closeAllWindows. Those can force the close action. But here this is
> overhead.
Andreea is working on this I will use that method when it will be ready (next review iteration or I will make a followup patch)

> > +              self.softwareUpdate.activeUpdate.state.contains("applied");
> > +    }, "Download has been completed and applied", aTimeout);
> > +
> > +    assert.notEqual(this.getUpdateState(), "downloadFailed",
> > +                    "Download has been done successfully");
> 
> Which other states could we have here? I think this check is not complete.
The other state would be "updateButtonBox" witch is restart button.
After the download starts the state would be either Restart or Failed. 


> ::: tests/update/testFallbackUpdate/test3.js
> @@ +35,5 @@
> >   * Test that the patch hasn't been applied and the complete patch gets downloaded
> >   **/
> >  function testFallbackUpdate_ErrorPatching() {
> >    // The dialog should be open in the background and shows a failure
> > +  wizardPages.waitForDialogOpen(controller);
> 
> So we still get the old ui, when we make an update invalid?
Yes, we get the old ui when the update is invalid, or when we force to fall-back, and to handle those windows we still use the old libraries. 

Reports:
http://mozmill-crowd.blargon7.com/#/update/report/f54ca65f73fd56845c30b7ab7d8d5395
http://mozmill-crowd.blargon7.com/#/update/report/f54ca65f73fd56845c30b7ab7d8d6ba3
http://mozmill-crowd.blargon7.com/#/update/report/f54ca65f73fd56845c30b7ab7d8d52dd
http://mozmill-crowd.blargon7.com/#/update/report/f54ca65f73fd56845c30b7ab7d8d35c1
http://mozmill-crowd.blargon7.com/#/update/report/f54ca65f73fd56845c30b7ab7d8d60f0
http://mozmill-crowd.blargon7.com/#/update/report/f54ca65f73fd56845c30b7ab7d8d43b8

Comment 48

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 819052 [details] [diff] [review]
patch v4.0

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

Regarding the windows.js library, with all the metro priorities now, I don't have time to update it. We either add a TODO here to change that once the lib is ready or someone else can take it and finish it, it's not much left to do there.

::: lib/software-update.js
@@ +112,5 @@
>     */
>    get isCompleteUpdate() {
>      // Throw when isCompleteUpdate is called without an update. This should
>      // never happen except if the test is incorrectly written.
> +    assert.ok(this.activeUpdate, "isCompleteUpdate is called  with an activeUpdate");

There are 2 whitespaces at the middle of this message. Also please call it: "An active update has been found".

@@ +201,5 @@
>     *
>     * @param {object} updateData
>     *        All the data collected during the update process
>     */
> +  assertUpdateApplied : function SoftwareUpdate_assertUpdateApplied(updateData) {

aUpdateData as parameter.

@@ +239,4 @@
>      var updateStatus = this.stagingDirectory;
>      updateStatus.append("update.status");
>  
> +    assert.ok(this.activeUpdate, "forceFallback is called  with an activeUpdate");

Also 2 whitespaces here. "An active update has been found"

::: lib/ui/about-dialog.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +

Add "use strict;" please.

@@ +18,5 @@
> +  "adminDisabled",
> +  "noUpdatesFound",
> +  "otherInstanceHandlingUpdates",
> +  "manualUpdate",
> +  "unsupportedSystem"

If some of these are actual id's, we should move them in a getElements method and only leave actual states here.

@@ +27,5 @@
> +function AboutDialog() {
> +  this._controller = null;
> +
> +  // Controller of the main window from which we open About Dialog
> +  this._mainController = null;

We could call this browserController?

@@ +125,5 @@
> +   *        Sets if the function should wait until the download has been finished
> +   *        [optional - default: true]
> +   * @param {Number} aTimeout
> +   *        Timeout the download has to stop
> +   *        [optional - default: 600s]

We use [arg_name=default_value] for optional parameters. Please see bug 882137

@@ +144,5 @@
> +          return self.updateState === "updateButtonBox";
> +        }, "Final wizard page has been selected.");
> +        break;
> +      default:
> +        break;

Why aren't we throwing if the wrong case was provided?

@@ +209,5 @@
> +   *        How long it will wait for downloading state to change
> +   */
> +  waitForDownloadFinished : function AboutDialog_waitForDownloadFinished(aTimeout) {
> +    assert.ok(this.softwareUpdate.activeUpdate,
> +              " waitForDownloadFinished is called  with an activeUpdate");

It's odd to have the variables name in the message. Lets call it "An active update has been found".

::: lib/ui/wizard-pages.js
@@ +36,5 @@
> +
> +// On Mac there is another DOM structure used as on Windows and Linux
> +if (mozmill.isMac) {
> +  var WIZARD_BUTTONS_BOX = WIZARD_BUTTONS +
> +    '/anon({"flex":"1"})/{"class":"wizard-buttons-btm"}/';

Indentation should start bellow WIZARD_BUTTONS.

::: tests/update/testDirectUpdate/test2.js
@@ +15,5 @@
>  }
>  
>  function teardownModule(aModule) {
>    // Store the patch info from a possibly found update
> +  persisted.updates[persisted.updateIndex].patch = aModule.aboutDialog.softwareUpdate.patchInfo;

Isn't this exceeding the 80char limit?

Comment 49

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.1

(In reply to Andreea Matei [:AndreeaMatei] from comment #48)
> @@ +18,5 @@
> > +  "adminDisabled",
> > +  "noUpdatesFound",
> > +  "otherInstanceHandlingUpdates",
> > +  "manualUpdate",
> > +  "unsupportedSystem"
> 
> If some of these are actual id's, we should move them in a getElements
> method and only leave actual states here.
They are all id's, and we use the selectedindex in updateState getter to determine which elements is being displayed in the updateDeck.
> @@ +144,5 @@
> > +          return self.updateState === "updateButtonBox";
> > +        }, "Final wizard page has been selected.");
> > +        break;
> > +      default:
> > +        break;
> 
> Why aren't we throwing if the wrong case was provided?
For a moment we only check here if the state is downloading, in which case we wait for download to finish.
Any other case should pass, as we check other states separately in tests.

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 825262 [details] [diff] [review]
patch_v4.1

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

::: lib/software-update.js
@@ +10,1 @@
>  Cu.import("resource://gre/modules/Services.jsm");

Add 'use strict' here too.

@@ +28,1 @@
>    this._downloadDuration = -1;

This is also obsolete in this class now.

::: lib/ui/about-dialog.js
@@ +25,5 @@
> +];
> +
> +const TIMEOUT_UPDATE_DOWNLOAD = 360000;
> +
> +function AboutDialog() {

Missing jsdoc.

@@ +29,5 @@
> +function AboutDialog() {
> +  this._controller = null;
> +
> +  // Controller of the main window from which we open About Dialog
> +  this.browserController = null;

I would update the comment so it says 'browser window'.

@@ +39,5 @@
> +  /**
> +   * Returns the controller of the class instance
> +   *
> +   * @returns {MozMillController}
> +   *          Controller of the window

One line please. Same for all the other instances

@@ +48,5 @@
> +
> +  /**
> +   * Checks if updates were found
> +   *
> +   *  @returns {Boolean}

wrong indentation additional to the above mentioned changes.

@@ +56,5 @@
> +    return this.updateState !== "noUpdatesFound";
> +  },
> +
> +  /**
> +   * Method used to determine current state of updates

please kill 'method used'

@@ +61,5 @@
> +   *
> +   * @returns {String}
> +   *          The Id of the actual enabled element in updateDeck
> +   */
> +  get updateState() {

I wouldn't call it update state then if all elements are ids. 'wizardState' or so sounds better.

@@ +74,5 @@
> +  checkForUpdates: function AboutDialog_checkForUpdates() {
> +    assert.ok(this._controller, "About Dialog has been opened");
> +    var updateBtn = this.getElement({type: "updateButton"});
> +    assert.ok(updateBtn.exists(), "Check for updates button has been found");
> +    this._controller.waitThenClick(updateBtn);

I really wouldn't do the assert check here, but move this to a test. So this method should return true if the button is visible instead.

@@ +78,5 @@
> +    this._controller.waitThenClick(updateBtn);
> +  },
> +
> +  // Bug 800872
> +  // Change this function once windows.js library is implemented

What's that? Please add this as a todo item inside the jsdoc comment.

@@ +91,5 @@
> +  close : function AboutDialog_close(aIgnoreFailures) {
> +    const DOM_WINDOW_DESTROYED_TOPIC = "dom-window-destroyed";
> +
> +    var windowClosed = false;
> +    var observer = {

Please give the object a better name like windowDestroyedObserver.

@@ +126,5 @@
> +   * @param {String} aChannel
> +   *        Update channel to use
> +   * @param {Boolean} aWaitForFinish
> +   *        Sets if the function should wait until the download has been finished
> +   *        [aChannel=true]

Please check again how default values are set correctly.

@@ +135,5 @@
> +  download: function AboutDialog_download(aChannel, aWaitForFinish, aTimeout) {
> +    var waitForFinish = (aWaitForFinish === undefined) ? true : aWaitForFinish;
> +    var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +    var self = this;
> +    // Check that the correct channel has been set

nit: blank line please.

@@ +137,5 @@
> +    var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +    var self = this;
> +    // Check that the correct channel has been set
> +    assert.equal(aChannel, this.softwareUpdate.channel,
> +                 "The update channel has been set correctly.");

Where do we measure the download time? This has not been implemented here.

@@ +148,5 @@
> +          return self.updateState === "updateButtonBox";
> +        }, "Final wizard page has been selected.");
> +        break;
> +      default:
> +        break;

No break necessary here.

@@ +153,5 @@
> +    }
> +  },
> +
> +  /**
> +   *

Missing function description.

@@ +155,5 @@
> +
> +  /**
> +   *
> +   * @param {object} aSpec- Information of the UI elements which should be retrieved
> +   * @config {string} [type] Identifier of the element

Please use a new line for the parameter description

@@ +158,5 @@
> +   * @param {object} aSpec- Information of the UI elements which should be retrieved
> +   * @config {string} [type] Identifier of the element
> +   *
> +   * @returns {MozMillElement}
> +   *          Returns an  MozMill element

A single line please for the @returns line.

@@ +171,5 @@
> +        break;
> +      case "updateDeck":
> +        element = new elementslib.ID(this._controller.window.document, "updateDeck");
> +        break;
> +      default:

This should really be the getElements() method. getElement() is a wrapper only.

@@ +185,5 @@
> +   * @param {MozMillController} aController
> +   *        Controller of the parent window
> +   */
> +  open: function AboutDialog_open(aController) {
> +    this.browserController = aController;

Lets assert for a valid controller before. Btw. do we still need the browserController later?

@@ +187,5 @@
> +   */
> +  open: function AboutDialog_open(aController) {
> +    this.browserController = aController;
> +    this.browserController.mainMenu.click("#aboutName");
> +    this._controller = utils.handleWindow("type", "Browser:About", undefined, false);

Whether this has to be moved out to waitForOpened() or the old UI has to be made in sync with this change.

@@ +192,5 @@
> +
> +    // Check that we are in the right window
> +    var button = this.getElement({type: "updateButton"});
> +    expect.ok(!button.getNode().hidden,
> +              "The update button is always visible even after an update.");

So I don't understand what we test in checkForUpdates() then, where exists() has been used.

@@ +223,5 @@
> +
> +    assert.notEqual(this.updateState, "downloadFailed",
> +                    "Download has been done successfully");
> + }
> +};

Indentation issues

::: lib/ui/wizard-pages.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Again as mentioned in my last review, rename this file.

@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +// Helper lookup constants for elements of the software update dialog

This line doesn't seem to belong here.

@@ +168,5 @@
> +          this.currentPage);
> +    }
> +
> +    // Calculate the duration in ms
> +    this._downloadDuration = Date.now() - startTime;

This needs to be defined in the constructor. See the left-over declaration in the backend module.

@@ +181,5 @@
> +
> +   * @returns {MozMillElement}
> +   *          Returns an  MozMill element
> +   */
> +  getElement : function softwareUpdate_getElement(aSpec) {

Again, this is getElements().

::: tests/update/testDirectUpdate/test1.js
@@ +9,1 @@
>  var prefs = require("../../../lib/prefs");

Why this reordering? Also I would simply call the variable for the imported module 'update'.

@@ +15,1 @@
>    aModule.controller = mozmill.getBrowserController();

Put this behind the controller definition, which should always be first here.

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I think we might have to wait for the landing of the patch on bug 600500 which reorganizes the about dialog for updates. We also would need a sane fallback mechanism for older versions.

Comment 52

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.2

I updated the patch.

(In reply to Henrik Skupin (:whimboo) [away 12/21 - 01/01] from comment #51)
> We also would need a sane fallback mechanism for older versions.
I've tested the patch with Nightly 20 and it passes, with builds as Nightly 11 it doesn't pass not even without the patch and even there we have the new UI. 

Reports:
http://mozmill-crowd.blargon7.com/#/update/report/361b4f553c3df363a1b22c12641ab280
http://mozmill-crowd.blargon7.com/#/update/report/361b4f553c3df363a1b22c126417e5cb
http://mozmill-crowd.blargon7.com/#/update/report/361b4f553c3df363a1b22c126416e763
http://mozmill-crowd.blargon7.com/#/update/report/361b4f553c3df363a1b22c126416abb4
http://mozmill-crowd.blargon7.com/#/update/report/361b4f553c3df363a1b22c126415b187
http://mozmill-crowd.blargon7.com/#/update/report/361b4f553c3df363a1b22c12641567ae

Comment 53

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8351214 [details] [diff] [review]
patch v4.2

I removed Henrik as a reviewer until I will get an r+ from Andrei and Andreea.

Comment 54

[Andrei Eftimie]

Comment on attachment 8351214 [details] [diff] [review]
patch v4.2

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

::: firefox/lib/ui/about-dialog.js
@@ +56,5 @@
> +    return deck.childNodes[deck.selectedIndex].id;
> +  },
> +
> +  // Bug 800872
> +  // TODO: Change this function once windows.js library is implemented

Please remove this comment.
That bug is irrelevant at the moment for this test as it won't include any new window library.

::: firefox/lib/ui/wizard-pages.js
@@ +11,5 @@
> +const TIMEOUT_UPDATE_DOWNLOAD = 360000;
> +
> +const WIZARD = '/id("updates")';
> +const WIZARD_BUTTONS = WIZARD + '/anon({"anonid":"Buttons"})';
> +const WIZARD_DECK = WIZARD  + '/anon({"anonid":"Deck"})';

All these should be updated to nodeCollector.

@@ +73,5 @@
> +   * @returns {String}
> +   *          Returns the current page Id as string
> +   */
> +  get currentPage() {
> +    return this._wizard.getNode().getAttribute('currentpageid');

nit: duble quotes for consistency

@@ +96,5 @@
> +
> +  /**
> +   * Close the software update dialog
> +   */
> +  closeDialog: function softwareUpdate_closeDialog() {

I think the name should actually be `WizardPages_closeDialog`

@@ +115,5 @@
> +   *        Sets if the function should wait until the download has been finished
> +   * @param {Number} aTimeout
> +   *        Timeout the download has to stop
> +   */
> +  download: function aboutDialogDownload(aChannel, aWaitForFinish, aTimeout) {

`WizardPages_download`

@@ +116,5 @@
> +   * @param {Number} aTimeout
> +   *        Timeout the download has to stop
> +   */
> +  download: function aboutDialogDownload(aChannel, aWaitForFinish, aTimeout) {
> +    var waitForFinish = aWaitForFinish || true;

In the about-dialog lib we instantiate this variable differently.
Mainly if it's `undefined` it will fallback to `true`. We should normalise it's behaviour. 

The question is: do we want it defaulting to `true` when we don't specify it at all?

@@ +153,5 @@
> +          this.waitforDownloadFinished(timeout);
> +
> +          assert.waitFor(function () {
> +            return self.currentPage ===  WIZARD_PAGES.finished ||
> +              self.currentPage === WIZARD_PAGES.finishedBackground;

nit: indent

@@ +191,5 @@
> +     * value: value to match
> +     */
> +      case "button":
> +        elem = new elementslib.Lookup(this._controller.window.document,
> +          WIZARD_BUTTONS_BOX + WIZARD_BUTTON[spec.subtype]);

nodeCollector please

@@ +220,5 @@
> +    var timeout = aTimeout || TIMEOUT_UPDATE_CHECK;
> +    var self = this;
> +
> +    assert.waitFor(function() {
> +      return self.currentPage != WIZARD_PAGES.checking;

nit: !==

@@ +263,5 @@
> +    var self = this;
> +
> +    assert.waitFor(function () {
> +      return self.currentPage !== WIZARD_PAGES.downloading ||
> +        progress.getNode().value === '100';

nit: double quotes

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8351214 [details] [diff] [review]
patch v4.2

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

Just some notes regarding this patch before we start to update it.

::: firefox/lib/ui/wizard-pages.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

As what I have mentioned earlier the name of the library file is no appropriate given that it is too vague. Please check its original window name and rename it accordingly.

@@ +11,5 @@
> +const TIMEOUT_UPDATE_DOWNLOAD = 360000;
> +
> +const WIZARD = '/id("updates")';
> +const WIZARD_BUTTONS = WIZARD + '/anon({"anonid":"Buttons"})';
> +const WIZARD_DECK = WIZARD  + '/anon({"anonid":"Deck"})';

I don't think it is worth the time now. It may be that we can get rid of the old dialog hopefully soon. So I would leave that as it is.

Comment 56

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.3

I renamed the ui module for handling the old ui to UpdateWizard, as the type of window handled is: "Update:Wizard"
http://mozmill-crowd.blargon7.com/#/update/report/d8c9b09561f242bc8489be43d60287cc
http://mozmill-crowd.blargon7.com/#/update/report/d8c9b09561f242bc8489be43d60277ea

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8389183 [details] [diff] [review]
patch_v4.3

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

Please ask Andreea and Andrei first for review.

Comment 58

[Andrei Eftimie]

Comment on attachment 8389183 [details] [diff] [review]
patch_v4.3

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

::: firefox/lib/ui/about-dialog.js
@@ +56,5 @@
> +    return deck.childNodes[deck.selectedIndex].id;
> +  },
> +
> +  // Bug 800872
> +  // TODO: Change this function once windows.js library is implemented

This is no longer valid, the bug has been closed, but we decided against implementing a windows lib.
We should still have a closeAllWindows method (which should work similar to closeAllTabs), but I'm not sure if this is tracked somewhere.

@@ +70,5 @@
> +    const DOM_WINDOW_DESTROYED_TOPIC = "dom-window-destroyed";
> +
> +    var windowClosed = false;
> +    var windowDestroyedObserver = {
> +      observe: function(aSubject, aTopic, aData) {

Update all anonymous functions to the fat arrow syntax please.

@@ +110,5 @@
> +   */
> +  download: function AboutDialog_download(aChannel, aWaitForFinish, aTimeout) {
> +    var waitForFinish = (aWaitForFinish === undefined) ? true : aWaitForFinish;
> +    var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +    var self = this;

You can remove this is you switch the waitFor callback to the fat arrow syntax because of lexical bound scope.

@@ +190,5 @@
> +   * Waits until checking for updates is done
> +   */
> +  waitForCheckFinished: function waitForCheckFinished() {
> +    var self = this;
> +    assert.waitFor(function() {

Same here: fat arrow, remove `self`.

@@ +201,5 @@
> +   *
> +   * @param {Number} aTimeout
> +   *        How long it will wait for downloading state to change
> +   */
> +  waitForDownloadFinished : function AboutDialog_waitForDownloadFinished(aTimeout) {

We should assert to the validity of aTimeout (number, higher then 0)

::: firefox/lib/ui/update-wizard.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

missing: "use strict";

@@ +35,5 @@
> +
> +// On Mac there is another DOM structure used as on Windows and Linux
> +if (mozmill.isMac) {
> +  var WIZARD_BUTTONS_BOX = WIZARD_BUTTONS +
> +      '/anon({"flex":"1"})/{"class":"wizard-buttons-btm"}/';

nit: this is indented with 4 spaces

@@ +98,5 @@
> +   * Close the software update dialog
> +   */
> +  closeDialog: function UpdateWizard_closeDialog() {
> +    if (this._controller) {
> +      this._controller.keypress(null, "VK_ESCAPE", {});

Please trigger this event on an element, this is a deprecated method on the controller and will no longer be available in the next mozmill version.

@@ +131,5 @@
> +    var next = this.getElement({type: "button", subtype: "next"});
> +    var page = this.currentPage;
> +
> +    // Click 'Next' and wait until the next page has been selected
> +    this._controller.click(next);

next.click()

@@ +138,5 @@
> +    }, "Update available page has been processed.");
> +
> +    // If incompatible add-on are installed we have to skip over the wizard page
> +    if (this.currentPage === WIZARD_PAGES.incompatibleList) {
> +      this._controller.click(next);

next.click()

@@ +153,5 @@
> +          this.waitforDownloadFinished(timeout);
> +
> +          assert.waitFor(function () {
> +            return self.currentPage ===  WIZARD_PAGES.finished ||
> +              self.currentPage === WIZARD_PAGES.finishedBackground;

nit: indent in regards to previous' line first return value.

@@ +174,5 @@
> +  /**
> +   * Retrieve an UI element based on the given spec
> +   *
> +   * @param {Object} aSpec Information of the UI element which should be retrieved
> +   * @config {string} [type] Identifier of the element

Mark `type` as an attribute of aSpec.
Also include subtype.

@@ +184,5 @@
> +  getElement : function UpdateWizard_getElement(aSpec) {
> +    var elem = null;
> +    var spec = aSpec || {};
> +
> +    switch(spec.type) {

nit: space between switch and paranthesis

@@ +188,5 @@
> +    switch(spec.type) {
> +    /**
> +     * subtype: subtype to match
> +     * value: value to match
> +     */

Remove this comment.

@@ +194,5 @@
> +        elem = new elementslib.Lookup(this._controller.window.document,
> +          WIZARD_BUTTONS_BOX + WIZARD_BUTTON[spec.subtype]);
> +        break;
> +      case "wizard":
> +        elem = new elementslib.Lookup(this._controller.window.document, WIZARD);

findElement

@@ +197,5 @@
> +      case "wizard":
> +        elem = new elementslib.Lookup(this._controller.window.document, WIZARD);
> +        break;
> +      case "wizard_page":
> +        elem = new elementslib.Lookup(this._controller.window.document, WIZARD_DECK +

findElement

@@ +201,5 @@
> +        elem = new elementslib.Lookup(this._controller.window.document, WIZARD_DECK +
> +          '/id(' + spec.subtype + ')');
> +        break;
> +      case "download_progress":
> +        elem = new elementslib.ID(this._controller.window.document, "downloadProgress");

findElement

@@ +263,5 @@
> +    var self = this;
> +
> +    assert.waitFor(function () {
> +      return self.currentPage !== WIZARD_PAGES.downloading ||
> +        progress.getNode().value === '100';

nit: indentation

::: firefox/tests/functional/restartTests/testSoftwareUpdateAutoProxy/test2.js
@@ +9,1 @@
>  var {expect} = require("../../../../../lib/assertions");

This can be removed.

::: firefox/tests/update/testDirectUpdate/test2.js
@@ +9,1 @@
>  var { assert } = require("../../../../lib/assertions");

You can remove this.

@@ +39,4 @@
>  
> +  // Wait until the check has been finished
> +  var updateButton = aboutDialog.getElement({type: "updateButton"});
> +  controller.waitThenClick(updateButton);

updateButton.waitThenClick()

@@ +44,3 @@
>  
>    // Download the update
>    assert.waitFor(function() {

Fat arrow syntax.

::: firefox/tests/update/testDirectUpdate/test3.js
@@ +9,1 @@
>  var { expect } = require("../../../../lib/assertions");

Can be removed.

@@ +38,5 @@
>    // Open the software update dialog and wait until the check has been finished
> +  aboutDialog.open(controller);
> +
> +  var updateButton = aboutDialog.getElement({type: "updateButton"});
> +  controller.waitThenClick(updateButton);

updateButton.waitThenClick()

::: firefox/tests/update/testFallbackUpdate/test2.js
@@ +9,1 @@
>  var { assert } = require("../../../../lib/assertions");

Can be removed.

@@ +39,4 @@
>  
> +  // Wait until the check has been finished
> +  var updateButton = aboutDialog.getElement({type: "updateButton"});
> +  controller.waitThenClick(updateButton);

updateButton.waitThenClick()

@@ +44,3 @@
>  
> +  // If updates were found begin the download
> +  assert.waitFor(function () {

Fat arrow syntax.

::: firefox/tests/update/testFallbackUpdate/test3.js
@@ +9,1 @@
>  var {assert} = require("../../../../lib/assertions");

Can be removed.

@@ +51,4 @@
>  
> +    //Checks if the update button exist, and clicks on it
> +    var updateButton = aboutDialog.getElement({type: "updateButton"});
> +    controller.waitThenClick(updateButton);

updateButton.waitThenClick()

::: firefox/tests/update/testFallbackUpdate/test4.js
@@ +9,1 @@
>  var { expect } = require("../../../../lib/assertions");

Can be removed.

@@ +38,5 @@
>    // Open the software update dialog and wait until the check has been finished
> +  aboutDialog.open(controller);
> +
> +  var updateButton = aboutDialog.getElement({type: "updateButton"});
> +  controller.waitThenClick(updateButton);

updateButton.waitThenClick()

Comment 59

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v5.0

Thanks for review Andrei.

http://mozmill-crowd.blargon7.com/#/update/report/c90eeeb0a5511695efc7462553a44698
http://mozmill-crowd.blargon7.com/#/update/report/c90eeeb0a5511695efc7462553a468f9
http://mozmill-crowd.blargon7.com/#/update/report/c90eeeb0a5511695efc7462553a47570
http://mozmill-crowd.blargon7.com/#/update/report/c90eeeb0a5511695efc7462553a45984
http://mozmill-crowd.blargon7.com/#/update/report/c90eeeb0a5511695efc7462553a55ac2
http://mozmill-crowd.blargon7.com/#/update/report/c90eeeb0a5511695efc7462553a568d1

Comment 60

[Andrei Eftimie]

Comment on attachment 8411718 [details] [diff] [review]
patch_v5.0

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

r+ from me with the nits fixed. Ask Henrik for a review with the updated patch.

::: firefox/lib/ui/about-dialog.js
@@ +81,5 @@
> +      this._controller.window.close();
> +
> +      // Wait for it to be finished
> +      assert.waitFor(() => windowClosed,
> +                     "About Dialog has been closed");

You can leave this on one line.

@@ +144,5 @@
> +   * @param {object} aSpec- Information of the UI elements which should be retrieved
> +   * @config {string} [type] Identifier of the element
> +   *
> +   * @returns Elements which have been found
> +   * @type {array of ElemBase}

The type should be on the return line

::: firefox/lib/ui/update-wizard.js
@@ +106,5 @@
> +      window.keypress("VK_ESCAPE", {});
> +      this._controller.sleep(500);
> +      this._controller = null;
> +      this._wizard = null;
> +    }

I wonder if we should also do a check here to make sure the window has been closed.

@@ +213,5 @@
> +   */
> +  waitForCheckFinished : function UpdateWizard_waitForCheckFinished(aTimeout) {
> +    var timeout = aTimeout || TIMEOUT_UPDATE_CHECK;
> +
> +    assert.waitFor(function() {

Missed to update this functions' syntax.

Comment 61

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v5.1

Thanks for review Andrei, I fixed those and I added an assertion to check that window has been closed.  
http://mozmill-crowd.blargon7.com/#/update/report/db3ef7ec039e4c6b255196b628c408b2
http://mozmill-crowd.blargon7.com/#/update/report/db3ef7ec039e4c6b255196b628c3e0aa
http://mozmill-crowd.blargon7.com/#/update/report/db3ef7ec039e4c6b255196b628c3a131
http://mozmill-crowd.blargon7.com/#/update/report/db3ef7ec039e4c6b255196b628c394e8
http://mozmill-crowd.blargon7.com/#/update/report/db3ef7ec039e4c6b255196b628c3814c
http://mozmill-crowd.blargon7.com/#/update/report/db3ef7ec039e4c6b255196b628c35c51

Comment 62

[Andrei Eftimie]

Comment on attachment 8414382 [details] [diff] [review]
patch_v5.1

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

Looks good. Ask review fro Henrik please with the nits fixed.

::: firefox/lib/ui/about-dialog.js
@@ +142,5 @@
> +   *
> +   * @param {object} aSpec- Information of the UI elements which should be retrieved
> +   * @config {string} [type] Identifier of the element
> +   *
> +   * @returns {array of ElemBase} Elements which have been found

nit: {ElemBase[]}

::: firefox/lib/ui/update-wizard.js
@@ +104,5 @@
> +      let window = new findElement.MozMillElement("Elem",
> +                                                  this._controller.window);
> +      window.keypress("VK_ESCAPE", {});
> +      assert.waitFor(() => window.getNode().closed,
> +                     "Update:Wizard windows has been closed");

Hurray for removal of sleep.
nit: "window"

Comment 63

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v5.2

Thanks for review Andrei.

Comment 64

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8417959 [details] [diff] [review]
patch_v5.2

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

Let remove the review request for now. Given that we add some more ui libraries, we could rewrite a good portion here to use base window etc. Also with all the work for signed os x builds the patch mostly bitrotted. I would wait until all the refactoring is done.

Comment 65

[Cosmin Malutan, [:cosmin-malutan]]

Sound's good, If there is no pressure here, I'll assign this to default.

Comment 66

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Well the refactoring is done and the BaseWindow patch has been landed. That can be used now to update the patch. Also I do not plan any other refactoring for update tests in the near future.

Comment 67

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v6.0

I spent a lot of time on this and a small update on patch so it could stay on the table didn't hurt.

>(In reply to Henrik Skupin (:whimboo) from comment #66)
> Well the refactoring is done and the BaseWindow patch has been landed. That
> can be used now to update the patch.
I renamed the newly created class AboutDialog to AboutWindow and I extend it from BaseWindow as we do in BrowserWindow, to have the same implementation for all UI classes.
I moved the old UI lib in UpdateWizard, we still need this to handle the update errors page in UpdateFallback and in functional/restartTests/testSoftwareUpdateAutoProxy.
I didn't touch the software-update backend lib except for removing the UI related methods.

Whimboo, what do you think about this change AboutDialog > AboutWindow and it extends the BaseWindow class?
If it like it I would ask Andrei to review it first. 


Reports 
http://mozmill-crowd.blargon7.com/#/update/report/ad8dad905f3281cb521a23ddf38894c6
http://mozmill-crowd.blargon7.com/#/update/report/ad8dad905f3281cb521a23ddf388caeb

http://mozmill-crowd.blargon7.com/#/update/report/ad8dad905f3281cb521a23ddf3854887
http://mozmill-crowd.blargon7.com/#/update/report/ad8dad905f3281cb521a23ddf3854f5c

Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/447e3274a76247e6567e1c2aa6015df7

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8507840 [details] [diff] [review]
patch v6.0

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

(In reply to Cosmin Malutan from comment #67)
> I renamed the newly created class AboutDialog to AboutWindow and I extend it
> from BaseWindow as we do in BrowserWindow, to have the same implementation
> for all UI classes.

The role for this window is dialog, so we may have to stick with dialog. But on the other side we do not have the usual dialog buttons (ok, cancel, help), so `Window` would make sense again. Given that it is not modal we could have it as window.

> I moved the old UI lib in UpdateWizard, we still need this to handle the
> update errors page in UpdateFallback and in
> functional/restartTests/testSoftwareUpdateAutoProxy.

Right, this code is most likely sticking around for a long time.

> I didn't touch the software-update backend lib except for removing the UI
> related methods.

Please make sure that you have added my latest changes done within the last month for v2 signing on OS X.

Otherwise the code goes into the right direction, but still misses to fully implement the BaseWindow inheritance for the old UI, which has to be kept. Also the open/close methods have to be made more secure. f- because of those remaining issues.

::: firefox/lib/ui/about.js
@@ +15,5 @@
> +/**
> + * @class Constructor for AboutWindow class
> + * @constructor
> + */
> +function AboutWindow() {

Please adjust this code so you can pass in a controller if the window is already open.

@@ +54,5 @@
> + *        Sets if the function should wait until the download has been finished
> + * @param {Number} [aTimeout=600s]
> + *        Timeout the download has to stop
> + */
> +AboutWindow.prototype.download = function AboutWindow_download(aChannel, aWaitForFinish, aTimeout) {

We shouldn't need the channel anymore here.

@@ +85,5 @@
> +AboutWindow.prototype.getElement = function AboutWindow_getElement(aSpec) {
> +  var elements = this.getElements(aSpec);
> +
> +  return (elements.length > 0) ? elements[0] : undefined;
> +};

This will be in BaseWindow soon. So we wont need it anymore.

@@ +127,5 @@
> +
> +  this._controller =  windows.waitForWindowState(
> +    () => aController.mainMenu.click("#aboutName"),
> +    {type: "Browser:About", state: "open"}
> +  );

Please see the code Daniel has written for e.g the Places window. The about window itself doesn't know anything about the browser. You wanna pass in the method via an argument.

Also I think we should add the type/id of the window as class member. Better to file a new bug for that too.

::: firefox/lib/ui/update-wizard.js
@@ +55,5 @@
> +    next: 'anon({"anonid":"WizardButtonDeck"})/[1]/{"dlgtype":"next"}',
> +    cancel: '{"dlgtype":"cancel"}',
> +    finish: 'anon({"anonid":"WizardButtonDeck"})/[0]/{"dlgtype":"finish"}',
> +    extra1: '{"dlgtype":"extra1"}',
> +    extra2: '{"dlgtype":"extra2"}'

Taking those lookup strings for now is ok, but in the near future we should setup a BaseDialog class we could let this inherit from, and which already has the dialog buttons handled. There is a bug already on file for that.

@@ +59,5 @@
> +    extra2: '{"dlgtype":"extra2"}'
> +  }
> +}
> +
> +function UpdateWizard() {

This also has to be based on BaseWindow for now until we have BaseDialog.

@@ +99,5 @@
> +   * Close the software update dialog
> +   */
> +  closeDialog: function UpdateWizard_closeDialog() {
> +    windows.waitForWindowState(
> +      () => this._controller.window.close(),

This is a force close, which we should only do if that has been specified.

@@ +117,5 @@
> +   *        Sets if the function should wait until the download has been finished
> +   * @param {Number} aTimeout
> +   *        Timeout the download has to stop
> +   */
> +  download: function UpdateWizard_download(aChannel, aWaitForFinish, aTimeout) {

Same as for the About window. We wont need the channel here anymore.

@@ +177,5 @@
> +   *
> +   * @returns {MozMillElement}
> +   *          Returns an  MozMill element
> +   */
> +  getElement : function UpdateWizard_getElement(aSpec) {

This has to become getElements().

@@ +211,5 @@
> +    var updatePrompt = Cc["@mozilla.org/updates/update-prompt;1"]
> +                       .createInstance(Ci.nsIUpdatePrompt);
> +    updatePrompt.checkForUpdates();
> +
> +    this.waitForDialogOpen();

Similar to the about window. This has to be handled similarly, and the waitForDialogOpen method is obsolete.

::: firefox/tests/update/testDirectUpdate/testUpdate.js
@@ +63,5 @@
>  
>    // Check if the user has permissions to run the update
>    assert.ok(update.allowed, "User has permissions to update the build");
>  
> +  aboutWindow.open(controller);

I would suggest we make use of BrowserWindow in this test and have a method like browser.openAboutWindow(), which returns an AboutWindow instance.

@@ +68,4 @@
>  
> +  // Wait until the check has been finished
> +  var updateButton = aboutWindow.getElement({type: "updateButton"});
> +  updateButton.waitThenClick();

Is there a reason why we have to wait here?

@@ +73,5 @@
>  
>    // If an update has been found, download the patch
> +  assert.waitFor(() => aboutWindow.updatesFound, "An update has been found");
> +  aboutWindow.download(persisted.channel);
> +  aboutWindow.close();

Ultimately we should use the restart button, but we can't right now due to missing support for user restarts. So please add a comment here.

Comment 69

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v7.0

This patch is huge, and I'd like to get a feedback prior of a detailed review which I will request from Andrei and Andreea. 

(In reply to Henrik Skupin (:whimboo) from comment #68)
> Otherwise the code goes into the right direction, but still misses to fully
> implement the BaseWindow inheritance for the old UI, which has to be kept.
> Also the open/close methods have to be made more secure. f- because of those
> remaining issues.
Done, the close is inherited, the open is like in other modules with an opening callback sent from BrowserWindow.  
> Please see the code Daniel has written for e.g the Places window. The about
> window itself doesn't know anything about the browser. You wanna pass in the
> method via an argument.
Done that, but it was really hard to follow, I'm a bit concerned that people who will try to use the modules for the first time won't understand the opening of modules from the browserWindo object. 

> > +  // Wait until the check has been finished
> > +  var updateButton = aboutWindow.getElement({type: "updateButton"});
> > +  updateButton.waitThenClick();
It seemed to work without the wait, but this was added a long time ago and it might fail at some point without the waiting, I would keep it like this, it's readable and safe.





Reports:
http://mozmill-crowd.blargon7.com/#/update/report/b67428b6e502c230ff85b6a4c41c91dd
http://mozmill-crowd.blargon7.com/#/update/report/b67428b6e502c230ff85b6a4c41ca1a0
http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c41cbcc4

Comment 70

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8521230 [details] [diff] [review]
patch v7.0

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

Class structure wise this looks fine. I have made some inline comments for clarification and tasks.

::: firefox/lib/ui/browser.js
@@ +15,3 @@
>  var baseWindow = require("../../../lib/ui/base-window");
>  var unknownContentTypeDialog = require("unknown-content-type-dialog");
> +var updateWizard = require("update-wizard");

Maybe it would be better to not make them global but part of the method for opening the appropriate window. Otherwise we have to import too much.

::: firefox/lib/ui/update-wizard.js
@@ +63,5 @@
> +/**
> + * @class Constructor for UpdateWizard class
> + * @constructor
> + */
> +function UpdateWizard(aController) {

`UpdateWizardWindow` please

@@ +102,5 @@
> +  var waitForFinish = aWaitForFinish || true;
> +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +
> +  // Retrieve the timestamp, so we can measure the duration of the download
> +  var startTime = Date.now();

I do not see this code in the about window.

@@ +159,5 @@
> +
> +  switch (spec.type) {
> +    case "button":
> +      var lookup = WIZARD_BUTTONS_BOX + WIZARD_BUTTON[spec.subtype];
> +      elements = [new findElement.Lookup(this._controller.window.document, lookup)];

findElement has to be used without new.

@@ +235,5 @@
> +
> +  if (aCallback && (typeof aCallback === "function")) {
> +    aCallback();
> +  }
> +  var controller = windows.handleWindow("type", "Update:Wizard", undefined, false);

Why does it not use waitForWindowState()?

@@ +236,5 @@
> +  if (aCallback && (typeof aCallback === "function")) {
> +    aCallback();
> +  }
> +  var controller = windows.handleWindow("type", "Update:Wizard", undefined, false);
> +  controller.window.focus();

Why is focus() needed? Maybe worth a comment? If it is why not making it part of the constructor?

@@ +240,5 @@
> +  controller.window.focus();
> +
> +  var updateWizard = new UpdateWizard(controller);
> +  assert.waitFor(() => updateWizard.currentPage !== WIZARD_PAGES.dummy,
> +                 "Dummy wizard page has been made invisible");

Same here. Make this part of the constructor

::: firefox/tests/update/testDirectUpdate/testUpdate.js
@@ +90,5 @@
>    // Open the software update dialog and wait until the check has been finished
> +  var aboutWindow = browserWindow.openAboutWindow();
> +
> +  var updateButton = aboutWindow.getElement({type: "updateButton"});
> +  updateButton.waitThenClick();

Why this extra code? I do not see why we wanna do that. This is done automatically.

Comment 71

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v7.1

(In reply to Henrik Skupin (:whimboo) from comment #70)
> > +  // Retrieve the timestamp, so we can measure the duration of the download
> > +  var startTime = Date.now();
> 
> I do not see this code in the about window.
It's never used, or logged, so I've removed it from here too, usually we check the build time and the time-stamp logs if there is something suspicious with the download time.

> > +  }
> > +  var controller = windows.handleWindow("type", "Update:Wizard", undefined, false);
> 
> Why does it not use waitForWindowState()?
I struggled with that for a while until I found-out why it wasn't working:
waitForWindowState waits for an observer("toplevel-window-ready") which is not triggered the WizardWindow it's opened. 
handleWindow on the other hand it just checks and waits until the desired window exists.


> > +  controller.window.focus();
> 
> Why is focus() needed? Maybe worth a comment? If it is why not making it
> part of the constructor?
This has been here since we have the library, I removed it, works just as good as before, probably at the time the window required to be focused, mozmill evolved a lot since then so we don't need it. 

> > +  var updateButton = aboutWindow.getElement({type: "updateButton"});
> > +  updateButton.waitThenClick();
> 
> Why this extra code? I do not see why we wanna do that. This is done
> automatically.
This is the check button in comment 50 you requested to move this outside of method, this get's really hard to follow so I would really like to get this landed is lingering for more than 4 years, and it's impossible to remember everything that has been discussed and decided. 

(In reply to Henrik Skupin (:whimboo) from comment #50)
> @@ +74,5 @@
> > +  checkForUpdates: function AboutDialog_checkForUpdates() {
> > +    assert.ok(this._controller, "About Dialog has been opened");
> > +    var updateBtn = this.getElement({type: "updateButton"});
> > +    assert.ok(updateBtn.exists(), "Check for updates button has been found");
> > +    this._controller.waitThenClick(updateBtn);
> 
> I really wouldn't do the assert check here, but move this to a test. So this
> method should return true if the button is visible instead.

Comment 72

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8521387 [details] [diff] [review]
patch v7.1

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

(In reply to Cosmin Malutan from comment #71)
> > I do not see this code in the about window.
> It's never used, or logged, so I've removed it from here too, usually we
> check the build time and the time-stamp logs if there is something
> suspicious with the download time.

This is not true! Please see the code at the bottom of the method for the calculation and persisted storage. This time is needed in the test report and is visible in the dashboard.

> > Why does it not use waitForWindowState()?
> I struggled with that for a while until I found-out why it wasn't working:
> waitForWindowState waits for an observer("toplevel-window-ready") which is
> not triggered the WizardWindow it's opened. 
> handleWindow on the other hand it just checks and waits until the desired
> window exists.

No workarounds please. Figure out why we do not get such a notification. It's a top-level window so we should get it.

> > > +  controller.window.focus();
> > 
> > Why is focus() needed? Maybe worth a comment? If it is why not making it
> > part of the constructor?
> This has been here since we have the library, I removed it, works just as
> good as before, probably at the time the window required to be focused,
> mozmill evolved a lot since then so we don't need it. 

No popup related code in that window, so focus will never be necessary then.

> > > +  var updateButton = aboutWindow.getElement({type: "updateButton"});
> > > +  updateButton.waitThenClick();
> > 
> > Why this extra code? I do not see why we wanna do that. This is done
> > automatically.
> This is the check button in comment 50 you requested to move this outside of

In this comment I was talking about the assert but not that code.

> method, this get's really hard to follow so I would really like to get this
> landed is lingering for more than 4 years, and it's impossible to remember
> everything that has been discussed and decided. 

Meanwhile you should be aware that this is not an argument for getting code landed.

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Also as long as there are open discussions please try to avoid uploading new patches, but get discussions finished first. This only adds an extra delay you really don't want to have.

Comment 74

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #72)
> > > Why does it not use waitForWindowState()?
> > I struggled with that for a while until I found-out why it wasn't working:
> > waitForWindowState waits for an observer("toplevel-window-ready") which is
> > not triggered the WizardWindow it's opened. 
> > handleWindow on the other hand it just checks and waits until the desired
> > window exists.
> 
> No workarounds please. Figure out why we do not get such a notification.
> It's a top-level window so we should get it.
I tried to get this a s modalDialog but I couldn't, the only topic notified is "domwindowopened".
http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#1187

This get's notified before the current watched topic("toplevel-window-ready"), I would change the two watched topics on windows opening/closing from windows.js to 
"domwindowopened" and "domwindowclosed".
  
> 
> > > > +  var updateButton = aboutWindow.getElement({type: "updateButton"});
> > > > +  updateButton.waitThenClick();
> > > 
> > > Why this extra code? I do not see why we wanna do that. This is done
> > > automatically.
> > This is the check button in comment 50 you requested to move this outside of
> 
> In this comment I was talking about the assert but not that code.
The only code here was the asserting the node existed and clicking on it, waitThenClick was already handling this. I think it was a great advice to move it to the test. Since then we had lot's of review spins and was never spotlighted until now and I see no reason to rollback to having a method just for checking that the update button exists when we have waitThenClick, method on element which does that.

Comment 75

[Cosmin Malutan, [:cosmin-malutan]]

Hi Robert, looks like we have no observer whatsoever when opening "Update:Wizard" window, we would like to know if you now any or a way to get notified when it's opened.
The code we use to open the window is:
> Components.classes["@mozilla.org/updates/update-prompt;1"].createInstance(Components.interfaces.nsIUpdatePrompt).checkForUpdates()

Do you know why we don't get an "toplevel-window-ready" observer topic notified when the window gets opened like we get for any other window?

Comment 76

[Robert Strong (they/them - no direct email)]

The app update tests use the following observer which uses a load event listener and your tests should be able to use something like this.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/chrome/utils.js#1438

Comment 77

[Cosmin Malutan, [:cosmin-malutan]]

Robert it works but not as expected looks like we miss the event when we invalidate the update and open the Update:Wizard page. 
I attached an tescase in bug 1099069.

Whimboo, considering that this might be a bug in Firefox, should we block this bug again or go with the already existing implementation for handling the Update:Wizard window with windows.handleWindow?

Comment 78

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I explained it on the other bug. So this is a special situation which we do not have that often. It needs special casing, so waitForWindowState is fine and doesn't need a change. So your current approach should be right by using handleWindow.

Comment 79

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8521387 [details] [diff] [review]
patch v7.1

(In reply to Henrik Skupin (:whimboo) from comment #78)
> I explained it on the other bug. So this is a special situation which we do
> not have that often. It needs special casing, so waitForWindowState is fine
> and doesn't need a change. So your current approach should be right by using
> handleWindow.
Then I'll set the review flags back.

Comment 80

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8521387 [details] [diff] [review]
patch v7.1

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

I haven't tested this as it doesn't apply anymore, please update the patch so I can do some tests.

::: firefox/lib/ui/update-wizard.js
@@ +204,5 @@
> + *
> + * @param {Number} aTimeout
> + *        Timeout the download has to stop
> + */
> +UpdateWizardWindow.prototype.waitforDownloadFinished = function UpdateWizardWindow_waitForDownloadFinished(aTimeout) {

I would use something shorter here as we do with the other libs, like function UWW_waitForDownloadFinished.

Comment 81

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 599290.patch

I've done the update. 
Thanks 

http://mozmill-crowd.blargon7.com/#/update/report/635fcffb06b246be47416301bd662657
http://mozmill-crowd.blargon7.com/#/update/report/635fcffb06b246be47416301bd663b78

Comment 82

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8525989 [details] [diff] [review]
599290.patch

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

Tested well on OSX too.

Comment 83

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8525989 [details] [diff] [review]
599290.patch

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

So I checked this patch and I can see that a lot of work has been spent to get it into that state. But there are tons of missing or broken things in that patch. I'm not sure why none of those have been seen by the last review. IMO it will still take time to get this patch finished up.

When you have updated the patch, please run example tests. Also please let me know what exactly you tested. Thanks.

::: firefox/lib/ui/about.js
@@ +12,5 @@
> +
> +const TIMEOUT_UPDATE_DOWNLOAD = 360000;
> +
> +/**
> + * @class Constructor for AboutWindow class

Please add this as description and not as part of the  @class declaration.

@@ +16,5 @@
> + * @class Constructor for AboutWindow class
> + * @constructor
> + */
> +function AboutWindow(aController) {
> +  this._controller = aController || null;

I do not see where you call the constructor of the base window.

@@ +28,5 @@
> + * Checks if updates were found
> + *
> + * @returns {Boolean} True if updates were found
> + */
> +AboutWindow.prototype.__defineGetter__('updatesFound', function() {

nit: missing blank after function

@@ +36,5 @@
> +/**
> + * Returns the duration of the download
> + *
> + * @returns {Number}
> + *          Returns the download duration in milliseconds

All in one line please. "Download duration in milliseconds" should also be enough.

@@ +45,5 @@
> +
> +/**
> + * Determine current state of updates
> + *
> + * @returns {String} The Id of the actual enabled element in updateDeck

Maybe you can give examples of what this can be?

@@ +47,5 @@
> + * Determine current state of updates
> + *
> + * @returns {String} The Id of the actual enabled element in updateDeck
> + */
> +AboutWindow.prototype.__defineGetter__('wizardState', function() {

nit: missing blank.

@@ +49,5 @@
> + * @returns {String} The Id of the actual enabled element in updateDeck
> + */
> +AboutWindow.prototype.__defineGetter__('wizardState', function() {
> +  var deck = this.getElement({type: "updateDeck"});
> +  deck.waitForElement();

Why do we need waitForElement?

@@ +51,5 @@
> +AboutWindow.prototype.__defineGetter__('wizardState', function() {
> +  var deck = this.getElement({type: "updateDeck"});
> +  deck.waitForElement();
> +
> +  return deck.getNode().childNodes[deck.getNode().selectedIndex].id;

Please do not use childNodes but selectedPanel.id to retrieve the value.

@@ +55,5 @@
> +  return deck.getNode().childNodes[deck.getNode().selectedIndex].id;
> +});
> +
> +
> +/**

nit: extra blank line

@@ +56,5 @@
> +});
> +
> +
> +/**
> + * Download the update of the given channel and type

There is no given channel and type.

@@ +59,5 @@
> +/**
> + * Download the update of the given channel and type
> + *
> + * @param {Boolean} aWaitForFinish
> + *        Sets if the function should wait until the download has been finished

This is optional, so please reflect it here.

@@ +61,5 @@
> + *
> + * @param {Boolean} aWaitForFinish
> + *        Sets if the function should wait until the download has been finished
> + * @param {Number} [aTimeout=600s]
> + *        Timeout the download has to stop

Please fix the wording to make it better to understand

@@ +67,5 @@
> +AboutWindow.prototype.download = function AboutWindow_download(aWaitForFinish, aTimeout) {
> +  var waitForFinish = (aWaitForFinish === undefined) ? true : aWaitForFinish;
> +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +
> +  if (waitForFinish) {

I do not see where the download is getting triggered. This method is called "download". Also where is the check for the correct update channel?

Lots of things are missed here so please compare with the old code carefully.

@@ +70,5 @@
> +
> +  if (waitForFinish) {
> +    var startTime = Date.now();
> +    this.waitForDownloadFinished(timeout);
> +

What happens if incompatible add-ons are installed? The old update check ui should open.

@@ +73,5 @@
> +    this.waitForDownloadFinished(timeout);
> +
> +    assert.waitFor(() => {
> +
> +      // The OR statement is a fallback for older nightly versions

nit: why the extra empty line above?

Also which Nightlies would need this fallback? I haven't seen a change recently.

@@ +115,5 @@
> + * Waits until checking for updates is done
> + */
> +AboutWindow.prototype.waitForCheckFinished = function AboutWindow_waitForCheckFinished() {
> +  assert.waitFor(() => this.wizardState !== "checkingForUpdates",
> +                 "An update has been found.");

This message is wrong. Especially in cases when no update has been found, but the page has still been changed.

Also what happened with the timeout constant we had before? We should not abort after 5s.

@@ +121,5 @@
> +
> +/**
> + * Waits until downloading is completed
> + *
> + * @param {Number} aTimeout

This is optional.

@@ +128,5 @@
> +AboutWindow.prototype.waitForDownloadFinished = function AboutWindow_waitForDownloadFinished(aTimeout) {
> +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +
> +  assert.waitFor(() => !!this.softwareUpdate.activeUpdate,
> +                 "An active update has been found", aTimeout);

I'm sure you want 'timeout' here.

@@ +131,5 @@
> +  assert.waitFor(() => !!this.softwareUpdate.activeUpdate,
> +                 "An active update has been found", aTimeout);
> +
> +  assert.waitFor(() => !!this.softwareUpdate.activeUpdate.state.contains("applied"),
> +                 "Download has been completed and applied", aTimeout);

Here too, but which should have a different timeout value.

@@ +134,5 @@
> +  assert.waitFor(() => !!this.softwareUpdate.activeUpdate.state.contains("applied"),
> +                 "Download has been completed and applied", aTimeout);
> +
> +  assert.notEqual(this.wizardState, "downloadFailed",
> +                  "Download has been done successfully");

Can we do this before the apply waitfor call? If the download failed I don't want that we hang in that waitFor unnecessary.

@@ +140,5 @@
> +
> +/**
> + * Open the About window
> + *
> + * @param {function} aCalback

nit: Callback

@@ +141,5 @@
> +/**
> + * Open the About window
> + *
> + * @param {function} aCalback
> + *        Callback function that opens the Browser:About Window

A callback is implicitly a function. Also just 'opens the about window'.

::: firefox/lib/ui/browser.js
@@ +112,5 @@
> + * Open the About window
> + *
> + * @param {object} [aSpec]
> + *        Information for opening the window
> + * @param {string} [aSpec.type="menu"|"callback"]

Possible values are part of the description but not here. I mentioned this a lot over the last weeks. Please remember that in the future.

@@ +116,5 @@
> + * @param {string} [aSpec.type="menu"|"callback"]
> + *        How to open the About Window
> + */
> +BrowserWindow.prototype.openAboutWindow = function BW_openAboutWindow(aSpec={}) {
> +  var about = require("about");

Please move this up to the top. We have not talked about including modules inside of those open methods. If we wanna do this we should do it generally in a new bug.

@@ +126,5 @@
> +        this._controller.mainMenu.click("#aboutName");
> +        break;
> +      case "callback":
> +        assert.ok(aSpec.callback && (typeof aSpec.callback === "function"),
> +                  "A callback for opening About Window has ben provided.");

I don't see why we have to explicitly check for 'aSpec.callback'

@@ +158,5 @@
> +
> +  var callback = () => {
> +    updatePrompt.checkForUpdates();
> +  };
> +  return updateWizard.open(callback);

You can directly specify the callback code in the open call.

::: firefox/lib/ui/update-wizard.js
@@ +60,5 @@
> +  }
> +}
> +
> +/**
> + * @class Constructor for UpdateWizardWindow class

Please add this as the description and not as part of the @class declaration.

@@ +64,5 @@
> + * @class Constructor for UpdateWizardWindow class
> + * @constructor
> + */
> +function UpdateWizardWindow(aController) {
> +  this._controller = aController || null;

I do not see where you call the constructor of the base window.

Also please please keep the known DTDs for this window and do not remove them.

@@ +103,5 @@
> +  return this._downloadDuration;
> +});
> +
> +/**
> + * Download the update of the given channel and type

There is no given channel or type.

@@ +106,5 @@
> +/**
> + * Download the update of the given channel and type
> + *
> + * @param {Boolean} aWaitForFinish
> + *        Sets if the function should wait until the download has been finished

This is optional.

@@ +108,5 @@
> + *
> + * @param {Boolean} aWaitForFinish
> + *        Sets if the function should wait until the download has been finished
> + * @param {Number} aTimeout
> + *        Timeout the download has to stop

This is also optional.

@@ +113,5 @@
> + */
> +UpdateWizardWindow.prototype.download = function UWW_download(aWaitForFinish, aTimeout) {
> +  var waitForFinish = aWaitForFinish || true;
> +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +

I miss the check for the correct update channel.

@@ +121,5 @@
> +
> +  // Click 'Next' and wait until the next page has been selected
> +  next.click();
> +  assert.waitFor(() => this.currentPage !== page,
> +                 "Update available page has been processed.");

Mind adding single quotes around the "update available page"? That reads much better.

@@ +135,5 @@
> +  // page gets directly shown. Make sure we react correctly for different pages.
> +  switch (this.currentPage) {
> +    case WIZARD_PAGES.downloading:
> +      if (waitForFinish) {
> +        var startTime = Date.now();

Please leave this and other code as much as possible at the old locations.

@@ +143,5 @@
> +                             this.currentPage === WIZARD_PAGES.finishedBackground,
> +                       "Final wizard page has been selected. " + this.currentPage);
> +
> +        // Calculate the duration in ms
> +        this._downloadDuration = Date.now() - startTime;

This has to be calculated outside. Same comment as above applies here.

@@ +194,5 @@
> +
> +/**
> + * Wait that check for updates has been finished
> + *
> + * @param {number} timeout

This is optional and lacks a description.

@@ +207,5 @@
> +/**
> + * Waits for the given page of the update dialog wizard
> + */
> +UpdateWizardWindow.prototype.waitForWizardPage = function UWW_waitForWizardPage(aStep) {
> +  dump("\n aStep = "+aStep+"\n");

You missed to remove that?

@@ +216,5 @@
> +/**
> + * Wait until the download has been finished
> + *
> + * @param {Number} aTimeout
> + *        Timeout the download has to stop

Timeout should be optional.

@@ +236,5 @@
> +
> +/**
> + * Open the Update Wizard window
> + *
> + * @param {function} aCalback

nit: aCallback.

@@ +237,5 @@
> +/**
> + * Open the Update Wizard window
> + *
> + * @param {function} aCalback
> + *        Callback function that opens the Update:Wizard Window

Similar to the about window module.

@@ +242,5 @@
> + *
> + * @returns {UpdateWizardWindow} New instance of UpdateWizardWindow
> + */
> +function open(aCallback) {
> +  if (aCallback && (typeof aCallback === "function")) {

Why the check for aCallback? If the parameter is required you should assert for it. Without an action the window will not open.

::: firefox/tests/functional/restartTests/testSoftwareUpdateAutoProxy/test2.js
@@ +10,2 @@
>  var softwareUpdate = require("../../../../lib/software-update");
> +var updateWizard = require("../../../../lib/ui/update-wizard");

Please separate ui and api modules.

@@ +10,4 @@
>  var softwareUpdate = require("../../../../lib/software-update");
> +var updateWizard = require("../../../../lib/ui/update-wizard");
> +
> +const WIZARD_PAGES = updateWizard.WIZARD_PAGES;

I don't see why this is helpful. Use it directly in the test.

@@ +36,5 @@
>   * Performs a check for a software update failure: 'Update XML file malformed (200)'
>   */
>  function testSoftwareUpdateAutoProxy() {
>    // Open the software update dialog and wait until the check has been finished
> +  var updateWizard = browserWindow.openUpdateWizard();

You have already imported the ui module as updateWizard, so it will be overwritten here.

::: firefox/tests/update/testDirectUpdate/testUpdate.js
@@ +4,5 @@
>  
>  "use strict";
>  
>  // Include required modules
> +var browser = require("../../../lib/ui/browser");

Please separate ui and api modules.

@@ +67,5 @@
>  
> +  // Wait until the check has been finished
> +  var updateButton = aboutWindow.getElement({type: "updateButton"});
> +  updateButton.waitThenClick();
> +  aboutWindow.waitForCheckFinished();

This is a repeated block of code, which should all most likely covered in waitForCheckFinished(). Even better to rename this method to checkForUpdates()?

@@ +73,5 @@
>    try {
>      // If an update has been found, download the patch
> +    assert.waitFor(() => aboutWindow.updatesFound, "An update has been found");
> +    aboutWindow.download();
> +    aboutWindow.close();

Keep in mind that the about window will stay open if no updates are found. You might want to add this to finally?

@@ +78,5 @@
>    }
>    finally {
>      // Store details about the patch
> +    var patch = update.patchInfo;
> +    patch["download_duration"] = aboutWindow.downloadDuration;

I totally do not see why this has to be part of the test. This can greatly be done inside the ui class, which itself can extend patchInfo.

@@ +96,5 @@
>    // Open the software update dialog and wait until the check has been finished
> +  var aboutWindow = browserWindow.openAboutWindow();
> +
> +  var updateButton = aboutWindow.getElement({type: "updateButton"});
> +  updateButton.waitThenClick();

Just call click(). The button is always present.

Comment 84

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v8.0

(In reply to Henrik Skupin (:whimboo) from comment #83)
> When you have updated the patch, please run example tests. Also please let
> me know what exactly you tested. Thanks.
As usual, I ran complete testruns, and the functional testSoftwareUpdateAutoProxy:
http://mozmill-crowd.blargon7.com/#/update/report/4baf975f17cb2c8a74373dc7dd05f19b 
http://mozmill-crowd.blargon7.com/#/update/report/4baf975f17cb2c8a74373dc7dd057ced


> @@ +67,5 @@
> > +AboutWindow.prototype.download = function AboutWindow_download(aWaitForFinish, aTimeout) {
> > +  var waitForFinish = (aWaitForFinish === undefined) ? true : aWaitForFinish;
> > +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> > +
> > +  if (waitForFinish) {
> 
> I do not see where the download is getting triggered. This method is called
> "download". Also where is the check for the correct update channel?
We used to check the channel that came as a parameter, and as we removed thought that we shouldn't check it anymore, but I looked over the wizard pages, and I added that check.

> @@ +70,5 @@
> > +
> > +  if (waitForFinish) {
> > +    var startTime = Date.now();
> > +    this.waitForDownloadFinished(timeout);
> > +
> 
> What happens if incompatible add-ons are installed? The old update check ui
> should open.
In comment 47 I said we should file a new bug, should we handle it in this patch? 

> @@ +73,5 @@
> > +    this.waitForDownloadFinished(timeout);
> > +
> > +    assert.waitFor(() => {
> > +
> > +      // The OR statement is a fallback for older nightly versions
> 
> nit: why the extra empty line above?
> 
> Also which Nightlies would need this fallback? I haven't seen a change
> recently.
It's not a recent change, I in the early stages of About:Window it was different, this condition is for older versions support.


> @@ +116,5 @@
> > + * @param {string} [aSpec.type="menu"|"callback"]
> > + *        How to open the About Window
> > + */
> > +BrowserWindow.prototype.openAboutWindow = function BW_openAboutWindow(aSpec={}) {
> > +  var about = require("about");
> 
> Please move this up to the top. We have not talked about including modules
> inside of those open methods. If we wanna do this we should do it generally
> in a new bug.
In comment 70 inside the review you said it would be better to make them part of the method so we will import them only when we need it, if you prefer to do this in different bug please say so, otherwise it's unclear.
 

> @@ +135,5 @@
> > +  // page gets directly shown. Make sure we react correctly for different pages.
> > +  switch (this.currentPage) {
> > +    case WIZARD_PAGES.downloading:
> > +      if (waitForFinish) {
> > +        var startTime = Date.now();
> 
> Please leave this and other code as much as possible at the old locations.
This code has to be added here to, we need it in both UI modules.

Comment 85

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Cosmin Malutan from comment #84)
> > > +AboutWindow.prototype.download = function AboutWindow_download(aWaitForFinish, aTimeout) {
> > > +  var waitForFinish = (aWaitForFinish === undefined) ? true : aWaitForFinish;
> > > +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> > > +
> > > +  if (waitForFinish) {
> > 
> > I do not see where the download is getting triggered. This method is called
> > "download". Also where is the check for the correct update channel?
> We used to check the channel that came as a parameter, and as we removed
> thought that we shouldn't check it anymore, but I looked over the wizard
> pages, and I added that check.

Only the parameters were removed because the init method tries to set the expected update channel now. But this does not prevent us to check if the expected channel has really been set. Also what about the download trigger? You haven't answered that part of my question.

> > > +  if (waitForFinish) {
> > > +    var startTime = Date.now();
> > > +    this.waitForDownloadFinished(timeout);
> > 
> > What happens if incompatible add-ons are installed? The old update check ui
> > should open.
> In comment 47 I said we should file a new bug, should we handle it in this
> patch? 

What is the reasoning of filing a new bug for that? What we need are working update tests, and your proposal here is against that. Whenever we have incompatible add-ons (which we hopefully don't have) our update tests won't be successful and break due to this situation. This is not acceptable. Please check by executing such a situation, what path we have to cover.

> > Also which Nightlies would need this fallback? I haven't seen a change
> > recently.
> It's not a recent change, I in the early stages of About:Window it was
> different, this condition is for older versions support.

Please answer my question here. If this is older then 31.0ESR why should we keep that?

> > > +  var about = require("about");
> > 
> > Please move this up to the top. We have not talked about including modules
> > inside of those open methods. If we wanna do this we should do it generally
> > in a new bug.
> In comment 70 inside the review you said it would be better to make them
> part of the method so we will import them only when we need it, if you
> prefer to do this in different bug please say so, otherwise it's unclear.

That comes when it takes a long time for the next review, and you forget about specifics. Sorry. If all of you agree on such a declaration we could keep it. Otherwise lets use the top of the file.

> > @@ +135,5 @@
> > > +  // page gets directly shown. Make sure we react correctly for different pages.
> > > +  switch (this.currentPage) {
> > > +    case WIZARD_PAGES.downloading:
> > > +      if (waitForFinish) {
> > > +        var startTime = Date.now();
> > 
> > Please leave this and other code as much as possible at the old locations.
> This code has to be added here to, we need it in both UI modules.

This was not referred to the different modules we need but the location inside the same method. At this location you would even miss to execute it in certain situation. Please think about that.

Comment 86

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #85) 
> That comes when it takes a long time for the next review, and you forget
> about specifics. Sorry. If all of you agree on such a declaration we could
> keep it. Otherwise lets use the top of the file.

This bug waited for 4,5 months for a review from you, from 2014-05-06 until 2014-09-23 when you removed the flag because our codebase has changed in between.

Comment 87

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrei Eftimie from comment #86)
> This bug waited for 4,5 months for a review from you, from 2014-05-06 until
> 2014-09-23 when you removed the flag because our codebase has changed in
> between.

This was not meant to offend Cosmin! It was telling my own story in terms of when I do the next review. Also see the included "sorry".

Comment 88

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v8.1

(In reply to Henrik Skupin (:whimboo) from comment #83)
> @@ +73,5 @@
> > +    this.waitForDownloadFinished(timeout);
> > +
> > +    assert.waitFor(() => {
> > +
> > +      // The OR statement is a fallback for older nightly versions
> 
> nit: why the extra empty line above?
> 
> Also which Nightlies would need this fallback? I haven't seen a change
> recently.
Older than Esr31, I though we would need it while testing older branches for regression range testing, but you're right, It makes no sense to cover all changes that happened. 


Here are the testrun reports:
http://mozmill-crowd.blargon7.com/#/update/report/1d4125666f4752482c2ebb69d90aa0fb
http://mozmill-crowd.blargon7.com/#/update/report/1d4125666f4752482c2ebb69d90a884a
With incompatible addons for testing with inconpatible addons, I applied the patch from bug 1100346:
http://mozmill-crowd.blargon7.com/#/update/report/1d4125666f4752482c2ebb69d908e745
http://mozmill-crowd.blargon7.com/#/update/report/1d4125666f4752482c2ebb69d9088a71

Comment 89

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So I think you will follow-up here with an updated patch?

Comment 90

[Cosmin Malutan, [:cosmin-malutan]]

Yes, I still have some things to do. But it's a working patch, good enough for checking the fix from the blocking bug.

Comment 91

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v10.0

I expect this to have other review spins, but I would like to have Henrik's feedback on implementation sooner.
As it's clearly that we won't get rid of update wizard as we relay on it for fallback update and for updating while we have incompatible addons, I completely refactored the library so it has the same structure as the new ones and I got rid of lookup expressions.
In update test we had some try/finally blocks,to set the patchInfo on the persisted object. We don't need those if in download method we don't assert but we expect.
For handling incompatible addons while in the new UI, I had to import the old UI and expect it to open  when we click on applyBillboard button.


Reports:
Regular update
http://mozmill-crowd.blargon7.com/#/update/report/dd0cbbf05a3aab4a7676502a8640a20d
http://mozmill-crowd.blargon7.com/#/update/report/dd0cbbf05a3aab4a7676502a86408903

Incompatible addons
http://mozmill-crowd.blargon7.com/#/update/report/dd0cbbf05a3aab4a7676502a8642a00d
http://mozmill-crowd.blargon7.com/#/update/report/dd0cbbf05a3aab4a7676502a864244af

Comment 92

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: Screenshot from 2015-01-19 17:20:38.png

By the way, the current implementation does't supports updating with incompatible addons since app.update.altwindowtype preference is default to Browser:About.
This causes an unhandled modal dialog to be raised, see the attached image.

Comment 93

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Cosmin Malutan, [:cosmin-malutan] from comment #92)
> By the way, the current implementation does't supports updating with
> incompatible addons since app.update.altwindowtype preference is default to
> Browser:About.
> This causes an unhandled modal dialog to be raised, see the attached image.

Thanks for the information. I don't think that we should be affected by this given that we only allow extension of very specific scope.

Comment 94

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8551315 [details] [diff] [review]
patch v10.0

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

This is going the right direction. Keep in mind that I only mention the most important things to continue.

::: firefox/lib/software-update.js
@@ -48,5 @@
> -  errorPatching: 'errorpatching',
> -  finished: 'finished',
> -  finishedBackground: 'finishedBackground',
> -  installed: 'installed'
> -}

Why are you removing those? I think those still apply and we should not use hard-coded strings in the code.

@@ -74,5 @@
> -    finish: 'anon({"anonid":"WizardButtonDeck"})/[0]/{"dlgtype":"finish"}',
> -    extra1: '{"dlgtype":"extra1"}',
> -    extra2: '{"dlgtype":"extra2"}'
> -  }
> -}

Hurray! :)

::: firefox/lib/ui/about.js
@@ +20,5 @@
> + * @class Constructor for AboutWindow class
> + * @constructor
> + */
> +function AboutWindow(aController) {
> +  this._controller = aController || null;

You want to call the base window constructor.

@@ +53,5 @@
> + * @returns {object} The information of the active update
> + */
> +AboutWindow.prototype.__defineGetter__('patchInfo', function () {
> +  var patch = this.softwareUpdate.patchInfo;
> +  patch.download_duration = this._downloadDuration;

We should initialize _downloadDuration to maybe -1 in the constructor.

@@ +67,5 @@
> +AboutWindow.prototype.__defineGetter__('wizardState', function() {
> +  var deck = this.getElement({type: "updateDeck"});
> +  deck.waitForElement();
> +
> +  return deck.getNode().childNodes[deck.getNode().selectedIndex].id;

Doesn't offer the deck a selected property? I don't like that we have to go through childNodes here.

@@ +83,5 @@
> +AboutWindow.prototype.download = function AboutWindow_download(aWaitForFinish, aTimeout) {
> +  var waitForFinish = (aWaitForFinish === undefined) ? true : aWaitForFinish;
> +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +
> +  if (this.wizardState === "downloadAndInstall") {

I would like to see constants for the states.

::: firefox/lib/ui/browser.js
@@ +287,5 @@
> +  var callback = () => {
> +    updatePrompt.checkForUpdates();
> +  };
> +  return updateWizard.open(callback);
> +};

Do we really need that? I would not expose it on the browser class given that this is a deprecated dialog and would come up automatically in case of add-on incompatibilities.

::: firefox/lib/ui/update-wizard.js
@@ +29,5 @@
> +  assert.waitFor(() => this.currentPage !== "dummy",
> +                 "Dummy wizard page has been made invisible");
> +}
> +
> +UpdateWizardWindow.prototype = Object.create(baseWindow.BaseWindow.prototype);

This is actually a dialog, right? So we could let it base on that new class.

@@ +147,5 @@
> +      var nodeCollector = new domUtils.nodeCollector(wizard.getNode());
> +      nodeCollector.queryAnonymousNode("anonid", "Buttons");
> +      nodeCollector.root = nodeCollector.nodes[0];
> +      nodeCollector.queryAnonymousNode("dlgtype", spec.subtype);
> +      elements = nodeCollector.elements;

The dialog class should give us the buttons already, so we can save this element in that class.

::: firefox/tests/addons/[e-mail]/addon.ini
@@ -1,4 @@
> -[download]
> -linux=http://release.seleniumhq.org/selenium-ide/editor/2.7.0/selenium-ide-editor-2.7.0.xpi
> -mac=http://release.seleniumhq.org/selenium-ide/editor/2.7.0/selenium-ide-editor-2.7.0.xpi
> -win=http://release.seleniumhq.org/selenium-ide/editor/2.7.0/selenium-ide-editor-2.7.0.xpi

You want to leave this file in-tact.

::: firefox/tests/functional/restartTests/testSoftwareUpdateAutoProxy/test2.js
@@ +34,5 @@
>   * Performs a check for a software update failure: 'Update XML file malformed (200)'
>   */
>  function testSoftwareUpdateAutoProxy() {
>    // Open the software update dialog and wait until the check has been finished
> +  var updateWizard = browserWindow.openUpdateWizard();

This test is about to get removed because it's not that helpful anymore. So lets open the window directly and not via the browser class.

::: firefox/tests/update/testDirectUpdate/testUpdate.js
@@ +74,3 @@
>  
> +  // Store details about the patch
> +  persisted.updates[persisted.update.index].patch = aboutWindow.patchInfo;

This won't longer store the patchInfo in case of failures in the check or download. Also what happened with the check for updatesFound?

Comment 95

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #94)
> ::: firefox/lib/software-update.js
> @@ -48,5 @@
> > -  errorPatching: 'errorpatching',
> > -  finished: 'finished',
> > -  finishedBackground: 'finishedBackground',
> > -  installed: 'installed'
> > -}
> 
> Why are you removing those? I think those still apply and we should not use
> hard-coded strings in the code.
What I had in minde when I removed this: 
1 - All keys have the exact same value as the key name.
2 - We would have to import the update-wizard module in places where we might nod need it but we need the WIZARD_PAGES constants object. 
I would go here as you think is the best, I just wanted to explain my reasoning.


> > +  // Store details about the patch
> > +  persisted.updates[persisted.update.index].patch = aboutWindow.patchInfo;
> 
> This won't longer store the patchInfo in case of failures in the check or
> download. Also what happened with the check for updatesFound?
I removed the assertions from the download method and we have only expects, so this will be ran as it would be in a try/catch block. Regarding the later check, is up to the next test to check that. Again here I don't have a strong opinion, but that's how I saw it.

Comment 96

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v11.0

I made some improvements to this patch, and I addressed Henriks requests, so I think this is in a good state for review-ing.

Comment 97

[Cosmin Malutan, [:cosmin-malutan]]

Here are some reports: 

Inconpatible addons:
http://mozmill-crowd.blargon7.com/#/update/report/52e1dd05f3d97380e39f88e69c40a2a6
http://mozmill-crowd.blargon7.com/#/update/report/52e1dd05f3d97380e39f88e69c40a6e5

Regular update:
http://mozmill-crowd.blargon7.com/#/update/report/52e1dd05f3d97380e39f88e69c40aae2
http://mozmill-crowd.blargon7.com/#/update/report/52e1dd05f3d97380e39f88e69c40b5de
http://mozmill-crowd.blargon7.com/#/update/report/52e1dd05f3d97380e39f88e69c409c29
http://mozmill-crowd.blargon7.com/#/update/report/52e1dd05f3d97380e39f88e69c40a186

Up to date buld:
http://mozmill-crowd.blargon7.com/#/update/report/52e1dd05f3d97380e39f88e69c40c1ed
http://mozmill-crowd.blargon7.com/#/update/report/52e1dd05f3d97380e39f88e69c40cf3f

Comment 98

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Cosmin Malutan, [:cosmin-malutan] from comment #95)
> > Why are you removing those? I think those still apply and we should not use
> > hard-coded strings in the code.
> What I had in minde when I removed this: 
> 1 - All keys have the exact same value as the key name.
> 2 - We would have to import the update-wizard module in places where we
> might nod need it but we need the WIZARD_PAGES constants object. 
> I would go here as you think is the best, I just wanted to explain my
> reasoning.

You never used hard-coded strings when those are in use in various places. Here in the same and other modules, or in tests. Maintaining this list can become a nightmare of values are changing. Having a single location to change is what you always should do.

> > > +  // Store details about the patch
> > > +  persisted.updates[persisted.update.index].patch = aboutWindow.patchInfo;
> > 
> > This won't longer store the patchInfo in case of failures in the check or
> > download. Also what happened with the check for updatesFound?
> I removed the assertions from the download method and we have only expects,
> so this will be ran as it would be in a try/catch block. Regarding the later
> check, is up to the next test to check that. Again here I don't have a
> strong opinion, but that's how I saw it.

Not being able to download or any other failure during download makes all the following tests useless. I don't see a reason why this has been changed. If there is, please really file a separate bug. Here we should really not change any kind of logic. The patch is already complex enough, so I don't want to deal with other regressions of such types. Thanks.

Comment 99

[Cosmin Malutan, [:cosmin-malutan]]

I agree with both, so they are in the patch set for review, thanks.

Comment 100

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8553737 [details] [diff] [review]
patch v11.0

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

The commit message needs the reviewers updated.

::: firefox/lib/ui/about.js
@@ +11,5 @@
> +
> +var dialogs = require("../../../lib/ui/dialogs");
> +var updateWizard = require("update-wizard");
> +
> +const TIMEOUT_UPDATE_CHECK     = 3000000;

nit: There is an extra blank space before the equal sign

@@ +14,5 @@
> +
> +const TIMEOUT_UPDATE_CHECK     = 3000000;
> +const TIMEOUT_UPDATE_DOWNLOAD = 360000;
> +
> +

nit: Extra blank line, please remove

@@ +16,5 @@
> +const TIMEOUT_UPDATE_DOWNLOAD = 360000;
> +
> +
> +/**
> + * @class Constructor for AboutWindow class

nit: * AboutWindow class

@@ +40,5 @@
> +});
> +
> +
> +/**
> + * Checks if updates were found

nit: Checks if restart is needed

@@ +42,5 @@
> +
> +/**
> + * Checks if updates were found
> + *
> + * @returns {Boolean} True if updates were found

nit: ... True if a restart is needed

@@ +49,5 @@
> +  return this.wizardState === "apply";
> +});
> +
> +/**
> + * Returns information of the active update in the queue.

nit: ... information about the ...

@@ +51,5 @@
> +
> +/**
> + * Returns information of the active update in the queue.
> + *
> + * @returns {object} The information of the active update

nit: ... information about ...

@@ +72,5 @@
> +
> +  return deck.getNode().selectedPanel.id;
> +});
> +
> +

nit: Extra blank line, please remove

@@ +74,5 @@
> +});
> +
> +
> +/**
> + * Download the update of the given channel and type

You don't specify a channel nor a type anywhere

@@ +79,5 @@
> + *
> + * @param {Boolean} [aWaitForFinish=true]
> + *        Sets if the function should wait until the download has been finished
> + * @param {Number} [aTimeout=600s]
> + *        Timeout the download has to stop

nit: Time until the download should finish

@@ +85,5 @@
> +AboutWindow.prototype.download = function AW_download(aWaitForFinish, aTimeout) {
> +  var waitForFinish = (aWaitForFinish === undefined) ? true : aWaitForFinish;
> +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +
> +  if (this.wizardState === "downloadAndInstall") {

Please use constants for the download states

@@ +106,5 @@
> +    return;
> +  }
> +
> +  if (waitForFinish) {
> +    var startTime = Date.now();

I think the start time should be initialized right after you start the download.

@@ +159,5 @@
> +                 "An update has been found.", TIMEOUT_UPDATE_CHECK);
> +};
> +
> +/**
> + * Waits until downloading is completed

nit: ... until download is ...

@@ +162,5 @@
> +/**
> + * Waits until downloading is completed
> + *
> + * @param {Number} [aTimeout=360000]
> + *        How long it will wait for downloading state to change

nit: The amount of time to wait for ...

@@ +181,5 @@
> +/**
> + * Open the About window
> + *
> + * @param {function} aCalback
> + *        Callback function that opens the Browser:About Window

nit: Callback that opens...

@@ +186,5 @@
> + *
> + * @returns {AboutWindow} New instance of AboutWindow
> + */
> +function open(aCallback) {
> +  assert.ok(aCallback, "Callback function has been provided");

nit: "Callback has been specified"

::: firefox/lib/ui/browser.js
@@ +322,5 @@
>  
>    return placesOrganizer.open(callback);
>  };
>  
> + /**

nit: You have a blank space before the comment

::: firefox/lib/ui/update-wizard.js
@@ +36,5 @@
> +
> +const PREF_APP_UPDATE_ALTWINDOWTYPE = "app.update.altwindowtype";
> +
> +/**
> + * @class Constructor for UpdateWizardWindow class

nit: * UpdateWizardWindow class

@@ +65,5 @@
> +  return wizard.getNode().getAttribute('currentpageid');
> +});
> +
> +/**
> + * Returns information of the active update in the queue.

nit: ... information about the ...

@@ +67,5 @@
> +
> +/**
> + * Returns information of the active update in the queue.
> + *
> + * @returns {object} The information of the active update

nit: ... information about the ...

@@ +95,5 @@
> +  dialogs.CommonDialog.prototype.close.call(this, aCallback);
> +};
> +
> +/**
> + * Download the update of the given channel and type

You don't use a specific channel or type here

@@ +100,5 @@
> + *
> + * @param {Boolean} [aWaitForFinish=false]
> + *        Sets if the function should wait until the download has been finished
> + * @param {Number} [aTimeout=360000]
> + *        Timeout the download has to stop

nit: Time until the download should finish

@@ +123,5 @@
> +  // page gets directly shown. Make sure we react correctly for different pages.
> +  switch (this.currentPage) {
> +    case WIZARD_PAGES.downloading:
> +      if (waitForFinish) {
> +        var startTime = Date.now();

Again, I think that startTime should be initialized right after you start the download

@@ +130,5 @@
> +        assert.waitFor(() => this.currentPage ===  WIZARD_PAGES.finished ||
> +                             this.currentPage === WIZARD_PAGES.finishedBackground,
> +                       "Final wizard page has been selected. " + this.currentPage);
> +
> +        // Calculate the duration in ms

nit: ... the download duration ...

@@ +175,5 @@
> +      break;
> +    case "wizard":
> +      elements = [findElement.ID(this.controller.window.document, "updates")];
> +      break;
> +    case "download_progress":

Please move this case before "wizard", to have them in order.

@@ +258,5 @@
> +
> +  return updateWizardWindow;
> +}
> +
> +// Export of variables

//Export of constants

@@ +260,5 @@
> +}
> +
> +// Export of variables
> +exports.WIZARD_PAGES = WIZARD_PAGES;
> +exports.UpdateWizardWindow =  UpdateWizardWindow;

This should be moved under a "//Export of classes" block

::: lib/dom-utils.js
@@ +679,5 @@
>    queryAnonymousNode : function nodeCollector_queryAnonymousNode(aAttribute, aValue) {
>      var node = this._document.getAnonymousElementByAttribute(this._root,
>                                                               aAttribute,
>                                                               aValue);
> +    if (!node) {

Please remove this

Comment 101

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v12.0

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #100)
> 
> @@ +106,5 @@
> > +    return;
> > +  }
> > +
> > +  if (waitForFinish) {
> > +    var startTime = Date.now();
> 
> I think the start time should be initialized right after you start the
> download.
This is the only place we could start, is right away after we triggered the download, and right after we checked incompatible addons(which will fallback to other download scenario).

Thanks for review Mihaela.

Comment 102

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8555139 [details] [diff] [review]
patch v12.0

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

Just a few nits, otherwise it looks good to me.

::: firefox/lib/ui/about.js
@@ +27,5 @@
> +  adminDisabled: "adminDisabled",
> +  noUpdatesFound: "noUpdatesFound",
> +  otherInstanceHandlingUpdates: "otherInstanceHandlingUpdates",
> +  manualUpdate: "manualUpdate",
> +  unsupportedSystem: "unsupportedSystem"

I suggest you order these alphabetically

::: firefox/lib/ui/update-wizard.js
@@ +30,5 @@
> +  errorextra: 'errorextra',
> +  errorPatching: 'errorpatching',
> +  finished: 'finished',
> +  finishedBackground: 'finishedBackground',
> +  installed: 'installed'

I suggest you order these alphabetically

@@ +100,5 @@
> +
> +/**
> + * Download the update
> + *
> + * @param {Boolean} [aWaitForFinish=false]

You set this to true in the function

Comment 103

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v13.0

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #102)
> ::: firefox/lib/ui/about.js
> @@ +27,5 @@
> > +  adminDisabled: "adminDisabled",
> > +  noUpdatesFound: "noUpdatesFound",
> > +  otherInstanceHandlingUpdates: "otherInstanceHandlingUpdates",
> > +  manualUpdate: "manualUpdate",
> > +  unsupportedSystem: "unsupportedSystem"
> 
> I suggest you order these alphabetically
I would really keep those constants in the order they are in the UI, just in case at some point we might want to compare the selectedIndex value.

Thanks

Reports:
Regular
http://mozmill-crowd.blargon7.com/#/update/report/6b1e6b9bf618fb20da2bf8727487065b
http://mozmill-crowd.blargon7.com/#/update/report/6b1e6b9bf618fb20da2bf872748719ae

Incompatible add-ons 
http://mozmill-crowd.blargon7.com/#/update/report/6b1e6b9bf618fb20da2bf8727486faa4
http://mozmill-crowd.blargon7.com/#/update/report/6b1e6b9bf618fb20da2bf8727487028a

No update
http://mozmill-crowd.blargon7.com/#/update/report/6b1e6b9bf618fb20da2bf87274870562
http://mozmill-crowd.blargon7.com/#/update/report/6b1e6b9bf618fb20da2bf87274871463

Comment 104

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8555701 [details] [diff] [review]
patch v13.0

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

::: firefox/lib/ui/about.js
@@ +38,5 @@
> + * @param {MozMillController} [aController]
> + *        MozMillController of About Window
> + */
> +function AboutWindow(aController) {
> +  dialogs.CommonDialog.call(this, aController);

The About window is not a dialog but a window. So not sure why this has been changed with the latest patch. I explicitly told it has to be based off BaseWindow.

@@ +87,5 @@
> +AboutWindow.prototype.__defineGetter__('wizardState', function() {
> +  var deck = this.getElement({type: "updateDeck"});
> +  deck.waitForElement();
> +
> +  return deck.getNode().selectedPanel.id;

Way better! Thanks.

@@ +100,5 @@
> + *        Timeout until the download should finish
> + */
> +AboutWindow.prototype.download = function AW_download(aWaitForFinish, aTimeout) {
> +  var waitForFinish = (aWaitForFinish === undefined) ? true : aWaitForFinish;
> +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;

Please assign the defaults directly in the parameter list.

@@ +101,5 @@
> + */
> +AboutWindow.prototype.download = function AW_download(aWaitForFinish, aTimeout) {
> +  var waitForFinish = (aWaitForFinish === undefined) ? true : aWaitForFinish;
> +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;
> +

I still do not see the check for the correct update channel. Please do not remove it! Same for the old dialog.

@@ +108,5 @@
> +    installButton.click();
> +    assert.waitFor(() => this.wizardState !== WIZARD_PAGES.downloadAndInstall);
> +  }
> +
> +  // If there are incompatible addons we fallback on downland wizard for updating

nit: downland. Also I think you want to say "old software update dialog", right?

@@ +126,5 @@
> +  if (waitForFinish) {
> +    var startTime = Date.now();
> +    this.waitForDownloadFinished(timeout);
> +    assert.waitFor(() => this.wizardState === WIZARD_PAGES.apply,
> +                   "Final wizard page has been selected. self.wizardState : " + this.wizardState);

The message for the state is useless because it will not be the value you expect. So remove all after the dot.

@@ +143,5 @@
> + *
> + * @returns {ElemBase[]} Elements which have been found
> + */
> +AboutWindow.prototype.getElements = function AW_getElements(aSpec) {
> +  var spec = aSpec || {};

Please assign it in the parameter list and update the jsdoc for the default.

@@ +188,5 @@
> +    return !!this.softwareUpdate.activeUpdate;
> +  }, "An active update has been found");
> +
> +  assert.waitFor(() => !!this.softwareUpdate.activeUpdate.state.contains("applied"),
> +                 "Download has been completed and applied", aTimeout);

So downloading and applying the update is a single wizard step? If that is the case the timeout you push in here will not work. Overall you only wait 6min including the apply step. Formerly we had an additional wait for call for the applying step. I would favor to have both of them separate.

@@ +197,5 @@
> +
> +/**
> + * Open the About window
> + *
> + * @param {function} aCalback

nit: spelling mistake.

@@ +203,5 @@
> + *
> + * @returns {AboutWindow} New instance of AboutWindow
> + */
> +function open(aCallback) {
> +  assert.ok(aCallback, "Callback has been provided");

Assert that it is a function.

@@ +205,5 @@
> + */
> +function open(aCallback) {
> +  assert.ok(aCallback, "Callback has been provided");
> +
> +  var controller = windows.waitForWindowState(() => { aCallback(); },

I don't see why we need another wrapper around the callback. You should be able to pass it in directly.

::: firefox/lib/ui/update-wizard.js
@@ +33,5 @@
> +  finishedBackground: 'finishedBackground',
> +  installed: 'installed'
> +};
> +
> +const PREF_APP_UPDATE_ALTWINDOWTYPE = "app.update.altwindowtype";

I have no idea what this is, especially seeing no comment at all for it.

@@ +93,5 @@
> + * @param {function} aCallback
> + *        Define new function that closes the window
> + */
> +UpdateWizardWindow.prototype.close = function UWW_close(aCallback) {
> +  prefs.clearUserPref(PREF_APP_UPDATE_ALTWINDOWTYPE);

This pref will be left around if close() is not getting called. So I really would like to know why we need it now.

@@ +107,5 @@
> + *        Time until the download should finish
> + */
> +UpdateWizardWindow.prototype.download = function UWW_download(aWaitForFinish, aTimeout) {
> +  var waitForFinish = aWaitForFinish || true;
> +  var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;

Assign default values in the parameter section

@@ +115,5 @@
> +      this.currentPage === WIZARD_PAGES.updatesFoundBillboard ||
> +      this.currentPage === WIZARD_PAGES.errorPatching) {
> +    this.next();
> +  }
> +

I don't see the check for the correct update channel.

@@ +141,5 @@
> +    case WIZARD_PAGES.finished:
> +    case WIZARD_PAGES.finishedBackground:
> +      break;
> +    default:
> +      assert.fail("No handler for wizard page: " + this.currentPage);

"Invalid wizard page for downloading an update ..."

@@ +174,5 @@
> +    case "buttons":
> +      nodeCollector.root = this.getElement({type: "wizard"}).getNode();
> +      nodeCollector.queryAnonymousNode("anonid", "Buttons");
> +      elements = nodeCollector.elements;
> +      break;

Aren't those all common dialog elements?

@@ +190,5 @@
> +
> +  return elements;
> +};
> +
> +UpdateWizardWindow.prototype.next = function UMW_next() {

next is not an action so not a valid function name.

@@ +196,5 @@
> +  var next = this.getElement({type: "button", subtype: "next"});
> +  var page = this.currentPage;
> +
> +  // Click 'Next' and wait until the next page has been selected
> +  next.waitThenClick();

Why do we need waitThenClick?

@@ +210,5 @@
> + *        Timeout for the page to change
> + */
> +UpdateWizardWindow.prototype.waitForWizardPage = function UWW_waitForWizardPage(aStep, aTimeout=30000) {
> +  assert.waitFor(() => this.currentPage === aStep,
> +                 "New wizard page has been selected", aTimeout);

Lets add which page we were waiting for.

@@ +224,5 @@
> + */
> +UpdateWizardWindow.prototype.waitForWizardPageChange = function UWW_waitForWizardPageChange(aStep, aTimeout=30000) {
> +  assert.waitFor(() => this.currentPage !== aStep,
> +                 "Wizard page has changed: " + this.currentPage + " != " + aStep, aTimeout);
> +};

Ńame this method waitForWizardPageChanged. Also listing this.currentPage here is useless as mentioned a lot in the past. It will not contain the value you expect.

@@ +232,5 @@
> + *
> + * @param {Number} aTimeout
> + *        Timeout the download has to stop
> + */
> +UpdateWizardWindow.prototype.waitforDownloadFinished = function UWW_waitForDownloadFinished(aTimeout) {

The timeout needs a default here.

@@ +243,5 @@
> +
> +/**
> + * Open the Update Wizard window
> + *
> + * @param {function} [aCalback]

nit: callback

@@ +244,5 @@
> +/**
> + * Open the Update Wizard window
> + *
> + * @param {function} [aCalback]
> + *        Callback function that opens the Update:Wizard Window, it's optional

I feel that i have to mention it each time we use callback... "function" is not necessary here.

@@ +246,5 @@
> + *
> + * @param {function} [aCalback]
> + *        Callback function that opens the Update:Wizard Window, it's optional
> + *        because the window might be already open, in which case, we just pull
> + *        the window and instantiate the class

No this is not what open() should do. Instead you want to create an instance of the class with the correct controller.

@@ +258,5 @@
> +
> +  var controller = windows.handleWindow("type", "Update:Wizard", undefined, false);
> +  var updateWizardWindow = new UpdateWizardWindow(controller);
> +
> +  return updateWizardWindow;

you can return directly.

::: firefox/tests/functional/restartTests/testSoftwareUpdateAutoProxy/test2.js
@@ +40,5 @@
> +    var updatePrompt = Cc["@mozilla.org/updates/update-prompt;1"]
> +                       .createInstance(Ci.nsIUpdatePrompt);
> +    updatePrompt.checkForUpdates();
> +  });
> +  update.waitForWizardPageChange(updateWizard.WIZARD_PAGES.checking);

You don't want to wait for checking but for the check being finished.

@@ +45,2 @@
>  
> +  expect.notEqual(update.currentPage, "errors",

No hard-coded strings here too.

::: firefox/tests/update/testDirectUpdate/testUpdate.js
@@ +110,5 @@
>    // Check that updates have been applied correctly
>    update.assertUpdateApplied(persisted);
>  
>    // Sanity check the about dialog
> +  aboutWindow.waitForCheckFinished();

This is not what we tested before.

::: firefox/tests/update/testFallbackUpdate/testUpdate.js
@@ +85,5 @@
>  function testUpdateNotAppliedAndDownloadComplete() {
>    persisted.nextTest = "testUpdateAppliedNoOtherUpdate";
>  
>    // The dialog should be open in the background and shows a failure
> +  var wizard = updateWizard.open();

open() reflects that the window will be opened here by test code, but that is not what we want to have here. This window opens automatically.

@@ +99,5 @@
>  
> +    //Checks if the update button exist, and clicks on it
> +    var updateButton = aboutWindow.getElement({type: "updateButton"});
> +    updateButton.waitThenClick();
> +    aboutWindow.waitForCheckFinished();

I would have encapsulated all those lines into AboutWindow.checkForUpdate(). We have those quite often across the tests.

@@ +120,5 @@
> +      wizard.close();
> +    }
> +    finally {
> +      // Store details about the patch
> +      persisted.updates[persisted.update.index].patch_fallback = wizard.patchInfo;

I don't want to see the same finally code twice. The try/catch would work perfectly around the if/else.

@@ +133,5 @@
>  function testUpdateAppliedNoOtherUpdate() {
>    // Collect some data of the current (updated) build
>    persisted.updates[persisted.update.index].build_post = update.buildInfo;
>  
>    // Open the software update dialog and wait until the check has been finished

The comment does not apply anymore. Same on other spots in the tests.

Comment 105

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #104) 
> > +function AboutWindow(aController) {
> > +  dialogs.CommonDialog.call(this, aController);
> 
> The About window is not a dialog but a window. So not sure why this has been
> changed with the latest patch. I explicitly told it has to be based off
> BaseWindow.
At the time you said that we had no plans for CommonDialog class, I guess that's why we decided to inherit form BaseWindow, but the feature says about itself that it's a dialog:
http://hg.mozilla.org/mozilla-central/file/29b05d283b00/browser/base/content/aboutDialog.xul
 

> ::: firefox/lib/ui/update-wizard.js
> @@ +33,5 @@
> > +  finishedBackground: 'finishedBackground',
> > +  installed: 'installed'
> > +};
> > +
> > +const PREF_APP_UPDATE_ALTWINDOWTYPE = "app.update.altwindowtype";
> 
> I have no idea what this is, especially seeing no comment at all for it.
> 
> @@ +93,5 @@
> > + * @param {function} aCallback
> > + *        Define new function that closes the window
> > + */
> > +UpdateWizardWindow.prototype.close = function UWW_close(aCallback) {
> > +  prefs.clearUserPref(PREF_APP_UPDATE_ALTWINDOWTYPE);
> 
> This pref will be left around if close() is not getting called. So I really
> would like to know why we need it now.
 Since the About:Dialog is the default way for updating,
 for errors it will rise new prompt dialogs, but in case the Update:Wizard is open
 we expect the error to be displayed in-window, so when the Update:Wizard is open
 we need to let the browser know, by setting this preference:
 http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js?rev=a9240b1eb2fb#4813
 http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js?rev=a9240b1eb2fb#4756
 


> @@ +174,5 @@
> > +    case "buttons":
> > +      nodeCollector.root = this.getElement({type: "wizard"}).getNode();
> > +      nodeCollector.queryAnonymousNode("anonid", "Buttons");
> > +      elements = nodeCollector.elements;
> > +      break;
> 
> Aren't those all common dialog elements?
We have an wrapper and also we have different buttons without ID, like "Next", "Back", "Finish"

> Ńame this method waitForWizardPageChanged. Also listing this.currentPage
> here is useless as mentioned a lot in the past. It will not contain the
> value you expect.
this.currentPage lists the actual value and the aStep lists the expected value.

> @@ +120,5 @@
> > +      wizard.close();
> > +    }
> > +    finally {
> > +      // Store details about the patch
> > +      persisted.updates[persisted.update.index].patch_fallback = wizard.patchInfo;
> 
> I don't want to see the same finally code twice. The try/catch would work
> perfectly around the if/else.
In each finally we call methods of different UI modules, we either have the finally twice or the same if/else twice.

Comment 106

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Cosmin Malutan, [:cosmin-malutan] from comment #105)
> that's why we decided to inherit form BaseWindow, but the feature says about
> itself that it's a dialog:
> http://hg.mozilla.org/mozilla-central/file/29b05d283b00/browser/base/content/
> aboutDialog.xul

No, that's just the file name. Please see the root element which is `window`. So it's not a dialog.

>  Since the About:Dialog is the default way for updating,
>  for errors it will rise new prompt dialogs, but in case the Update:Wizard

What kind of new prompt dialogs are that?

> is open
>  we expect the error to be displayed in-window, so when the Update:Wizard is
> open
>  we need to let the browser know, by setting this preference:
>  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> nsUpdateService.js?rev=a9240b1eb2fb#4813
>  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> nsUpdateService.js?rev=a9240b1eb2fb#4756

What do you mean with "displayed in-window"? Maybe you can give an example or screenshot so I can better understand this.

> > Aren't those all common dialog elements?
> We have an wrapper and also we have different buttons without ID, like
> "Next", "Back", "Finish"

The dialog buttons also don't have an id, but anon-id. So you say that these buttons here have different anon-ids? Keep in mind that common dialog buttons can be used for whatever you want, and appropriately re-labeled.

> > Ńame this method waitForWizardPageChanged. Also listing this.currentPage
> > here is useless as mentioned a lot in the past. It will not contain the
> > value you expect.
> this.currentPage lists the actual value and the aStep lists the expected
> value.

No, in all the cases this.currentPage will be equal to aStep because it is passed into the method when it is called!

> > @@ +120,5 @@
> > > +      wizard.close();
> > > +    }
> > > +    finally {
> > > +      // Store details about the patch
> > > +      persisted.updates[persisted.update.index].patch_fallback = wizard.patchInfo;
> > 
> > I don't want to see the same finally code twice. The try/catch would work
> > perfectly around the if/else.
> In each finally we call methods of different UI modules, we either have the
> finally twice or the same if/else twice.

Hm ok, I think that is minor then. With the method I mentioned we would need another variable to hold the patch info. Leave it as it is. Thanks.

Comment 107

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #106)
> (In reply to Cosmin Malutan, [:cosmin-malutan] from comment #105)
> > that's why we decided to inherit form BaseWindow, but the feature says about
> > itself that it's a dialog:
> > http://hg.mozilla.org/mozilla-central/file/29b05d283b00/browser/base/content/
> > aboutDialog.xul
> 
> No, that's just the file name. Please see the root element which is
> `window`. So it's not a dialog.
I see, I will revert this, sorry for misunderstanding.

> >  Since the About:Dialog is the default way for updating,
> >  for errors it will rise new prompt dialogs, but in case the Update:Wizard
> 
> What kind of new prompt dialogs are that?
> 
> > is open
> >  we expect the error to be displayed in-window, so when the Update:Wizard is
> > open
> >  we need to let the browser know, by setting this preference:
> >  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> > nsUpdateService.js?rev=a9240b1eb2fb#4813
> >  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> > nsUpdateService.js?rev=a9240b1eb2fb#4756
> 
> What do you mean with "displayed in-window"? Maybe you can give an example
> or screenshot so I can better understand this.
See the screenshot already attached.

> > > Aren't those all common dialog elements?
> > We have an wrapper and also we have different buttons without ID, like
> > "Next", "Back", "Finish"
> 
> The dialog buttons also don't have an id, but anon-id. So you say that these
> buttons here have different anon-ids? Keep in mind that common dialog
> buttons can be used for whatever you want, and appropriately re-labeled.
Yes, they are different, we don't have them on the base class.
 
> > > Ńame this method waitForWizardPageChanged. Also listing this.currentPage
> > > here is useless as mentioned a lot in the past. It will not contain the
> > > value you expect.
> > this.currentPage lists the actual value and the aStep lists the expected
> > value.
> 
> No, in all the cases this.currentPage will be equal to aStep because it is
> passed into the method when it is called!
I see, thanks.

Comment 108

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v14.0

Whimboo, I made the changes.

Regular update:
http://mozmill-crowd.blargon7.com/#/update/report/4d9c8aea57549df57dc5846cc000ec7a
http://mozmill-crowd.blargon7.com/#/update/report/4d9c8aea57549df57dc5846cc000f6a1
Incompatible-addons:
http://mozmill-crowd.blargon7.com/#/update/report/4d9c8aea57549df57dc5846cc000d867
http://mozmill-crowd.blargon7.com/#/update/report/4d9c8aea57549df57dc5846cc000df4f
Up to date build:
http://mozmill-crowd.blargon7.com/#/update/report/4d9c8aea57549df57dc5846cc00111a5
http://mozmill-crowd.blargon7.com/#/update/report/4d9c8aea57549df57dc5846cc0011ea6

Comment 109

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Cosmin Malutan, [:cosmin-malutan] from comment #107)
> > >  Since the About:Dialog is the default way for updating,
> > >  for errors it will rise new prompt dialogs, but in case the Update:Wizard
> > 
> > What kind of new prompt dialogs are that?
> > 
> > > is open
> > >  we expect the error to be displayed in-window, so when the Update:Wizard is
> > > open
> > >  we need to let the browser know, by setting this preference:
> > >  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> > > nsUpdateService.js?rev=a9240b1eb2fb#4813
> > >  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> > > nsUpdateService.js?rev=a9240b1eb2fb#4756
> > 
> > What do you mean with "displayed in-window"? Maybe you can give an example
> > or screenshot so I can better understand this.
> See the screenshot already attached.

When I do a review and it is not clear what code is doing, the in-code documentation is not sufficient enough. Any kind of additional info as attached on this bug, I will not take into consideration to replace the comments in code. So please make sure it is well described.

If not done so please update the new patch appropriately.

Comment 110

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v14.1

I updated the comment.

Comment 111

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8557794 [details] [diff] [review]
patch v14.1

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

::: firefox/lib/ui/about-window.js
@@ +105,5 @@
> + *        Sets if the function should wait until the download has been finished
> + * @param {Number} [aTimeout=600s]
> + *        Timeout until the download should finish
> + */
> +AboutWindow.prototype.download = function AW_download(aWaitForFinish=true, aTimeout=360000) {

I haven't said you should remove this or other constants. We should still keep those at the top of the file.

@@ +180,5 @@
> + *
> + * @param {Number} [aTimeout=360000]
> + *        The amount of time to wait for check to complete
> + */
> +AboutWindow.prototype.waitForCheckFinished = function AW_waitForCheckFinished(aTimeout=30000) {

This needs a global constant. Also the jsdoc has a different value.

@@ +191,5 @@
> + *
> + * @param {Number} [aTimeout=360000]
> + *        The amount of time to wait for download to complete
> + */
> +AboutWindow.prototype.waitForDownloadFinished = function AW_waitForDownloadFinished(aTimeout=360000) {

This needs a global constant.

@@ +194,5 @@
> + */
> +AboutWindow.prototype.waitForDownloadFinished = function AW_waitForDownloadFinished(aTimeout=360000) {
> +  assert.waitFor(() => {
> +    return !!this.softwareUpdate.activeUpdate;
> +  }, "An active update has been found");

Why is this not part of download()? We cannot download a patch if there is no active update.

@@ +196,5 @@
> +  assert.waitFor(() => {
> +    return !!this.softwareUpdate.activeUpdate;
> +  }, "An active update has been found");
> +
> +  assert.waitFor(() => this.softwareUpdate.activeUpdate.state !== "downloading",

Given that we have UI tests, please check the UI here and not back-end code.

@@ +200,5 @@
> +  assert.waitFor(() => this.softwareUpdate.activeUpdate.state !== "downloading",
> +                 "Download has been completed.", aTimeout);
> +
> +  assert.waitFor(() => this.softwareUpdate.activeUpdate.state === "applied",
> +                 "Update has been applied.", aTimeout);

Same here.

@@ +203,5 @@
> +  assert.waitFor(() => this.softwareUpdate.activeUpdate.state === "applied",
> +                 "Update has been applied.", aTimeout);
> +
> +  assert.notEqual(this.wizardState, WIZARD_PAGES.downloadFailed,
> +                  "Update has downloaded successfully");

'has been downloaded'

@@ +210,5 @@
> +/**
> + * Open the About window
> + *
> + * @param {function} aCallback
> + *        Callback that opens the Browser:About Window

nit 'window'

::: firefox/lib/ui/update-wizard.js
@@ +33,5 @@
> +};
> +
> +// In the old Software Update UI, the errors are displayed in-dialog, for the new
> +// Software Update UI the errors are displayed in new prompts(dialogs). When
> +// the old UI is open we have to set preference, so the errors will be showed as

nits: 'have to set the preference' and 'will be shown'

@@ +34,5 @@
> +
> +// In the old Software Update UI, the errors are displayed in-dialog, for the new
> +// Software Update UI the errors are displayed in new prompts(dialogs). When
> +// the old UI is open we have to set preference, so the errors will be showed as
> +// expected, otherwise we would have unhandled dialogs when errors are raised.

'unhandled modal dialogs'

@@ +52,5 @@
> +
> +  this.softwareUpdate = new softwareUpdate.SoftwareUpdate();
> +  this._downloadDuration = -1;
> +
> +  prefs.setPref(PREF_APP_UPDATE_ALTWINDOWTYPE, "Update:Wizard");

This pref only has to be set inside the download method, right? We should use a try/finally to ensure it gets really reset.

@@ +109,5 @@
> + *        Sets if the function should wait until the download has been finished
> + * @param {Number} [aTimeout=360000]
> + *        Time until the download should finish
> + */
> +UpdateWizardDialog.prototype.download = function UWD_download(aWaitForFinish=true, aTimeout=360000) {

As a constant at the top please.

@@ +119,5 @@
> +  // If updates are found, proceed to download
> +  if (this.currentPage === WIZARD_PAGES.updatesfoundbasic ||
> +      this.currentPage === WIZARD_PAGES.updatesFoundBillboard ||
> +      this.currentPage === WIZARD_PAGES.errorPatching) {
> +    this.goToNextWizardPage();

What about `selectNextPage`?

@@ +217,5 @@
> + *        The wizard page id to change to
> + * @param {Number} [aTimeout=30000]
> + *        Timeout for the page to change
> + */
> +UpdateWizardDialog.prototype.waitForWizardPage = function UWD_waitForWizardPage(aStep, aTimeout=30000) {

Why a default of 30000? This is chrome and should get the default timeout of 5s. Same as it was before.

@@ +219,5 @@
> + *        Timeout for the page to change
> + */
> +UpdateWizardDialog.prototype.waitForWizardPage = function UWD_waitForWizardPage(aStep, aTimeout=30000) {
> +  assert.waitFor(() => this.currentPage === aStep,
> +                 "New wizard page has been selected:" + aStep, aTimeout);

missing blank after the colon.

@@ +230,5 @@
> + *        The wizard page id to change from
> + * @param {Number} [aTimeout=30000]
> + *        Timeout for the page to change
> + */
> +UpdateWizardDialog.prototype.waitForWizardPageChanged = function UWD_waitForWizardPageChanged(aStep, aTimeout=30000) {

Same as above for the timeout value.

@@ +232,5 @@
> + *        Timeout for the page to change
> + */
> +UpdateWizardDialog.prototype.waitForWizardPageChanged = function UWD_waitForWizardPageChanged(aStep, aTimeout=30000) {
> +  assert.waitFor(() => this.currentPage !== aStep,
> +                 "Wizard page has changed: " + aStep, aTimeout);

aStep is no longer the selected wizard page.

@@ +241,5 @@
> + *
> + * @param {Number} [aTimeout=360000]
> + *        Timeout the download has to stop
> + */
> +UpdateWizardDialog.prototype.waitforDownloadFinished = function UWD_waitForDownloadFinished(aTimeout=360000) {

A global constant.

::: firefox/tests/update/testFallbackUpdate/testUpdate.js
@@ +99,2 @@
>  
> +    aboutWindow.checkForUpdates();

nit: remove the blank line

Comment 112

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v15.0

(In reply to Henrik Skupin (:whimboo) from comment #111)
> > +  assert.waitFor(() => this.softwareUpdate.activeUpdate.state !== "downloading",
> 
> Given that we have UI tests, please check the UI here and not back-end code.
> 
> @@ +200,5 @@
> > +  assert.waitFor(() => this.softwareUpdate.activeUpdate.state !== "downloading",
> > +                 "Download has been completed.", aTimeout);
> > +
> > +  assert.waitFor(() => this.softwareUpdate.activeUpdate.state === "applied",
> > +                 "Update has been applied.", aTimeout);
> 
> Same here.
I have to wait for 'activeUpdate.state === "applied"' otherwise we will fail in forcing the fallback update.

http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516034e19
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851603436e

http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851602f986
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851602d08b

Comment 113

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8558990 [details] [diff] [review]
patch v15.0

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

> (In reply to Henrik Skupin (:whimboo) from comment #111)
> > > +  assert.waitFor(() => this.softwareUpdate.activeUpdate.state !== "downloading",
> > 
> > Given that we have UI tests, please check the UI here and not back-end code.
> > 
> I have to wait for 'activeUpdate.state === "applied"' otherwise we will fail
> in forcing the fallback update.

Why does it not work with any of the wizard pages? Those should reflect exactly those states. And as said we are testing the UI.

Also please do not request review for some updates if there are still open questions to discuss.

Comment 114

[Cosmin Malutan, [:cosmin-malutan]]

I don't know why, I'm not the one who implemented the update mechanism, perhaps it spawns two tasks when the download is completed, one for UI and one for the stage.

Yes we are testing the UI, but for this specific test we alter and back-end file to force the update, if we modify the file before the firefox does, the firefox will override our change and so we will have no dialog for downloading the fallback update. 

http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f85160336a1

Comment 115

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You can always check the applyied state at the very end. It doesn't stop us from verifying the UI first for downloading and applying.

It's actually a good point, and could be the reason of some sporadic update failures we see in this step. Thanks for catching that!

Comment 116

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v15.1

I switched the backend check with UI check.

http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851612f171
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f85161280c5

Comment 117

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8559111 [details] [diff] [review]
patch v15.1

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

I think we are pretty close. Just a couple of comments.

Beside those I would like that you run some update tests for Nightly. Here a list which comes to my mind:

* a build from yesterday to today
* the same build but as RTL locale
* a latest build with no available updates
* a build from two days ago, so we get a full mar file served
* a build before the last version bump
* a build from yesterday with an invalid update channel

If I missed a case please add it. Otherwise thanks a lot for all this massive work!

::: firefox/lib/ui/about-window.js
@@ +138,5 @@
> +  if (aWaitForFinish) {
> +    var startTime = Date.now();
> +    assert.waitFor(() => {
> +      return !!this.softwareUpdate.activeUpdate;
> +    }, "An active update has been found");

Why are you adding this check here and not at the beginning of this method right after the channel assertion? Doing any of the above clicks would cause a failure without this helpful information. Using the latest build should show it.

@@ +206,5 @@
> +  assert.waitFor(() => this.wizardState !== WIZARD_PAGES.downloading,
> +                 "Download has been completed.", aTimeout);
> +
> +  assert.waitFor(() => this.wizardState === WIZARD_PAGES.apply,
> +                 "Final wizard page has been selected.");

This is not part of the download but already the next step. It would be good to have an waitForUpdateApplied method here.

::: firefox/lib/ui/update-wizard.js
@@ +100,5 @@
> + *        Time until the download should finish
> + */
> +UpdateWizardDialog.prototype.download = function UWD_download(aWaitForFinish=true, aTimeout=TIMEOUT_UPDATE_DOWNLOAD) {
> +  prefs.setPref(PREF_APP_UPDATE_ALTWINDOWTYPE, "Update:Wizard");
> +

I don't see an activeUpdate check here.

@@ +105,5 @@
> +  try {
> +    // Check that the correct channel has been set
> +    assert.equal(this.softwareUpdate.updateChannel.defaultChannel,
> +      this.softwareUpdate.updateChannel.channel,
> +      "The update channel has been set correctly.");

You introduced a broken indentation here.

@@ +110,5 @@
> +
> +    // If updates are found, proceed to download
> +    if (this.currentPage === WIZARD_PAGES.updatesfoundbasic ||
> +      this.currentPage === WIZARD_PAGES.updatesFoundBillboard ||
> +      this.currentPage === WIZARD_PAGES.errorPatching) {

Same here.

::: firefox/tests/addons/[e-mail]/addon.ini
@@ -1,4 @@
> -[download]
> -linux=http://release.seleniumhq.org/selenium-ide/editor/2.7.0/selenium-ide-editor-2.7.0.xpi
> -mac=http://release.seleniumhq.org/selenium-ide/editor/2.7.0/selenium-ide-editor-2.7.0.xpi
> -win=http://release.seleniumhq.org/selenium-ide/editor/2.7.0/selenium-ide-editor-2.7.0.xpi

You don't want to remove this file.

Comment 118

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #117)
> Comment on attachment 8559111 [details] [diff] [review]
> 
> @@ +206,5 @@
> > +  assert.waitFor(() => this.wizardState !== WIZARD_PAGES.downloading,
> > +                 "Download has been completed.", aTimeout);
> > +
> > +  assert.waitFor(() => this.wizardState === WIZARD_PAGES.apply,
> > +                 "Final wizard page has been selected.");
> 
> This is not part of the download but already the next step. It would be good
> to have an waitForUpdateApplied method here.
Oh this is part of download, please notice that we only wait for "Restart to apply..." button, this is shown when the download is complete, we don't take any action further.

Comment 119

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #117)
> Comment on attachment 8559111 [details] [diff] [review]
> patch v15.1
> 
> Review of attachment 8559111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we are pretty close. Just a couple of comments.
> 
> Beside those I would like that you run some update tests for Nightly. Here a
> list which comes to my mind:
> 
> * a build from yesterday to today
> * the same build but as RTL locale
> * a latest build with no available updates
> * a build from two days ago, so we get a full mar file served
> * a build before the last version bump
> * a build from yesterday with an invalid update channel
> 
> If I missed a case please add it. Otherwise thanks a lot for all this
> massive work!
> 
> ::: firefox/lib/ui/about-window.js
> @@ +138,5 @@
> > +  if (aWaitForFinish) {
> > +    var startTime = Date.now();
> > +    assert.waitFor(() => {
> > +      return !!this.softwareUpdate.activeUpdate;
> > +    }, "An active update has been found");
> 
> Why are you adding this check here and not at the beginning of this method
> right after the channel assertion? Doing any of the above clicks would cause
> a failure without this helpful information. Using the latest build should
> show it.
I moved the assertion above, and it failed, this has an value only if the download started, until that we have a a couple of actions, handle the download button handle the incompatible addons, that's why we have to add it just before we wait for download.

Comment 120

[Cosmin Malutan, [:cosmin-malutan]]

Reports: 
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851622e25a
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851622f1ec

http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851622b4b4
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851622c481

http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851622a57f
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516229ffb

http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f85162297cd
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851622883f

http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f85162286db
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516227706

I covered all the cases Henrik mentioned + incompatible addons.

The RTL locales are broken, I'll file a bug in a bit.

Comment 121

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Cosmin Malutan, [:cosmin-malutan] from comment #118)
> > > +  assert.waitFor(() => this.wizardState !== WIZARD_PAGES.downloading,
> > > +                 "Download has been completed.", aTimeout);
> > > +
> > > +  assert.waitFor(() => this.wizardState === WIZARD_PAGES.apply,
> > > +                 "Final wizard page has been selected.");
> > 
> > This is not part of the download but already the next step. It would be good
> > to have an waitForUpdateApplied method here.
> Oh this is part of download, please notice that we only wait for "Restart to
> apply..." button, this is shown when the download is complete, we don't take
> any action further.

The download ends once the WIZARD_PAGE.downloading is no longer visible. Anything which comes now the download should not worry about. As result it is NOT part of the download.

(In reply to Cosmin Malutan, [:cosmin-malutan] from comment #119)
> > Why are you adding this check here and not at the beginning of this method
> > right after the channel assertion? Doing any of the above clicks would cause
> > a failure without this helpful information. Using the latest build should
> > show it.
> I moved the assertion above, and it failed, this has an value only if the
> download started, until that we have a a couple of actions, handle the
> download button handle the incompatible addons, that's why we have to add it
> just before we wait for download.

I see. I think in this case and also comparing again with the current update tests, we could even remove this extra check for activeUpdate here. We wouldn't reach this point anyway if no update is found by the check. And we hit this assertion when fetching the patchInfo.

Comment 122

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v16.0

Done.

To summarize what the patch does.
* Due to relocation of the Software Update UI in Firefox, we had to update our tests for handling the updates trough the new UI.
* We had to write UI test class for the About:Dialog, to rewrite the library of the old UI so it has a testing class, to separate the testing stage code from the UI classes.
* Other changes are that we calculate the download duration in the UI classes due to split of the libraries, and we fixed the handling of updates in case of addons incompatibility in the target version. 

Reports:
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516292fda
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516294f4e
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516296ea2
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851629734e
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f85162988f8
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f85162999cd
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851629b193
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851629da44
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f851629eb65

Comment 123

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v16.1

I had to add addon.ini file again, it didn't got added to the patch by previous refresh.

Comment 124

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8559741 [details] [diff] [review]
patch v16.1

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

I left two comments in the code. Otherwise I can only see test results on Linux. Please do at least some basic testing on Windows and OS X.

::: firefox/lib/ui/about-window.js
@@ +64,5 @@
> + * @returns {Boolean} True if restart is needed
> + */
> +AboutWindow.prototype.__defineGetter__('needsRestart', function() {
> +  return this.wizardState === WIZARD_PAGES.apply;
> +});

A restart is always necessary. So I do not see any benefit in this method.  Or what are the cases when we do not end-up here?

@@ +202,5 @@
> +  assert.waitFor(() => this.wizardState !== WIZARD_PAGES.downloading,
> +                 "Download has been completed.", aTimeout);
> +
> +  assert.notEqual(this.wizardState, WIZARD_PAGES.downloadFailed,
> +                  "Update has been downloaded");

So you removed the waitfor() for `this.wizardState === WIZARD_PAGES.apply` but haven't added it somewhere else. We have to check that this page is shown via a waitForUpdateApplied() method.

Comment 125

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v17.0

(In reply to Henrik Skupin (:whimboo) from comment #124)
> > +  assert.notEqual(this.wizardState, WIZARD_PAGES.downloadFailed,
> > +                  "Update has been downloaded");
> 
> So you removed the waitfor() for `this.wizardState === WIZARD_PAGES.apply`
> but haven't added it somewhere else. We have to check that this page is
> shown via a waitForUpdateApplied() method.
I made a new waitForUpdateApplied method in which I wait for both wizzardState and then for the backend status file to contains applied.

Reports
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516a3200d
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516a2cf9e
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516a2227c
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516a20c06
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516a3b25a

Comment 126

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8560399 [details] [diff] [review]
patch v17.0

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

Very close....

::: firefox/lib/ui/about-window.js
@@ +127,5 @@
> +
> +  if (aWaitForFinish) {
> +    var startTime = Date.now();
> +    this.waitForDownloadFinished(aTimeout);
> +    this.waitForUpdateApplied();

We are still in the download method here. Applying an downloaded update is still a different step, so the waitForUpdateApplied() should be called directly from the test then.

@@ +193,5 @@
> +                  "Update has been downloaded");
> +};
> +
> +/**
> + * Waits until update is applied

"Waits until the downloaded update has been applied"

@@ +198,5 @@
> + *
> + * @param {Number} [aTimeout=360000]
> + *        The amount of time to wait for update to apply
> + */
> +AboutWindow.prototype.waitForUpdateApplied = function AW_waitForUpdateApplied(aTimeout=TIMEOUT_UPDATE_DOWNLOAD) {

Please make that a separate constant as this is not about downloading.

@@ +202,5 @@
> +AboutWindow.prototype.waitForUpdateApplied = function AW_waitForUpdateApplied(aTimeout=TIMEOUT_UPDATE_DOWNLOAD) {
> +  assert.waitFor(() => this.wizardState === WIZARD_PAGES.apply,
> +                 "Final wizard page has been selected.");
> +
> +  // Wait for update to be staged

Please add a bit more context here why we have to wait for the patch status too. It's not clear right now.

Comment 127

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v17.1

Done


Reports:
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516a42a87
http://mozmill-crowd.blargon7.com/#/update/report/999627efae7b8074fc4a2f8516acf125

Comment 128

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8560446 [details] [diff] [review]
patch v17.1

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

r- because of the wrong constant value. Otherwise just nits. So I feel the next iteration will be the final one!

::: firefox/lib/ui/about-window.js
@@ +28,5 @@
> +  unsupportedSystem: "unsupportedSystem"
> +};
> +
> +const TIMEOUT_UPDATE_CHECK    = 30000;
> +const TIMEOUT_UPDATE_APPLY = 30000;

This is not the value which was in use before. This could cause race conditions because sometimes it can take longer than 30s. Especially on slow machines. So please check the former code correctly.

@@ +195,5 @@
> +
> +/**
> + * Waits until the downloaded update has been applied
> + *
> + * @param {Number} [aTimeout=360000]

This value doesn't match anymore.

@@ +202,5 @@
> +AboutWindow.prototype.waitForUpdateApplied = function AW_waitForUpdateApplied(aTimeout=TIMEOUT_UPDATE_APPLY) {
> +  assert.waitFor(() => this.wizardState === WIZARD_PAGES.apply,
> +                 "Final wizard page has been selected.");
> +
> +  // Wait for update to be staged because for update tests we alter and back-end

"we modify the update status file"

@@ +204,5 @@
> +                 "Final wizard page has been selected.");
> +
> +  // Wait for update to be staged because for update tests we alter and back-end
> +  // file to enforce the fallback update, if we modify the file before the firefox does,
> +  // the firefox will override our change and we will have no fallback update.

"fallback update. If..." and please remove "the" before Firefox in both cases.

Comment 129

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v17.2

(In reply to Henrik Skupin (:whimboo) from comment #128)
> 
> r- because of the wrong constant value. Otherwise just nits. So I feel the
> next iteration will be the final one!
Thanks Henrik, I've made the changes. Also I feel like this patch is the biggest contribution I had to the project.

Comment 130

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8561319 [details] [diff] [review]
patch v17.2

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

Wonderful! Just a small nit left in a comment. Otherwise this looks great and seems to have been tested well enough. When we get this landed please let it ride the trains.

::: firefox/lib/ui/about-window.js
@@ +203,5 @@
> +  assert.waitFor(() => this.wizardState === WIZARD_PAGES.apply,
> +                 "Final wizard page has been selected.");
> +
> +  // Wait for update to be staged because for update tests we modify the update
> +  // status file file to enforce the fallback update. If we modify the file

nit: file file

Comment 131

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v17.3

Wow, thanks a lot for bearing with me trough this bug.

Comment 132

[Cosmin Malutan, [:cosmin-malutan]]

We have to land it only on Nightly, we will let it to ride the trains.

Comment 133

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/a83c738833d2 (default)
Thanks a lot Cosmin, this was a long ride :)