884768 - Clean-up contents of frame.js

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

The content in frame.js needs a bit of clean-up and some parts have to be refactored for an easier usage. This will help me with a larger refactor on bug 865690.

Comment 1

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

Attachment: Patch v1

Comment 2

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

Attachment: Patch v1.1

Fixed a silly failure for the select statement in the AppQuitObserver.observe method. Now it's working as expected.

Comment 3

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

Comment on attachment 764723 [details] [diff] [review]
Patch v1.1

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

Other than a minor nit, these changes look okay to me. I'm not particularly familiar with this code, though, so you may want a second opinion.

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +2,4 @@
>   * 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 EXPORTED_SYMBOLS = ['Collector','Runner','events', 'runTestFile', 'log',

Nit: Add spaces after each comma.

Comment 4

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

Attachment: Patch v2

Fixed the white-space issues in EXPORTED_SYMBOLS.

Comment 5

[cmtalbert]

Comment on attachment 764809 [details] [diff] [review]
Patch v2

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

This looks good. Mostly just moving code around so it makes a bit more logical sense. r+ assuming there are no truly functional changes in this. I don't really see any other than we do things at different times, which all looked OK.  Please run tests, this is only the heart of everything :-)

Comment 6

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

(In reply to Clint Talbert ( :ctalbert ) from comment #5)
> This looks good. Mostly just moving code around so it makes a bit more
> logical sense. r+ assuming there are no truly functional changes in this. I
> don't really see any other than we do things at different times, which all
> looked OK.  Please run tests, this is only the heart of everything :-)

My patch for the i/o port assertion is based on this patch, and it solves nearly all of our problems. Nothing regressed in the mutt nor firefox tests.

I will get this landed soon. Thanks for the review.

Comment 7

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

Landed on master:
https://github.com/mozilla/mozmill/commit/50c01fc575c09440dfce8f5bed069865a5127b9f