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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools
P3
enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ntim, Assigned: dalimil)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
Firefox 53
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
Severity: normal → enhancement
Priority: -- → P3
(Reporter)

Updated

a year ago
Whiteboard: [good first bug][lang=js]
(Reporter)

Updated

a year ago
Keywords: good-first-bug
Whiteboard: [good first bug][lang=js] → [lang=js]
(Assignee)

Comment 1

a year ago
Created attachment 8821103 [details] [diff] [review]
Bug1322085.patch -rev1

There were too many eslint errors so I left out a few files for now (see .eslintignore)
Assignee: nobody → dalimilhajek
Status: NEW → ASSIGNED
Attachment #8821103 - Flags: review?(jryans)
(Reporter)

Comment 2

a year ago
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.
(Reporter)

Comment 3

a year ago
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.
Attachment #8821103 - Flags: review?(jryans)
(Reporter)

Comment 5

a year ago
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 ?
Attachment #8822088 - Flags: review?(ntim.bugs) → review+
(Reporter)

Comment 6

a year ago
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 ?
(Reporter)

Comment 7

a year ago
(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.
(Assignee)

Comment 8

a year ago
Regarding // let forceSyncReflow = syncReflowNode.offsetWidth;
This was intentional because the variable is never used and thus causes an eslint error.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 9

a year ago
Created attachment 8822143 [details] [diff] [review]
Bug1322085.patch - rev3

Fixed.
Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46b50b46ade9
Attachment #8822088 - Attachment is obsolete: true
Attachment #8822143 - Flags: review?(ntim.bugs)
(Reporter)

Comment 10

a year ago
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
Attachment #8822143 - Flags: review?(ntim.bugs) → review+
(Reporter)

Updated

a year ago
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 11

a year ago
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...

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08f703df94fc
Attachment #8822143 - Attachment is obsolete: true
Attachment #8822220 - Flags: review?(ntim.bugs)
(Reporter)

Comment 12

a year ago
(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.
(Reporter)

Comment 13

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

r+ assuming try will be green.
Attachment #8822220 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 14

a year ago
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...
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 15

a year ago
(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.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 16

a year ago
Ryan, do you know what might be going on ?
Flags: needinfo?(jryans)
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.)
Flags: needinfo?(jryans)
(Assignee)

Comment 18

a year ago
Created attachment 8827227 [details] [diff] [review]
Bug1322085.patch - rev5

Fixed.

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc7b81db2d8
Attachment #8822220 - Attachment is obsolete: true
Attachment #8827227 - Flags: review?(ntim.bugs)
(Assignee)

Comment 19

a year ago
Created attachment 8827228 [details] [diff] [review]
Bug1322085.patch - rev5
Attachment #8827227 - Attachment is obsolete: true
Attachment #8827227 - Flags: review?(ntim.bugs)
Attachment #8827228 - Flags: review?(ntim.bugs)
(Assignee)

Comment 20

a year ago
Created attachment 8827269 [details] [diff] [review]
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
Attachment #8827228 - Attachment is obsolete: true
Attachment #8827228 - Flags: review?(ntim.bugs)
Attachment #8827269 - Flags: review?(ntim.bugs)
(Reporter)

Comment 21

a year ago
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
Attachment #8827269 - Flags: review?(ntim.bugs) → review+
(Reporter)

Updated

a year ago
Keywords: checkin-needed

Comment 22

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b43e1793702
Make devtools/server/actors/*.js eslint-clean. r=ntim
Keywords: checkin-needed

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b43e1793702
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.