1322085 - Make devtools/server/actors/*.js eslint-clean

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Comment 1

[Dalimil Hajek [:dalimil]]

Attachment: Bug1322085.patch -rev1

There were too many eslint errors so I left out a few files for now (see .eslintignore)

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8821103 [details] [diff] [review]
Bug1322085.patch -rev1

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

::: .eslintignore
@@ +118,5 @@
> +devtools/server/actors/object.js
> +devtools/server/actors/script.js
> +devtools/server/actors/styleeditor.js
> +devtools/server/actors/stylesheets.js
> +devtools/server/actors/utils/TabSources.js

I recently took care of TabSources.js in bug 1325191

You'll likely need to rebase your patch on top that bug.

::: devtools/server/actors/call-watcher.js
@@ +49,5 @@
>     *        Determines whether or not FunctionCallActor stores a weak reference
>     *        to the underlying objects.
>     */
> +  initialize: function (conn, [window, global, caller, type, name, stack,
> +    timestamp, args, result], holdWeak) {

In this case, we tend to prefer:

initialize: function (
  conn,
  [window, global, caller, type, name, stack, timestamp, args, result],
  holdWeak
) {

@@ +256,5 @@
>     * created, in order to instrument the specified objects and become
>     * aware of everything the content does with them.
>     */
> +  setup: function ({ tracedGlobals, tracedFunctions, startRecording,
> +    performReload, holdWeak, storeCalls }) {

Same here:

setup: function({
 tracedGlobals, tracedFunctions, startRecording, performReload, holdWeak, storeCalls
}) {

::: devtools/server/actors/device.js
@@ +38,5 @@
>    },
>  
>    screenshotToDataURL: function () {
>      let window = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);
> +    let devicePixelRatio = window.devicePixelRatio;

This could be written as:

let { devicePixelRatio } = window;

::: devtools/server/actors/director-manager.js
@@ +38,5 @@
>   * over the remote debugging protocol.
>   */
> +var MessagePortActor = exports.MessagePortActor = protocol.ActorClassWithSpec(
> +  messagePortSpec,
> +  {

I really don't like increasing the whole block of indentation by 2.

Can we simply do:

var MessagePortActor = protocol.ActorClassWithSpec(messagePortSpec, {
 ...
});
exports.MessagePortActor = MessagePortActor;

so we don't have to bring up the level of indentation.

@@ +153,2 @@
>   */
> +var DirectorScriptActor = exports.DirectorScriptActor = protocol.ActorClassWithSpec(

Same thing here.

::: devtools/server/actors/preference.js
@@ +15,5 @@
>  
>  exports.unregister = function (handle) {
>  };
>  
> +var PreferenceActor = exports.PreferenceActor = protocol.ActorClassWithSpec(

Same comment about the indentation here.

::: devtools/server/actors/root.js
@@ +493,5 @@
> +    return DebuggerServer.connectToContent(this.conn, mm, onDestroy)
> +      .then(formResult => {
> +        this._processActors.set(id, formResult);
> +        return { formResult };
> +      });

I think increasing the indent here is misleading.

Putting the return in one line seems to fit in 90ch:
>return DebuggerServer.connectToContent(this.conn, mm, onDestroy).then(formResult => {
>  ...
>});

Can we go with that ?

::: devtools/server/actors/source.js
@@ +15,5 @@
>  const DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  const { assert, fetch } = DevToolsUtils;
>  const { joinURI } = require("devtools/shared/path");
>  const promise = require("promise");
> +const { resolve } = promise;

There seems to be only one instance of resolve() in this file.
Can we remove the define here, and just use promise.resolve() ?

@@ +399,4 @@
>  
> +      // Record the contentType we just learned during fetching
> +      return sourceFetched
> +        .then(result => {

Same comment here about the indent, can we just put .then( result => on the same line as the previous one ?

@@ +523,4 @@
>      return ({ content }) => {
>        return this.prettyPrintWorker.performTask("pretty-print", {
>          url: this.url,
> +        indent: indent,

nit: you can now simply use this:
url: this.url,
indent,
source: content

::: devtools/server/actors/utils/webconsole-listeners.js
@@ +1,2 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set ft= javascript ts=2 et sw=2 tw=80: */

Can you remove the vim/emacs config ?

::: devtools/server/actors/webaudio.js
@@ +7,2 @@
>  
> +const {Cu} = require("chrome");

nit: const { Cu } (with spaces) to be consistent. We should really lint this.

@@ +820,5 @@
>   */
>  function sanitizeAutomationArgs(args) {
>    return args.reduce((newArgs, el) => {
> +    newArgs.push(typeof el === "object" &&
> +      getConstructorName(el) === "Float32Array" ? castToArray(el) : el);

I'd prefer to store the condition in an array, and keep everything in one line.

let isArray = typeof el === "object" &&
              getConstructorName(el) === "Float32Array";
newArgs.push(isArray ? castToArray(el) : el);

::: devtools/server/actors/webgl.js
@@ +286,5 @@
>      let buffer = new this.tabActor.window.Uint8Array(4);
>      buffer = XPCNativeWrapper.unwrap(buffer);
>  
> +    proxy.readPixels(x, height - y - 1, 1, 1, context.RGBA,
> +      context.UNSIGNED_BYTE, buffer);

I prefer this form:
proxy.readPixels(
  x, height - y - 1, 1, 1, context.RGBA, context.UNSIGNED_BYTE, buffer
);

@@ +1149,5 @@
>     * @return any
>     *         The corresponding parameter's value.
>     */
> +  _getFramebufferAttachmentParameter: function (type,
> +    name = "FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE") {

_getFramebufferAttachmentParameter(type, name = "FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE") {
should fit in 90 ch.

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8821103 [details] [diff] [review]
Bug1322085.patch -rev1

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

Stealing review from jryans (with his permission). 

The patch should be good to go with the comments addressed, and rebased.

Comment 4

[Dalimil Hajek [:dalimil]]

Attachment: Bug1322085.patch - rev2

Fixed.

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=debd594868e034f6a0560fc06a19c64d198acf9c

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8822088 [details] [diff] [review]
Bug1322085.patch - rev2

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

Looks great, thanks!

::: devtools/server/actors/director-manager.js
@@ +573,5 @@
>      Object.defineProperties(attachOptions, {
>        port: { enumerable: true, value: port1 },
>        window: { enumerable: true, value: window },
> +      scriptOptions: { enumerable: true, value: Cu.cloneInto(this._scriptOptions,
> +        this._sandbox) },

I'd prefer:
scriptOptions: {
  enumerable: true,
  value: Cu.cloneInto(this._scriptOptions, this._sandbox)
},

::: devtools/server/actors/reflow.js
@@ +197,5 @@
>   */
>  var gIgnoreLayoutChanges = false;
>  exports.setIgnoreLayoutChanges = function (ignore, syncReflowNode) {
>    if (syncReflowNode) {
> +    // let forceSyncReflow = syncReflowNode.offsetWidth;

I'm guessing this wasn't intentional, can you undo this?

::: devtools/server/actors/utils/webconsole-worker-listeners.js
@@ +8,5 @@
>  
>  "use strict";
>  
> +/* global setConsoleEventHandler, retrieveConsoleEvents */
> +

This comment is already above in the file, can you remove it ?

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8822088 [details] [diff] [review]
Bug1322085.patch - rev2

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

::: devtools/server/actors/breakpoint.js
@@ +118,5 @@
>            result: true,
>            message: message
>          };
>        } else if (completion.yield) {
> +        // Shouldn't ever get yield completions from an eval

Why was the assertion removed ?

Comment 7

[Tim Nguyen :ntim]

(In reply to Dalimil Hajek [:dalimil] from comment #4)
> Push to try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=debd594868e034f6a0560fc06a19c64d198acf9c

For the test timeouts, I would recommend running the tests locally as it provides better logs. But you can always check the logs/failure screenshots on treeherder.

Comment 8

[Dalimil Hajek [:dalimil]]

Regarding // let forceSyncReflow = syncReflowNode.offsetWidth;
This was intentional because the variable is never used and thus causes an eslint error.

Comment 9

[Dalimil Hajek [:dalimil]]

Attachment: Bug1322085.patch - rev3

Fixed.
Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46b50b46ade9

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8822143 [details] [diff] [review]
Bug1322085.patch - rev3

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

r+ with this and the try failures fixed.

::: devtools/server/actors/reflow.js
@@ +197,5 @@
>   */
>  var gIgnoreLayoutChanges = false;
>  exports.setIgnoreLayoutChanges = function (ignore, syncReflowNode) {
>    if (syncReflowNode) {
> +    // let forceSyncReflow = syncReflowNode.offsetWidth;

So this line is to force a reflow, so commenting it out means it no longer does what it should (which might be why lots of tests are failing).

I'd prefer creating an exception for this line:

let forceSyncReflow = syncReflowNode.offsetWidth; // eslint-disable-line

Comment 11

[Dalimil Hajek [:dalimil]]

Attachment: Bug1322085.patch - rev4

I've changed the line. Although I still don't understand what effect the local variable has...

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08f703df94fc

Comment 12

[Tim Nguyen :ntim]

(In reply to Dalimil Hajek [:dalimil] from comment #11)
> Created attachment 8822220 [details] [diff] [review]
> Bug1322085.patch - rev4
> 
> I've changed the line. Although I still don't understand what effect the
> local variable has...

When CSS is processed in the browser, performance-wise, there are paints and reflows that happen. Reflow happens when a browser must process and draw part or all of a webpage again, such as after an update on an interactive site. The main difference between reflows and paints is that reflows affects other elements as well.

For example, changing the width of an element would trigger a reflow, because content next to it might need to be painted again at a different position.

As for paints, an example would be changing the background of an element, only the element itself would need to be redrawn, and content next to it can stay the same.

> let forceSyncReflow = syncReflowNode.offsetWidth;
is one way to force a reflow on an element.

If you'd like to learn more about this stuff, I'd recommend playing with the performance tool.

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8822220 [details] [diff] [review]
Bug1322085.patch - rev4

r+ assuming try will be green.

Comment 14

[Dalimil Hajek [:dalimil]]

There are some test failures that don't have associated bugs, but I cannot really tell if any of the test failures are related to the pieces of code that I changed...

Comment 15

[Tim Nguyen :ntim]

(In reply to Dalimil Hajek [:dalimil] from comment #14)
> There are some test failures that don't have associated bugs, but I cannot
> really tell if any of the test failures are related to the pieces of code
> that I changed...

I think the failures are. Try to run the failing tests locally, and check if they still fail without the patch.



My guess is that one of `var FooActor =` removals might be causing the failure:

- var FooActor = exports.FooActor = ...
+ exports.FooActor = ...

I can't see anything else that might be causing it.

I would recommend restoring all those removals, then running the failing test locally and check if no longer fails. 

If the removals are actually the source of the issue, try doing the removals one by one and run the failing test in between.

For the removal(s) that are causing the issue, you can add /* exported foo */ on top of the files.

Comment 16

[Tim Nguyen :ntim]

Ryan, do you know what might be going on ?

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8822220 [details] [diff] [review]
Bug1322085.patch - rev4

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

::: devtools/server/actors/root.js
@@ +491,5 @@
> +      this._processActors.delete(id);
> +    };
> +    return DebuggerServer.connectToContent(this.conn, mm, onDestroy).then(formResult => {
> +      this._processActors.set(id, formResult);
> +      return { formResult };

Looks this is the cause of the test errors.  Using `{ formResult }` changes behavior, the object returned will now be `{ formResult: <value> }` instead of `{ form: <value> }` like it was before.

You could go with `return { form: formResult }`.  (Converting the function to Task.async so you can use `form = yield DebuggerServer.connectToContent(...)` should get around the issue as well, but it's a more involved change.)

Comment 18

[Dalimil Hajek [:dalimil]]

Attachment: Bug1322085.patch - rev5

Fixed.

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc7b81db2d8

Comment 19

[Dalimil Hajek [:dalimil]]

Attachment: Bug1322085.patch - rev5

Comment 20

[Dalimil Hajek [:dalimil]]

Attachment: Bug1322085.patch - rev6

Sorry there have been more changes since last time and I forgot to rebase before.

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c64f00aaa0a

Comment 21

[Tim Nguyen :ntim]

Comment on attachment 8827269 [details] [diff] [review]
Bug1322085.patch - rev6

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

Good catch jryans! Subtle enough I missed it

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b43e1793702
Make devtools/server/actors/*.js eslint-clean. r=ntim

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2b43e1793702