1412269 - DevTools Framework needs to use ES6 classes so that we can switch it over to PureComponent

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P2)

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1412269 - DevTools Framework to ES6 classes

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

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8922771 [details]
Bug 1412269 - DevTools Framework to ES6 classes

https://reviewboard.mozilla.org/r/193926/#review200016

Please see my inline comments.

Otherwise works for me R+

Thanks Mike!

Honza

::: devtools/client/framework/components/toolbox-controller.js
(Diff revision 1)
> - * This component serves as a state controller for the toolbox React component. It's a
> - * thin layer for translating events and state of the outside world into the React update
> +  constructor(props, context) {
> +    super(props, context);
> - * cycle. This solution was used to keep the amount of code changes to a minimimum while
> - * adapting the existing codebase to start using React.
> - */
> -module.exports = createClass({

Please put back the comment for this component.

::: devtools/client/framework/components/toolbox-controller.js
(Diff revision 1)
> -module.exports = createClass({
> -  displayName: "ToolboxController",
>  
> -  getInitialState() {
> +    this.state = {
> -    // See the ToolboxToolbar propTypes for documentation on each of these items in state,
> -    // and for the defintions of the props that are expected to be passed in.

typo: defintions -> definitions

::: devtools/client/framework/components/toolbox-controller.js:43
(Diff revision 1)
> +    this.setDockButtonsEnabled = this.setDockButtonsEnabled.bind(this);
> +    this.setHostTypes = this.setHostTypes.bind(this);
> +    this.setCanCloseToolbox = this.setCanCloseToolbox.bind(this);
> +    this.setPanelDefinitions = this.setPanelDefinitions.bind(this);
> +    this.setToolboxButtons = this.setToolboxButtons.bind(this);
> +    this.setCanMinimize = this.setCanMinimize.bind(this);

Do we have to bind all these methods?

::: devtools/client/framework/components/toolbox-toolbar.js
(Diff revision 1)
> - * This is the overall component for the toolbox toolbar. It is designed to not know how
> - * the state is being managed, and attempts to be as pure as possible. The
> +  static get propTypes() {
> +    return {
> - * ToolboxController component controls the changing state, and passes in everything as
> - * props.
> - */
> -module.exports = createClass({

Comment for the component got removed, please put it back.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8922771 [details]
Bug 1412269 - DevTools Framework to ES6 classes

https://reviewboard.mozilla.org/r/193926/#review200016

> Do we have to bind all these methods?

This is what createClass does under the hood. When it comes to custom methods (not React built-in methods), if we want `this` inside a class to refer to the class then it has to be bound.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8922771 [details]
Bug 1412269 - DevTools Framework to ES6 classes

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23cedc247ac9
DevTools Framework to ES6 classes r=Honza

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/23cedc247ac9