Closed Bug 1322085 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: ntim, Assigned: dalimil)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file, 6 obsolete files)

No description provided.
Severity: normal → enhancement
Priority: -- → P3
Whiteboard: [good first bug][lang=js]
Keywords: good-first-bug
Whiteboard: [good first bug][lang=js] → [lang=js]
Attached patch Bug1322085.patch -rev1 (obsolete) — Splinter Review
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)
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 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)
Attached patch Bug1322085.patch - rev2 (obsolete) — Splinter Review
Fixed.

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=debd594868e034f6a0560fc06a19c64d198acf9c
Attachment #8821103 - Attachment is obsolete: true
Attachment #8822088 - Flags: review?(ntim.bugs)
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+
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 ?
(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.
Regarding // let forceSyncReflow = syncReflowNode.offsetWidth;
This was intentional because the variable is never used and thus causes an eslint error.
Flags: needinfo?(ntim.bugs)
Attached patch Bug1322085.patch - rev3 (obsolete) — Splinter Review
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)
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+
Flags: needinfo?(ntim.bugs)
Attached patch Bug1322085.patch - rev4 (obsolete) — Splinter Review
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)
(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 on attachment 8822220 [details] [diff] [review]
Bug1322085.patch - rev4

r+ assuming try will be green.
Attachment #8822220 - Flags: review?(ntim.bugs) → review+
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)
(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)
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.)
Attached patch Bug1322085.patch - rev5 (obsolete) — Splinter Review
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)
Attached patch Bug1322085.patch - rev5 (obsolete) — Splinter Review
Attachment #8827227 - Attachment is obsolete: true
Attachment #8827227 - Flags: review?(ntim.bugs)
Attachment #8827228 - Flags: review?(ntim.bugs)
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)
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/2b43e1793702
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.