1518656 - Remove unused parts of Mozmill/JSBridge extensions and clean up

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, enhancement)

Description

[Geoff Lankow (:darktrojan)]

There's a huge amount of code in the Mozmill extension we just don't use any more. There's also a load of ancient junk that can be modernised.

I have a series of patches to make stuff better.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1518656-mozmill-gut-1.diff

Part 1: Remove all the unused stuff. This is almost entirely deletion, but there are four files changed:

.eslintignore
mail/test/resources/mozmill/mozmill/extension/bootstrap.js
mail/test/resources/mozmill/mozmill/extension/chrome.manifest
mail/test/resources/mozmill/mozmill/extension/content/overlay.js

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1518656-mozmill-eslint-mozmill-1.diff

Part 2: rename .js files as .jsm (which they should be, and it gives us better linting) and lint them. I also replaced some functions with modern code.

I know the indentation of securable-module.jsm is a mess, I'll fix it in a separate patch to preserve history.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1518656-mozmill-eslint-jsbridge-1.diff

Part 3: jsbridge extension.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1518656-mozmill-package-names-1.diff

Part 4: rename the packages. This is just to avoid confusing them with the upstream packages.

Comment 5

[Geoff Lankow (:darktrojan)]

Comment on attachment 9035172 [details] [diff] [review]
1518656-mozmill-gut-1.diff

See comment 1 before you read this entire thing.

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9035203 [details] [diff] [review]
1518656-mozmill-package-names-1.diff

I don't know if you're the right person to approve this, but you're doing the rest so might as well.

Comment 7

[:aceman]

Comment on attachment 9035172 [details] [diff] [review]
1518656-mozmill-gut-1.diff

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

Thanks

Comment 8

[:aceman]

Comment on attachment 9035185 [details] [diff] [review]
1518656-mozmill-eslint-mozmill-1.diff

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

::: mail/test/resources/mozmill/mozmill/__init__.py
@@ -198,5 @@
>              "filename": python_callbacks_module.__file__
>          })
>          return True
>  
> -    def firePythonCallback_listener(self, obj):

How did you find out this is unused?

::: mail/test/resources/mozmill/mozmill/extension/content/modules/elementslib.jsm
@@ -191,4 @@
>    return "Link: " + this.linkName;
> -}
> -Link.prototype.getNodeForDocument = function (linkName) {
> -  var getText = function(el){

This is only unused because it is commented out in the line below. So possibly remove that one too.

::: mail/test/resources/mozmill/mozmill/extension/content/modules/frame.jsm
@@ +315,2 @@
>  
> +let jsbridge;

'var' for globals.

@@ +509,3 @@
>    this.collector.initTestDirectory(directory);
>  
> +  for (var test of Object.values(this.collector.test_modules_by_filename)) {

Why is this better?

@@ -688,5 @@
>        if (module.__root_path__ != undefined) {
>          this.collector.initTestDirectory(module.__root_path__);
>        }
>      }
> -    var deps = this.getDependencies(module);

Why can this be dropped?

::: mail/test/resources/mozmill/mozmill/extension/content/stdlib/EventUtils.jsm
@@ -6,5 @@
> -                        "disableNonTestMouseEvents", "synthesizeComposition",
> -                        "synthesizeQuerySelectedText", "synthesizeQueryTextContent",
> -                        "synthesizeQueryCaretRect", "synthesizeQueryTextRect",
> -                        "synthesizeQueryEditorRect", "synthesizeCharAtPoint",
> -                        "synthesizeSelectionSet"];

I'm not sure we want to drop all these even if currently unused.

@@ -216,5 @@
>   */
> -function synthesizeMouse(aTarget, aOffsetX, aOffsetY, aEvent, aWindow)
> -{
> -  if (!aWindow)
> -    aWindow = window;

Why removing?

@@ -879,5 @@
>        new KeyboardEvent("", {});
>        return KeyboardEvent;
>      } catch (ex) {}
>    }
> -  if (typeof content != "undefined" && ("KeyboardEvent" in content)) {

Isn't 'content' some special keyword/object? How do you know it is unused here?

::: mail/test/resources/mozmill/mozmill/extension/content/stdlib/arrays.jsm
@@ +34,5 @@
>  // the terms of any one of the MPL, the GPL or the LGPL.
>  //
>  // ***** END LICENSE BLOCK *****
>  
> +var EXPORTED_SYMBOLS = ["getSet", "compare"];

You are really taking it thoroughly:)

::: mail/test/resources/mozmill/mozmill/extension/content/stdlib/dom.jsm
@@ +39,3 @@
>  
>  
> +var getAttributes = function(node) {

Why not 'function getAttributes()' as you have done elsewhere?

::: mail/test/resources/mozmill/mozmill/extension/content/stdlib/httpd.jsm
@@ -6,5 @@
>  
>  /*
> - * An implementation of an HTTP server both as a loadable script and as an XPCOM
> - * component.  See the accompanying README file for user documentation on
> - * httpd.js.

Why?

@@ +2834,5 @@
>     * This object contains the default handlers for the various HTTP error codes.
>     */
>    _defaultErrors:
>    {
> +    400(metadata, response) {

This looks weird, but probably correct.

@@ +2939,5 @@
>     * Contains handlers for the default set of URIs contained in this server.
>     */
>    _defaultPaths:
>    {
> +    "/": function(metadata, response) {

This even more so ;)

@@ +4384,3 @@
>          this._headers[name][0] += "," + value;
>          NS_ASSERT(this._headers[name].length === 1,
> +            "how'd a non-special header have multiple values?");

Strange indent.

Comment 9

[:aceman]

Comment on attachment 9035201 [details] [diff] [review]
1518656-mozmill-eslint-jsbridge-1.diff

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

::: mail/test/resources/jsbridge/jsbridge/extension/bootstrap.js
@@ +39,5 @@
>    // We only needed to start the server once, so unregister the listener.
>    ExtensionSupport.unregisterWindowListener(addonID);
>  }
>  
>  function loadScript(url, targetWindow) {

The function could be inlined now to the single caller.

::: mail/test/resources/jsbridge/jsbridge/extension/chrome/content/modules/events.js
@@ +1,5 @@
>  var EXPORTED_SYMBOLS = ["backchannels", "fireEvent", "addBackChannel"];
>  
>  var backchannels = [];
>  
> +var fireEvent = function(name, obj) {

funcion fireEvent() ?

@@ +13,2 @@
>  
> +var addBackChannel = function(backchannel) {

function ...

Comment 10

[:aceman]

Comment on attachment 9035203 [details] [diff] [review]
1518656-mozmill-package-names-1.diff

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

Yes, thanks.

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9035185 [details] [diff] [review]
1518656-mozmill-eslint-mozmill-1.diff

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

::: mail/test/resources/mozmill/mozmill/extension/content/modules/frame.jsm
@@ -688,5 @@
>        if (module.__root_path__ != undefined) {
>          this.collector.initTestDirectory(module.__root_path__);
>        }
>      }
> -    var deps = this.getDependencies(module);

getDependencies always returns an empty array. Whether it's supposed to or not is a different matter, but it's a broken feature we don't use, so let's get rid of it.

::: mail/test/resources/mozmill/mozmill/extension/content/stdlib/EventUtils.jsm
@@ -6,5 @@
> -                        "disableNonTestMouseEvents", "synthesizeComposition",
> -                        "synthesizeQuerySelectedText", "synthesizeQueryTextContent",
> -                        "synthesizeQueryCaretRect", "synthesizeQueryTextRect",
> -                        "synthesizeQueryEditorRect", "synthesizeCharAtPoint",
> -                        "synthesizeSelectionSet"];

My reasoning is that now mochitest is available, any new testing we want that needs one of these functions should be written as a mochitest.

This stuff is all wrapped by functions in controller.jsm, except a few stray places where we've called it directly.

Also upon further inspection, we don't use synthesizeMouseScroll either.

@@ -216,5 @@
>   */
> -function synthesizeMouse(aTarget, aOffsetX, aOffsetY, aEvent, aWindow)
> -{
> -  if (!aWindow)
> -    aWindow = window;

window is undefined here. This code is pointless.

@@ -879,5 @@
>        new KeyboardEvent("", {});
>        return KeyboardEvent;
>      } catch (ex) {}
>    }
> -  if (typeof content != "undefined" && ("KeyboardEvent" in content)) {

Not in this JSM context. If we loaded this file via the subscript loader or a <script> tag, it might be, but we don't, and shouldn't.

::: mail/test/resources/mozmill/mozmill/extension/content/stdlib/arrays.jsm
@@ +34,5 @@
>  // the terms of any one of the MPL, the GPL or the LGPL.
>  //
>  // ***** END LICENSE BLOCK *****
>  
> +var EXPORTED_SYMBOLS = ["getSet", "compare"];

It's not 2006 any more!

::: mail/test/resources/mozmill/mozmill/extension/content/stdlib/httpd.jsm
@@ -6,5 @@
>  
>  /*
> - * An implementation of an HTTP server both as a loadable script and as an XPCOM
> - * component.  See the accompanying README file for user documentation on
> - * httpd.js.

Do you mean why did I change this comment, or why would anyone want to do what it says?

Comment 12

[:aceman]

(In reply to Geoff Lankow (:darktrojan) from comment #11)

/*

• • An implementation of an HTTP server both as a loadable script and as an XPCOM
• • component. See the accompanying README file for user documentation on
• • httpd.js.

Do you mean why did I change this comment, or why would anyone want to do
what it says?

Yes, why do we not need the XPCOM feature anymore?

Comment 13

[Geoff Lankow (:darktrojan)]

We never have, it's come from the version of the same file in netwerk, which also doesn't have the XPCOM bit any more but still has the comment.

Comment 14

[:aceman]

Geoff, can you please finish this? You just need to answer some of the remaining questions and update the patches according to the review comments.

Comment 15

[Geoff Lankow (:darktrojan)]

I spent a long time yesterday getting rid of the bitrot. It's almost ready again.

Yes, why do we not need the XPCOM feature anymore?

We've never used the XPCOM feature. This stuff was stolen from another test suite.

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 1518656-mozmill-eslint-mozmill-2.diff

Comment 17

[:aceman]

Comment on attachment 9043149 [details] [diff] [review]
1518656-mozmill-eslint-mozmill-2.diff

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

Thanks. It seems you still haven't answered the first 2 questions.
And don't forget to update the other patches (for review nits and bitrot).

Comment 18

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #8)

• def firePythonCallback_listener(self, obj):

How did you find out this is unused?

https://searchfox.org/comm-central/search?q=firepythoncallback

The only possible use could be in mozmill.js at line 194 or 197, but they're in functions also named firePythonCallback, which are never called as you can see.

• var getText = function(el){

This is only unused because it is commented out in the line below. So
possibly remove that one too.

I swear I did that, but since then I've had to redo large chunks of the patch to unbitrot it. So I'll do it again now.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/59888b1b19fb
Remove unused parts of Mozmill extension and clean up; r=aceman
https://hg.mozilla.org/comm-central/rev/e8578ad1d60b
Lint remaining parts of Mozmill extension; r=aceman
https://hg.mozilla.org/comm-central/rev/02142f15a54b
Lint JSBridge extension; r=aceman
https://hg.mozilla.org/comm-central/rev/8dbf6b5932f3
Rename Mozmill/JSBridge packages to avoid confusion with upstream packages; r=aceman