Move the Tooltips to ES6 classes

RESOLVED FIXED in Firefox 58

Status

P3
enhancement
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: zer0, Assigned: bharatraghunthan9767, Mentored)

Tracking

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

unspecified
Firefox 58
good-first-bug
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
In bug 1378847 we have replaced the usage of SDK's extend with DevTools ones, but we should use ES6 classes instead.
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
Severity: normal → enhancement
(Reporter)

Comment 2

11 months 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)
(Assignee)

Comment 4

11 months 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)
(Reporter)

Comment 5

11 months 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-
(Reporter)

Updated

11 months ago
Flags: needinfo?(zer0)
(Assignee)

Comment 6

11 months ago
Are decorators allowed in the class definitions?
Flags: needinfo?(zer0)
(Reporter)

Comment 7

11 months 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)
(Assignee)

Comment 9

11 months 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)
(Assignee)

Updated

11 months ago
Attachment #8910116 - Flags: review?(pbrosset)
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)
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)
(Assignee)

Comment 14

11 months ago
Modified and created a new diff as per changes suggested from Patrick Brosset <:pbro>
Flags: needinfo?(pbrosset)
Blocks: 1402832
(Assignee)

Updated

11 months ago
Attachment #8910116 - Flags: feedback?(pbrosset)
(Assignee)

Updated

11 months ago
Flags: needinfo?(pbrosset)
(Reporter)

Comment 15

11 months 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-
(Reporter)

Updated

11 months ago
Flags: needinfo?(zer0)
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)
(Assignee)

Comment 17

11 months ago
How do you go about rebasing a patch? Does hg pull --rebase do the job cleanly?
Flags: needinfo?(zer0)
Comment hidden (mozreview-request)
(Reporter)

Comment 19

11 months 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+
(Reporter)

Updated

11 months ago
Flags: needinfo?(zer0)
(Reporter)

Updated

11 months ago
Assignee: nobody → bharatraghunthan9767
(Reporter)

Updated

11 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 20

11 months 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)

Comment 21

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

Comment 22

11 months ago
Thanks for working on this!
Flags: needinfo?(zer0)

Comment 23

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71dcce73cbdb
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.