1393453 - Move the Tooltips to ES6 classes

Reporter

Description

•

7 years ago

In bug 1378847 we have replaced the usage of SDK's extend with DevTools ones, but we should use ES6 classes instead.

Patrick Brosset <:pbro>

Comment 1

•

7 years ago

Trying to make this bug actionable.

Is this related to these files?
devtools/client/shared/widgets/tooltip/Tooltip.js
devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
devtools/client/shared/widgets/tooltip/SwatchCubicBezierTooltip.js
devtools/client/shared/widgets/tooltip/SwatchFilterTooltip.js

If so, then the work required here is to replace the prototype-based classes in these files with native ES6 classes instead [1].
This looks like a relatively easy bug for someone who wants to try their look on some technical refactoring.
Matteo, can you confirm what I said before, and do you mind setting yourself as a mentor on this bug?

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes

Flags: needinfo?(zer0)

Priority: -- → P3

Patrick Brosset <:pbro>

Updated

•

7 years ago

Severity: normal → enhancement

Matteo Ferretti [:zer0] [:matteo]

Reporter

Comment 2

•

7 years ago

(In reply to Patrick Brosset <:pbro> from comment #1)

> Is this related to these files?
> devtools/client/shared/widgets/tooltip/Tooltip.js
> devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
> devtools/client/shared/widgets/tooltip/SwatchCubicBezierTooltip.js
> devtools/client/shared/widgets/tooltip/SwatchFilterTooltip.js

Yes, at least those were the file we had in mind, I made this bug as follow up of bug 1378847, and I was thinking to solve it myself; but it could be indeed a good first bug!

Mentor: zer0

Flags: needinfo?(zer0)

Keywords: good-first-bug

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/181602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181602/

Bharat Raghunathan

Assignee

Comment 4

•

7 years ago

Pardon me for any linting errors, but my version of NodeJS is broken and I'm unable to update to the LTS one, hence ./mach eslint is not working

Flags: needinfo?(zer0)

Matteo Ferretti [:zer0] [:matteo]

Reporter

Comment 5

•

7 years ago

mozreview-review

Comment on attachment 8910116 [details]
Bug 1393453 - Moves Tooltips to ES6 classes retaining decorators

https://reviewboard.mozilla.org/r/181602/#review187020

Thanks for working on this!

There are few catch, though; I'm writing down a kind of template for migrating from prototype to class, hope it helps.

Here a basic example of usage of `extend`:

```js 
const { extend } = require("devtools/shared/extend");
const { Task } = require("devtools/shared/task");

/**
 * This is both MyClass definition and MyClass' constructor definition.
 */
function MyClass(document, inspector) {
  MyBaseClass.call(this, document);

  this.inspector = inspector;  
}

/**
 * Here we inherited from MyBaseClass
 */
MyClass.prototype = extend(MyBaseClass.prototype, {
  /**
   * This is a property defined in the prototype
   */
  defaultType: "none",
  
  /**
   * Overriding the MyBaseClass.prototype.show function,
   * it's async so we use `Task.async` as wrapper.
   */
  show: Task.async(function* () {
    console.log("do other stuff");
    
    // Call then parent class' show function
    yield MyBaseClass.prototype.show.call(this);

  }),

  destroy: function () {
    MyBaseClass.prototype.destroy.call(this);
    this.inspector = null;
  }
});
```

This is the equivalent using the latest ES functionalities:

```js 
// We have removed both `extend` and `Task` since we're not using those modules anymore

/**
 * This is MyClass definition where we also inherited from MyBaseClass
 */
class MyClass extends MyBaseClass {
  /**
   * This is MyClass' constructor definition.
   */ 
  constructor(document, inspector) {
    super(document);

    // since `class` body doesn't allow properties, we have to move them
    // inside the constructor.
    this.defaultType = "none",

    this.inspector = inspector;

  } // notice that we're not using commas between functions inside a `class`

  /**
   * Overriding the MyBaseClass' show function,
   * it's async. Decorators cannot be used on classes.
   * Since they also don't accept properties, we have to find another way.
   * Luckily, here we can replace `Task.async` with the native `async` keyword;
   * and `yield` with `await`.
   */
  async show() {
    console.log("do other stuff");
    
    // Call then parent class' show function
    await super.show();
  }

  destroy() {
    super.destroy();
    this.inspector = null;
  }
}
```

I hope I covered everything, let me know if you have any questions!

I would also suggest to run few tests for the tooltip (you can find them under "devtools/client/shared/test" folder, here you can find the instruction to run tests, in case you need them: http://docs.firefox-dev.tools/tests/mochitest-devtools.html)

::: devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip.js:85
(Diff revision 1)
>    /**
>     * Reports if the tooltip is currently shown
>     *
>     * @return {Boolean} True if the tooltip is displayed.
>     */
>    isVisible: function () {

This syntax is not allowed in class definition; you should switch to: `isVisible() {` instead; same for the other methods.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:16
(Diff revision 1)
>  const {Spectrum} = require("devtools/client/shared/widgets/Spectrum");
>  const SwatchBasedEditorTooltip = require("devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip");
>  const {LocalizationHelper} = require("devtools/shared/l10n");
>  const L10N = new LocalizationHelper("devtools/client/locales/inspector.properties");
>  
>  const {extend} = require("devtools/shared/extend");

Remember to remove this line in every module you refactored, since `extend` is not used anymore.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:38
(Diff revision 1)
>   * @param {InspectorPanel} inspector
>   *        The inspector panel, needed for the eyedropper.
>   * @param {Function} supportsCssColor4ColorFunction
>   *        A function for checking the supporting of css-color-4 color function.
>   */
>  function SwatchColorPickerTooltip(document,

This basically is becoming the class' constructor, so you will have:

```js
class SwatchColorPickerTooltip extends SwatchBasedEditorTooltip {
  constructor(document, inspector, {supportsCssColor4ColorFunction}) {
    super(document);
  
    this.inspector = inspector;
    // etc
  }
}
```

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:58
(Diff revision 1)
> -SwatchColorPickerTooltip.prototype = extend(SwatchBasedEditorTooltip.prototype, {
> +class SwatchColorPickerTooltip extends SwatchBasedEditorTooltip {
>    /**
>     * Fill the tooltip with a new instance of the spectrum color picker widget
>     * initialized with the given color, and return the instance of spectrum
>     */
>    setColorPickerContent: function (color) {

This syntax is not allowed in class definition; you should switch to: `setColorPickerContent(color) {` instead; same for the other methods.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:118
(Diff revision 1)
>  
>      // only enable contrast if it is compatible and if the type of property is color.
>      this.spectrum.contrastEnabled = (name === "color") && this.isContrastCompatible;
>  
>      // Call then parent class' show function
> -    yield SwatchBasedEditorTooltip.prototype.show.call(this);
> +    yield SwatchBasedEditorTooltip.show.call(this);

You can use `super` instead:
```js
yield super.show();
```

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:213
(Diff revision 1)
>    isEditing: function () {
>      return this.tooltip.isVisible() || this.eyedropperOpen;
>    },
>  
>    destroy: function () {
> -    SwatchBasedEditorTooltip.prototype.destroy.call(this);
> +    SwatchBasedEditorTooltip.destroy.call(this);

You can use `super` here too.

::: devtools/client/shared/widgets/tooltip/SwatchCubicBezierTooltip.js:43
(Diff revision 1)
>    /**
>     * Fill the tooltip with a new instance of the cubic-bezier widget
>     * initialized with the given value, and return a promise that resolves to
>     * the instance of the widget
>     */
>    setCubicBezierContent: function (bezier) {

Same as above, the syntax should be `setCubicBezierContent(bezier) {`; and for the rest of the methods too.

::: devtools/client/shared/widgets/tooltip/SwatchCubicBezierTooltip.js:67
(Diff revision 1)
>  
>    /**
>     * Overriding the SwatchBasedEditorTooltip.show function to set the cubic
>     * bezier curve in the widget
>     */
>    show: Task.async(function* () {

Since you can't use property definition, you have to use the new `async` and `await` keyword, instead.

If, for any reason, it doesn't work as expected – you will get exceptions running tests, or the tooltip is broken when used –, then we have probably to move the decorators from the class definition; or in the worst scenario we could just leave as is (so, no porting to ES6 classes).

But I would try the `async` and `await` approach, first.

::: devtools/client/shared/widgets/tooltip/SwatchCubicBezierTooltip.js:69
(Diff revision 1)
>     * Overriding the SwatchBasedEditorTooltip.show function to set the cubic
>     * bezier curve in the widget
>     */
>    show: Task.async(function* () {
>      // Call the parent class' show function
> -    yield SwatchBasedEditorTooltip.prototype.show.call(this);
> +    yield SwatchBasedEditorTooltip.show.call(this);

You can use `super` here.

::: devtools/client/shared/widgets/tooltip/SwatchCubicBezierTooltip.js:92
(Diff revision 1)
>      this.currentBezierValue.textContent = bezier + "";
>      this.preview(bezier + "");
>    },
>  
>    destroy: function () {
> -    SwatchBasedEditorTooltip.prototype.destroy.call(this);
> +    SwatchBasedEditorTooltip.destroy.call(this);

You can use `super` here too.

::: devtools/client/shared/widgets/tooltip/SwatchFilterTooltip.js:58
(Diff revision 1)
>      return new CSSFilterEditorWidget(container, filter, this._cssIsValid);
>    },
>  
>    show: Task.async(function* () {
>      // Call the parent class' show function
> -    yield SwatchBasedEditorTooltip.prototype.show.call(this);
> +    yield SwatchBasedEditorTooltip.show.call(this);

Use `super` here.

::: devtools/client/shared/widgets/tooltip/SwatchFilterTooltip.js:87
(Diff revision 1)
>  
>      this.preview();
>    },
>  
>    destroy: function () {
> -    SwatchBasedEditorTooltip.prototype.destroy.call(this);
> +    SwatchBasedEditorTooltip.destroy.call(this);

Use `super` here.

::: devtools/client/shared/widgets/tooltip/SwatchFilterTooltip.js:108
(Diff revision 1)
>     * @param {object} options
>     *        options to pass to the output parser, with
>     *          the option |filterSwatch| set.
>     */
>    addSwatch: function (swatchEl, callbacks, parser, options) {
> -    SwatchBasedEditorTooltip.prototype.addSwatch.call(this, swatchEl,
> +    SwatchBasedEditorTooltip.addSwatch.call(this, swatchEl,

Use `super` and also change the method definition (`addSwatch(...)`)

::: devtools/client/shared/widgets/tooltip/Tooltip.js:153
(Diff revision 1)
>        }
>      }
>    }
>  }
>  
> -Tooltip.prototype = {
> +class Tooltip {

Unfortunately you can't have (yet) properties in class definition, it means you have to add them in the constructor:

```js
class Tooltip {
  constructor(doc, {
      consumeOutsideClick = false,
      closeOnKeys = [ESCAPE_KEYCODE],
      noAutoFocus = true,
      closeOnEvents = [],
    } = {}) {
    
  EventEmitter.decorate(this);
  
  this.defaultPosition = "before_start";
  this.defaultOffsetX = 0;
  this.defaultOffsetY = 0;
  // etc
```

Attachment #8910116 - Flags: review?(zer0) → review-

Matteo Ferretti [:zer0] [:matteo]

Reporter

Updated

•

7 years ago

Flags: needinfo?(zer0)

Bharat Raghunathan

Assignee

Comment 6

•

7 years ago

Are decorators allowed in the class definitions?

Flags: needinfo?(zer0)

Matteo Ferretti [:zer0] [:matteo]

Reporter

Comment 7

•

7 years ago

(In reply to Bharat Raghunathan from comment #6)
> Are decorators allowed in the class definitions?

Unfortunately, not. There are proposal about implementing real decorators, but currently a `class` can have only methods, defined in the form of `method() { }`, so you can't wrap the function.

So, if the only decorator we have is the `Task.async`, we can try to refactor using the `async` keyword, as mentioned, if the decorator is something else (for example, like this one: http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/server-logger.js#72) then we have two choices:

1. Leave as is – do not port to the class syntactical sugar 
2. Have the class definition, but leave the decorators in the `prototype` (since the `class` is just sugar syntax, we can still mix them)

We prefer the first, to avoid confusion.

Flags: needinfo?(zer0)

Comment hidden (mozreview-request)

Comment on attachment 8910116 [details]
Bug 1393453 - Moves Tooltips to ES6 classes retaining decorators

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181602/diff/1-2/

Bharat Raghunathan

Assignee

Comment 9

•

7 years ago

Now I got ESLint to work, however the tests aren't running properly, so kindly forgive me if there are any blatant errors/exceptions which are caught because it was untested, however, the code has been linted to the best of my ability.

Flags: needinfo?(zer0)

Comment hidden (mozreview-request)

Comment on attachment 8910116 [details]
Bug 1393453 - Moves Tooltips to ES6 classes retaining decorators

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181602/diff/2-3/

Bharat Raghunathan

Assignee

Updated

•

7 years ago

Attachment #8910116 - Flags: review?(pbrosset)

Patrick Brosset <:pbro>

Comment 11

•

7 years ago

mozreview-review

Comment on attachment 8910116 [details]
Bug 1393453 - Moves Tooltips to ES6 classes retaining decorators

https://reviewboard.mozilla.org/r/181602/#review188266

::: devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip.js:28
(Diff revision 3)
> +   * Reports if the tooltip is currently shown
> +   *
> +   * @return {Boolean} True if the tooltip is displayed.

This comment seems incorrect. It might have been copy/pasted from another function.

::: devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip.js:85
(Diff revision 3)
> -  // that was clicked
> +    // that was clicked
> -  this.activeSwatch = null;
> +    this.activeSwatch = null;
>  
> -  this._onSwatchClick = this._onSwatchClick.bind(this);
> +    this._onSwatchClick = this._onSwatchClick.bind(this);
> -}
> +  }
> -
> +  isVisible() {

nit: please add an empty line between the end of the constructor method and the isVisible method.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:37
(Diff revision 3)
> +  /**
> +   * Fill the tooltip with a new instance of the spectrum color picker widget
> +   * initialized with the given color, and return the instance of spectrum
> +   */

Same comment here, you moved this comment from its original function (setColorPickerContent) to the constructor.
Also, please add an empty line to separate the constructor and the following function.

Attachment #8910116 - Flags: review?(pbrosset)

Patrick Brosset <:pbro>

Comment 12

•

7 years ago

I just did a very quick scan because Matteo is already reviewing this commit.
This looks pretty good to me. I just noted a few problems where you moved some jsdoc comment at the wrong place.
I'll let Matteo do the final review as he's already done one and he has better context.

Comment hidden (mozreview-request)

Comment on attachment 8910116 [details]
Bug 1393453 - Moves Tooltips to ES6 classes retaining decorators

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181602/diff/3-4/

Bharat Raghunathan

Assignee

Comment 14

•

7 years ago

Modified and created a new diff as per changes suggested from Patrick Brosset <:pbro>

Flags: needinfo?(pbrosset)

Soledad Penades [:sole] [:spenades]

Updated

•

7 years ago

Blocks: dt-polish-debt

Bharat Raghunathan

Assignee

Updated

•

7 years ago

Attachment #8910116 - Flags: feedback?(pbrosset)

Bharat Raghunathan

Assignee

Updated

•

7 years ago

Flags: needinfo?(pbrosset)

Matteo Ferretti [:zer0] [:matteo]

Reporter

Comment 15

•

7 years ago

mozreview-review

Comment on attachment 8910116 [details]
Bug 1393453 - Moves Tooltips to ES6 classes retaining decorators

https://reviewboard.mozilla.org/r/181602/#review188656

Thanks for addressing the my comments and patrick's comments!
There are few nits still, and it seems the patch needs to be rebased, since it doesn't apply clean (I got rejection on `SwatchColorPickerTooltip`), and before the r+ I'd like to run few tests.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:37
(Diff revision 4)
> -                                  inspector,
> -                                  {supportsCssColor4ColorFunction}) {
> -  SwatchBasedEditorTooltip.call(this, document);
>  
> -  this.inspector = inspector;
> +class SwatchColorPickerTooltip extends SwatchBasedEditorTooltip {
> +  /**

This comment seems to belong to `setColorPickerContent` not sure why you moved.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:51
(Diff revision 4)
> -  this._onSpectrumColorChange = this._onSpectrumColorChange.bind(this);
> +    this._onSpectrumColorChange = this._onSpectrumColorChange.bind(this);
> -  this._openEyeDropper = this._openEyeDropper.bind(this);
> +    this._openEyeDropper = this._openEyeDropper.bind(this);
> -  this.cssColor4 = supportsCssColor4ColorFunction();
> +    this.cssColor4 = supportsCssColor4ColorFunction();
> -}
> +  }
>  
> -SwatchColorPickerTooltip.prototype = extend(SwatchBasedEditorTooltip.prototype, {
> +  // Creating a spectrum instance. this.spectrum will always be a promise that

This comment belongs before `this.spectrum = this.setColorPickerContent([0, 0, 0, 1]);`

::: devtools/client/shared/widgets/tooltip/SwatchCubicBezierTooltip.js:27
(Diff revision 4)
>   */
> -function SwatchCubicBezierTooltip(document) {
> -  SwatchBasedEditorTooltip.call(this, document);
>  
> +class SwatchCubicBezierTooltip extends SwatchBasedEditorTooltip {
> +  /**

Same as above, it seems you moved the comment from the method to the constructor.

::: devtools/client/shared/widgets/tooltip/SwatchFilterTooltip.js:30
(Diff revision 4)
>   *        This can be obtained from "shared/fronts/css-properties.js".
>   */
> -function SwatchFilterTooltip(document, cssIsValid) {
> +
> +class SwatchFilterTooltip extends SwatchBasedEditorTooltip {
> +  /**
> +   * Fill the tooltip with a new instance of the CSSFilterEditorWidget

Ditto.

::: devtools/client/shared/widgets/tooltip/SwatchFilterTooltip.js:35
(Diff revision 4)
> +   * Fill the tooltip with a new instance of the CSSFilterEditorWidget
> +   * widget initialized with the given filter value, and return a promise
> +   * that resolves to the instance of the widget when ready.
> +   */
> +  constructor(document, cssIsValid) {
> -  SwatchBasedEditorTooltip.call(this, document);
> +    SwatchBasedEditorTooltip.call(this, document);

This should be `super(document);`

::: devtools/client/shared/widgets/tooltip/SwatchFilterTooltip.js:106
(Diff revision 4)
>     * @param {object} options
>     *        options to pass to the output parser, with
>     *          the option |filterSwatch| set.
>     */
> -  addSwatch: function (swatchEl, callbacks, parser, options) {
> -    SwatchBasedEditorTooltip.prototype.addSwatch.call(this, swatchEl,
> +  addSwatch(swatchEl, callbacks, parser, options) {
> +    super(swatchEl, callbacks);

This shuld be `super.addSwatch(stawtchEl, callbacks);`

::: devtools/client/shared/widgets/tooltip/Tooltip.js:106
(Diff revision 4)
> +    // px
> +    this.defaultOffsetX = 0;
> +    // px
> +    this.defaultOffsetY = 0;
> +    // px
> -  this.doc = doc;
> +    this.doc = doc;

I would add a new line before `this.doc`

Attachment #8910116 - Flags: review?(zer0) → review-

Matteo Ferretti [:zer0] [:matteo]

Reporter

Updated

•

7 years ago

Flags: needinfo?(zer0)

Patrick Brosset <:pbro>

Comment 16

•

7 years ago

Comment on attachment 8910116 [details]
Bug 1393453 - Moves Tooltips to ES6 classes retaining decorators

Clearing the feedback flag since Matteo is already a reviewer on this patch. No need to ping 2 people.

Attachment #8910116 - Flags: feedback?(pbrosset)

Bharat Raghunathan

Assignee

Comment 17

•

7 years ago

How do you go about rebasing a patch? Does hg pull --rebase do the job cleanly?

Flags: needinfo?(zer0)

Comment hidden (mozreview-request)

Comment on attachment 8910116 [details]
Bug 1393453 - Moves Tooltips to ES6 classes retaining decorators

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181602/diff/4-5/

Matteo Ferretti [:zer0] [:matteo]

Reporter

Comment 19

•

7 years ago

mozreview-review

Comment on attachment 8910116 [details]
Bug 1393453 - Moves Tooltips to ES6 classes retaining decorators

https://reviewboard.mozilla.org/r/181602/#review188798

It seems you figure it out since now is applying properly! :)
I locally executed the tests under "devtools/client/shared/test" and it seems fine.
We can land this once the scheduled try build (here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f1220f1fc504ed9dd39bf3c7cd673451bbdb7b9) is green.

Thanks for working on this!

Attachment #8910116 - Flags: review?(zer0) → review+

Matteo Ferretti [:zer0] [:matteo]

Reporter

Updated

•

7 years ago

Flags: needinfo?(zer0)

Matteo Ferretti [:zer0] [:matteo]

Reporter

Updated

•

7 years ago

Assignee: nobody → bharatraghunthan9767

Matteo Ferretti [:zer0] [:matteo]

Reporter

Updated

•

7 years ago

Status: NEW → ASSIGNED

Bharat Raghunathan

Assignee

Comment 20

•

7 years ago

The tests on the Try Server seem to have finished, could you please look into it and try landing the patch?

Flags: needinfo?(zer0)

Pulsebot

Comment 21

•

7 years ago

Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71dcce73cbdb
Moves Tooltips to ES6 classes retaining decorators r=zer0

Matteo Ferretti [:zer0] [:matteo]

Reporter

Comment 22

•

7 years ago

Thanks for working on this!

Flags: needinfo?(zer0)

Phil Ringnalda (:philor)

Comment 23

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/71dcce73cbdb

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox58: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 58

BMO Automation

Updated

•

6 years ago

Product: Firefox → DevTools