1336198 - Use the react/redux box model in the computed view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

This purpose of this bug is the following:
(1) Replace the existing box model in the computed view with new box model written in react/redux
(2) Make all existing unit tests run correctly for the new box model
(3) Remove deprecated box model files

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Redux store should be initialized at the Inspector level.

We need a common share Redux Store among all our tools, so, it is best to move the initialization of the Store into the Inspector and use that store in all our side panels.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Remove deprecated boxmodel stylesheet.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Remove the deprecated box model source file.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Remove the deprecated box model markup.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Remove initialization of the deprecated box model.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 6: Refactor box model component into boxmodel/.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Refactor box model logic into box-model.js.

Comment 9

[Julian Descottes [:jdescottes]]

I will start reviewing the changesets, but quickly testing your patches it seems like we still need to add the new boxmodel in the computed-view right?

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8835731 [details] [diff] [review]
Part 1: Redux store should be initialized at the Inspector level.

Review of attachment 8835731 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks.

::: devtools/client/inspector/reducers.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

We are moving reducers/stores from inspector/layout/ up to inspector/. 
Most of the inpsector is not using react/reduc so I think it would be nice to 
add a small comment to explain that this file exposes redux reducers etc ...

Or we could group all those files in a inspector/redux folder.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8835757 [details] [diff] [review]
Part 4: Remove the deprecated box model markup.

Review of attachment 8835757 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like we are also missing an update of the old tests.

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8835758 [details] [diff] [review]
Part 5: Remove initialization of the deprecated box model.

Review of attachment 8835758 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/computed/computed.js
@@ -1415,5 @@
>    this.document = window.document;
>  
>    this.computedView = new CssComputedView(this.inspector, this.document,
>      this.inspector.pageStyle);
> -  this.boxModelView = new BoxModelView(this.inspector, this.document);

This is still used in many tests but I assume this will come in another changeset.

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8835870 [details] [diff] [review]
Part 7: Refactor box model logic into box-model.js.

Review of attachment 8835870 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think boxmodelview/BoxModelView is really appropriate for the current role of box-model.js.

If I understand correctly, it is a shared instance that is listening to reflow events and updates a store. 
It doesn't generate any markup, doesn't render any component so I don't really why we should consider it a view.

There are also other parts which I find a bit weird here. I think listening to SidebarSelect events should be the responsibility
of the layout view and the computed view, which should then as to the boxmodel(controller?) to track/untrack reflows. 

I don't think that, as a widget, the boxmodel should be aware of all the panels where it might be used.
I'm clearing my review flag here based on the fact that I think these questions will become clearer once we start
using this box-model in a second spot, the computed view.

(which is mandatory before we can land any of this anyway :))

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fee5872f8af4
Part 1: Redux store should be initialized at the Inspector level. r=jdescottes

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fee5872f8af4

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8 - Use the react/redux box model in the computed view.

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8837020 [details] [diff] [review]
Part 8 - Use the react/redux box model in the computed view.

Review of attachment 8837020 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! 

Some comments, clearing my review flag because we need to discuss some items.

Most importantly a bug where reflows are not tracked when doing: 
- open devtools with the inspector selected by default and either the computed/layout panel selected by default
- select a node that has dimensions driven by the screen size (such as the body of about:home)
- resize the window
-> BoxModel is not updated

Then we already discussed about it when you introduced the new box model widget:
- tabbing between inputs no longer works
- the geometry editor needs to be reimplemented
-> Need to decide if we fix it here or in follow ups.

I still feel like BoxModelView is a very confusing name for a singleton that is tracking reflows and providing props, 
but I won't hold the review on that.

Are you also planning to migrate the existing tests for the old boxmodel?

::: devtools/client/inspector/boxmodel/components/BoxModelApp.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const { addons, createClass, createFactory, DOM: dom, PropTypes } =

nit: dom not used

@@ +35,5 @@
> +
> +  render() {
> +    return dom.div(
> +      {
> +        className: "",

nit: remove empty classname ?

Is the wrapping div even necessary here?

::: devtools/client/inspector/computed/computed.js
@@ +626,5 @@
> +      Provider,
> +      { store: this.store },
> +      BoxModelApp({
> +        showBoxModelProperties: false,
> +        onHideBoxModelHighlighter: () => onHideBoxModelHighlighter(),

Do we really need to wrap the methods here?

Comment 18

[Julian Descottes [:jdescottes]]

Flagging qe-verify +, since we are replacing the existing box-model widget in the computed view with a new one, it would be nice to have some testing done on this when it's ready to land.

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Refactor box model logic into box-model.js. [1.0]

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8 - Use the react/redux box model in the computed view. [1.0]

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Use existing box model tests with the new react/redux box model.

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8837724 [details] [diff] [review]
Part 7: Refactor box model logic into box-model.js. [1.0]

Review of attachment 8837724 [details] [diff] [review]:
-----------------------------------------------------------------

R+ with my comments addressed!

::: devtools/client/inspector/inspector.js
@@ +22,5 @@
>  const Menu = require("devtools/client/framework/menu");
>  const MenuItem = require("devtools/client/framework/menu-item");
>  
>  const {HTMLBreadcrumbs} = require("devtools/client/inspector/breadcrumbs");
> +const BoxModelView = require("devtools/client/inspector/boxmodel/box-model");

BoxModelView -> BoxModel

@@ +573,5 @@
>        defaultTab == "computedview");
>  
>      this.ruleview = new RuleViewTool(this, this.panelWin);
>      this.computedview = new ComputedViewTool(this, this.panelWin);
> +    this.boxmodelview = new BoxModelView(this, this.panelWin);

this.boxmodelview -> this.boxmodel (and update call sites accordingly).

::: devtools/client/inspector/layout/layout.js
@@ +76,5 @@
>  
> +      onHideBoxModelHighlighter: () => onHideBoxModelHighlighter(),
> +      onShowBoxModelEditor: (element, event, property) =>
> +        onShowBoxModelEditor(element, event, property),
> +      onShowBoxModelHighlighter: (options = {}) => onShowBoxModelHighlighter(options),

why the extra wrapping for the 3 methods here?

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8837749 [details] [diff] [review]
Part 8 - Use the react/redux box model in the computed view. [1.0]

Review of attachment 8837749 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8837769 [details] [diff] [review]
Part 9: Use existing box model tests with the new react/redux box model.

Review of attachment 8837769 [details] [diff] [review]:
-----------------------------------------------------------------

We still have references to old-* classes in :
- browser_computed_search-filter
- browser_computed_search-filter_clear
- browser_inspector_textbox-menu.js

R+ with a green try and comments addressed.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
@@ +115,5 @@
>      let marginRight = this.getMarginValue("margin-right", "right");
>      let marginBottom = this.getMarginValue("margin-bottom", "bottom");
>      let marginLeft = this.getMarginValue("margin-left", "left");
>  
> +    height = this.getHeightValue(height);

good catch! how did we miss this with the previous review :/

::: devtools/client/inspector/boxmodel/test/browser.ini
@@ +20,5 @@
>  [browser_boxmodel_editablemodel_border.js]
>  [browser_boxmodel_editablemodel_stylerules.js]
>  [browser_boxmodel_guides.js]
> +# [browser_boxmodel_navigation.js]
> +# Disabled keyboard navigations need to be reimplemented (bug 1336198)

use skip-if true. It seems to be consistently used to disable tests in devtools

@@ +25,4 @@
>  [browser_boxmodel_rotate-labels-on-sides.js]
>  [browser_boxmodel_sync.js]
> +# [browser_boxmodel_tooltips.js]
> +# Disabled source rule tooltip needs to be reimplemented (bug 1336198)

use skip-if true

::: devtools/client/inspector/boxmodel/test/browser_boxmodel.js
@@ +158,3 @@
>    yield testActor.setAttribute("div", "style",
>                                 "height:150px;padding-right:50px;");
> +  yield waitForUpdate(inspector);

Why change this? The previous implementation is less prone to race conditions, right?

::: devtools/client/inspector/boxmodel/test/browser_boxmodel_rotate-labels-on-sides.js
@@ +40,3 @@
>      let isLong = elt.textContent.length > LONG_TEXT_ROTATE_LIMIT;
>      let classList = elt.parentNode.classList;
>      let canBeRotated = classList.contains("old-boxmodel-left") ||

update references to oldboxmodel here

::: devtools/client/inspector/boxmodel/test/browser_boxmodel_tooltips.js
@@ +100,5 @@
>      info("Selecting " + selector + " and checking the values tooltips");
>      yield selectNode(selector, inspector);
>  
>      info("Iterate over all values");
> +    // TODO Needs to be fixed.

Let's add some details about what should be fixed or remove the comment.

::: devtools/client/inspector/boxmodel/test/browser_boxmodel_update-in-iframes.js
@@ +53,2 @@
>    yield setStyleInIframe1(testActor, "p", "width", "200px");
> +  yield waitForUpdate(inspector);

same question: why change the pattern here?

::: devtools/client/inspector/boxmodel/test/head.js
@@ +57,5 @@
>  
>      return {
>        toolbox: data.toolbox,
>        inspector: data.inspector,
> +      view: data.inspector.computedview,

It would make sense to rename openBoxModelView now, as there are 2 views that can display the boxmodel widget. 

Fine with this being a followup.

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Refactor box model logic into box-model.js. [2.0]

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8 - Use the react/redux box model in the computed view. [2.0]

Rebased

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Julian Descottes [:jdescottes] from comment #24)
> Comment on attachment 8837769 [details] [diff] [review]
> Part 9: Use existing box model tests with the new react/redux box model.
> 
> Review of attachment 8837769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We still have references to old-* classes in :
> - browser_computed_search-filter
> - browser_computed_search-filter_clear
> - browser_inspector_textbox-menu.js
> 
> R+ with a green try and comments addressed.
> 
> ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
> @@ +115,5 @@
> >      let marginRight = this.getMarginValue("margin-right", "right");
> >      let marginBottom = this.getMarginValue("margin-bottom", "bottom");
> >      let marginLeft = this.getMarginValue("margin-left", "left");
> >  
> > +    height = this.getHeightValue(height);
> 
> good catch! how did we miss this with the previous review :/
> 
> ::: devtools/client/inspector/boxmodel/test/browser.ini
> @@ +20,5 @@
> >  [browser_boxmodel_editablemodel_border.js]
> >  [browser_boxmodel_editablemodel_stylerules.js]
> >  [browser_boxmodel_guides.js]
> > +# [browser_boxmodel_navigation.js]
> > +# Disabled keyboard navigations need to be reimplemented (bug 1336198)
> 
> use skip-if true. It seems to be consistently used to disable tests in
> devtools
> 
> @@ +25,4 @@
> >  [browser_boxmodel_rotate-labels-on-sides.js]
> >  [browser_boxmodel_sync.js]
> > +# [browser_boxmodel_tooltips.js]
> > +# Disabled source rule tooltip needs to be reimplemented (bug 1336198)
> 
> use skip-if true
> 
> ::: devtools/client/inspector/boxmodel/test/browser_boxmodel.js
> @@ +158,3 @@
> >    yield testActor.setAttribute("div", "style",
> >                                 "height:150px;padding-right:50px;");
> > +  yield waitForUpdate(inspector);
> 
> Why change this? The previous implementation is less prone to race
> conditions, right?
> 
> :::
> devtools/client/inspector/boxmodel/test/browser_boxmodel_rotate-labels-on-
> sides.js
> @@ +40,3 @@
> >      let isLong = elt.textContent.length > LONG_TEXT_ROTATE_LIMIT;
> >      let classList = elt.parentNode.classList;
> >      let canBeRotated = classList.contains("old-boxmodel-left") ||
> 
> update references to oldboxmodel here
> 
> ::: devtools/client/inspector/boxmodel/test/browser_boxmodel_tooltips.js
> @@ +100,5 @@
> >      info("Selecting " + selector + " and checking the values tooltips");
> >      yield selectNode(selector, inspector);
> >  
> >      info("Iterate over all values");
> > +    // TODO Needs to be fixed.
> 
> Let's add some details about what should be fixed or remove the comment.
> 
> :::
> devtools/client/inspector/boxmodel/test/browser_boxmodel_update-in-iframes.js
> @@ +53,2 @@
> >    yield setStyleInIframe1(testActor, "p", "width", "200px");
> > +  yield waitForUpdate(inspector);
> 
> same question: why change the pattern here?
> 
> ::: devtools/client/inspector/boxmodel/test/head.js
> @@ +57,5 @@
> >  
> >      return {
> >        toolbox: data.toolbox,
> >        inspector: data.inspector,
> > +      view: data.inspector.computedview,
> 
> It would make sense to rename openBoxModelView now, as there are 2 views
> that can display the boxmodel widget. 
> 
> Fine with this being a followup.

We need to add "yield waitForUpdate(inspector);" because we need to wait for the state dispatch and the react UI to update. This is a new race condition that we need to care about because we need to wait for the react/redux state changes.

Comment 28

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8 - Use the react/redux box model in the computed view. [3.0]

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Use existing box model tests with the new react/redux box model. [1.0]

Comment 30

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Use existing box model tests with the new react/redux box model. [2.0]

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23c40e8dac0f
Part 2: Remove deprecated boxmodel stylesheet. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef60037208a8
Part 3: Remove the deprecated box model source file. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/94eceadb11ff
Part 4: Remove the deprecated box model markup. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/37b47edaa183
Part 5: Remove initialization of the deprecated box model. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/2644c783041d
Part 6: Refactor box model component into boxmodel/. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c3b3e82e0bb
Part 7: Refactor box model logic into box-model.js. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad616aa3a65e
Part 8: Use the react/redux box model in the computed view. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/c316dd475a13
Part 9: Use existing box model tests with the new react/redux box model. r=jdescottes

Comment 32

[Julian Descottes [:jdescottes]]

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #27)
> 
> We need to add "yield waitForUpdate(inspector);" because we need to wait for
> the state dispatch and the react UI to update. This is a new race condition
> that we need to care about because we need to wait for the react/redux state
> changes.

Sorry but I don't get it. Before we were doing: 

> let wait = waitForUpdate(inspector);
> // do something async
> yield wait;

Which was replaced by 

> // do something async
> yield waitForUpdate(inspector);

In my opinion it makes the test more exposed to race conditions. We normally create the listener before triggering the action that will fire it.

Comment 33

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

Backed out for timeouts in devtools' boxmodel tests, e.g. browser_boxmodel_editablemodel_stylerules.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a6f8f22533b7f5ab14b4f1b1876af4f494b944
https://hg.mozilla.org/integration/mozilla-inbound/rev/545fbc653eebd919e12fa936fd2b424dd3e9fff1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bcbea3ffdef8d9d8d148879eff2381756fd8f1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5635c67b8893f86e4472f05e39cdcf2b79c3a2ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/3218b3125d95baf1d86e8e719e29c48494d425ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/685bad917a7545c75d9b5a5ca12eafb74e46a77e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae045b12a66fb49c32739661e1331db3bea47f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/00e51e881d963a87d7a15672ea71bab6020b21d3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c316dd475a13498298f2672944328724162e842e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log (example): https://treeherder.mozilla.org/logviewer.html#?job_id=78081182&repo=mozilla-inbound

[task 2017-02-16T21:44:56.561298Z] 21:44:56     INFO - TEST-PASS | devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel_stylerules.js | Should have updated the border. - 
[task 2017-02-16T21:44:56.564589Z] 21:44:56     INFO - TEST-PASS | devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel_stylerules.js | Should be the right border-bottom-width. - 
[task 2017-02-16T21:44:56.566769Z] 21:44:56     INFO - TEST-PASS | devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel_stylerules.js | Should have the right value in the box model. - 
[task 2017-02-16T21:44:56.568641Z] 21:44:56     INFO - Test that shorthand properties are parsed correctly
[task 2017-02-16T21:44:56.570650Z] 21:44:56     INFO - TEST-PASS | devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel_stylerules.js | Should have the right padding - 
[task 2017-02-16T21:44:56.572689Z] 21:44:56     INFO - Selecting the node for '#div3'
[task 2017-02-16T21:44:56.574401Z] 21:44:56     INFO - Buffered messages finished
[task 2017-02-16T21:44:56.577630Z] 21:44:56     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel_stylerules.js | Test timed out - 
[task 2017-02-16T21:44:56.578858Z] 21:44:56     INFO - JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/boxmodel/box-model.js, line 184: TypeError: toolbox is null
[task 2017-02-16T21:44:56.579893Z] 21:44:56     INFO - JavaScript error: chrome://devtools/content/inspector/inspector.js, line 154: TypeError: this._toolbox is null

Comment 34

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Use existing box model tests with the new react/redux box model. [3.0]

Comment 35

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e55c48f9609784a42665f6d6e37881b33baabc1

Comment 36

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Use existing box model tests with the new react/redux box model. [4.0]

Comment 37

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f1ccd5bf83fe4ecc95ade30dcab44f8766d064e

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

Various try builds, but still test timed out failures for linux. Considering disabling the tests for linux in the meantime.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ca7685001d023e29b2dbbcbcd8cadb0e713c1d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef7e3602ed6d7a1068f1fcc57836648c77ec407e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7931fedc2f8a2aab5e268a6206f65ef45ba06249
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f1ccd5bf83fe4ecc95ade30dcab44f8766d064e

Comment 39

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Use existing box model tests with the new react/redux box model. [5.0]

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e507c03b6cd
Part 2: Remove deprecated boxmodel stylesheet. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca5d4226787
Part 3: Remove the deprecated box model source file. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/e00c4a1694c4
Part 4: Remove the deprecated box model markup. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7101889b2ec
Part 5: Remove initialization of the deprecated box model. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/03b3126da4af
Part 6: Refactor box model component into boxmodel/. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/29efd4e3acf0
Part 7: Refactor box model logic into box-model.js. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d689c41408
Part 8: Use the react/redux box model in the computed view. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/148f39b53d9c
Part 9: Use existing box model tests with the new react/redux box model. r=jdescottes

Comment 41

[Iris Hsiao [:ihsiao]]

sorry had to back this out for devtool failures in browser_boxmodel.js, like https://treeherder.mozilla.org/logviewer.html#?job_id=78713885&repo=mozilla-inbound&lineNumber=6608

Comment 42

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbf5e2d59850
Backed out 8 changesets for devtool failures in browser_boxmodel.js

Comment 43

[Julian Descottes [:jdescottes]]

Gabriel, if you need to reproduce the timeouts locally you can use the linux build VM: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Using_the_VM

I just tried quickly and I get timeouts on tests pretty easily. Not sure I will have time to investigate more today, but I will report if I find anything.

Comment 44

[Julian Descottes [:jdescottes]]

The issue comes from the selectNode wrapper in boxmodel's head.js. This is linked to the pattern we discussed during the review:

> yield _selectNode(node, inspector, reason);
> yield waitForUpdate(inspector);

When the test fails, the boxmodel-view-updated is fired before we call waitForUpdate. Now the tricky part here is that if we swap this for 

> let onUpdate = waitForUpdate(inspector);
> yield _selectNode(node, inspector, reason);
> yield onUpdate;

Then we get some test failures. I focused on browser_boxmodel_editablemodel_stylerules.js to investigate, and it seems that the first time we select a node, the boxmodel-view-updated event is fired twice. 

My advice would be to fix the pattern used in selectNode and find out why we have an inconsistent number of events fired (or to update the tests so that they can pass the number of events expected, but that sounds super hacky)

Comment 45

[Julian Descottes [:jdescottes]]

Attachment: fix intermittent timeout example

If it helps, here's a patch that fixes one of the tests, but I'd prefer if we had a more generic approach.

Now that we have a good grasp of why this is failing, let's avoid disabling tests and fix this properly.

Comment 46

[Julian Descottes [:jdescottes]]

Attachment: Bug 1336198 - Part 2: Remove deprecated boxmodel stylesheet.

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

Comment 47

[Julian Descottes [:jdescottes]]

Attachment: Bug 1336198 - Part 3: Remove the deprecated box model source file.

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

Comment 48

[Julian Descottes [:jdescottes]]

Attachment: Bug 1336198 - Part 4: Remove the deprecated box model markup.

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

Comment 49

[Julian Descottes [:jdescottes]]

Attachment: Bug 1336198 - Part 5: Remove initialization of the deprecated box model.

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

Comment 50

[Julian Descottes [:jdescottes]]

Attachment: Bug 1336198 - Part 6: Refactor box model component into boxmodel/.

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

Comment 51

[Julian Descottes [:jdescottes]]

Attachment: Bug 1336198 - Part 7: Refactor box model logic into box-model.js.

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

Comment 52

[Julian Descottes [:jdescottes]]

Attachment: Bug 1336198 - Part 8: Use the react/redux box model in the computed view.

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

Comment 53

[Julian Descottes [:jdescottes]]

Attachment: Bug 1336198 - Part 9: Use existing box model tests with the new react/redux box model.

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

Comment 54

[Julian Descottes [:jdescottes]]

Comment on attachment 8840015 [details]
Bug 1336198 - Part 2: Remove deprecated boxmodel stylesheet.

https://reviewboard.mozilla.org/r/114572/#review116012

forwarding r+

Comment 55

[Julian Descottes [:jdescottes]]

Comment on attachment 8840016 [details]
Bug 1336198 - Part 3: Remove the deprecated box model source file.

https://reviewboard.mozilla.org/r/114574/#review116014

Comment 56

[Julian Descottes [:jdescottes]]

Comment on attachment 8840017 [details]
Bug 1336198 - Part 4: Remove the deprecated box model markup.

https://reviewboard.mozilla.org/r/114576/#review116016

Comment 57

[Julian Descottes [:jdescottes]]

Comment on attachment 8840018 [details]
Bug 1336198 - Part 5: Remove initialization of the deprecated box model.

https://reviewboard.mozilla.org/r/114578/#review116018

Comment 58

[Julian Descottes [:jdescottes]]

Comment on attachment 8840019 [details]
Bug 1336198 - Part 6: Refactor box model component into boxmodel/.

https://reviewboard.mozilla.org/r/114580/#review116020

Comment 59

[Julian Descottes [:jdescottes]]

Comment on attachment 8840020 [details]
Bug 1336198 - Part 7: Refactor box model logic into box-model.js.

https://reviewboard.mozilla.org/r/114582/#review116022

Comment 60

[Julian Descottes [:jdescottes]]

Comment on attachment 8840021 [details]
Bug 1336198 - Part 8: Use the react/redux box model in the computed view.

https://reviewboard.mozilla.org/r/114584/#review116030

Comment 61

[Julian Descottes [:jdescottes]]

Comment on attachment 8840022 [details]
Bug 1336198 - Part 9: Use existing box model tests with the new react/redux box model.

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

Comment 62

[Julian Descottes [:jdescottes]]

Attachment: Bug 1336198 - part10: fix race condition in boxmodel tests;

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

Comment 63

[Julian Descottes [:jdescottes]]

Comment on attachment 8840022 [details]
Bug 1336198 - Part 9: Use existing box model tests with the new react/redux box model.

https://reviewboard.mozilla.org/r/114586/#review116098

Comment 64

[Julian Descottes [:jdescottes]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf84030f4f00c7342a06188b0f23b6b8db9de2d2

Comment 65

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8840075 [details]
Bug 1336198 - part10: fix race condition in boxmodel tests;

https://reviewboard.mozilla.org/r/114610/#review116260

Looks good. Sadly, still failing on Windows.

::: devtools/client/inspector/test/shared-head.js:57
(Diff revision 1)
>    let {toolbox, inspector, testActor} = yield openInspector();
>  
>    info("Selecting the " + id + " sidebar");
> +
> +  if (id === "computedview" || id === "layoutview") {
> +    // The layout and computed viewS should wait until the box-model widget is ready.

s/viewS/views

Comment 66

[Julian Descottes [:jdescottes]]

I think it's just because we don't wait for boxmodel-view-updated when navigating to a page in this particular test.

Did some retriggers to see how frequent the failure really is, and sent a new try push that also waits for boxmodel-view-updated: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bab39be05867167b9a100ae34f66411cb2be6a6

Comment 67

[Julian Descottes [:jdescottes]]

Comment on attachment 8840075 [details]
Bug 1336198 - part10: fix race condition in boxmodel tests;

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

Comment 68

[Julian Descottes [:jdescottes]]

Comment on attachment 8840075 [details]
Bug 1336198 - part10: fix race condition in boxmodel tests;

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

Comment 69

[Julian Descottes [:jdescottes]]

The new try run is much better but we still have ~20% of failures on Windows e10s. I managed to reproduce it somehow on Linux, and I think it's linked to the fact that dispatching actions can be asynchronous.

Simply waiting for boxmodel-view-updated might not be enough. Here's a new try run where we also wait for the action to be dispatched in `waitForUpdate`: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cdd1f824b894bf21fa620e890e3a876a9f92a60

Comment 70

[Julian Descottes [:jdescottes]]

Last iteration: we now fire a different event when the boxmodel-view-update was linked to a new selection.
Try is amazingly green so far, even with many retriggers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a9d4ba4ada015779208970fdeff34001270c27d

I'll cleanup the patch and submit for review.

Comment 71

[Julian Descottes [:jdescottes]]

Comment on attachment 8840075 [details]
Bug 1336198 - part10: fix race condition in boxmodel tests;

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

Comment 72

[Julian Descottes [:jdescottes]]

Comment on attachment 8840075 [details]
Bug 1336198 - part10: fix race condition in boxmodel tests;

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

Comment 73

[Julian Descottes [:jdescottes]]

Comment on attachment 8840015 [details]
Bug 1336198 - Part 2: Remove deprecated boxmodel stylesheet.

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

Comment 74

[Julian Descottes [:jdescottes]]

Comment on attachment 8840016 [details]
Bug 1336198 - Part 3: Remove the deprecated box model source file.

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

Comment 75

[Julian Descottes [:jdescottes]]

Comment on attachment 8840017 [details]
Bug 1336198 - Part 4: Remove the deprecated box model markup.

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

Comment 76

[Julian Descottes [:jdescottes]]

Comment on attachment 8840018 [details]
Bug 1336198 - Part 5: Remove initialization of the deprecated box model.

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

Comment 77

[Julian Descottes [:jdescottes]]

Comment on attachment 8840019 [details]
Bug 1336198 - Part 6: Refactor box model component into boxmodel/.

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

Comment 78

[Julian Descottes [:jdescottes]]

Comment on attachment 8840020 [details]
Bug 1336198 - Part 7: Refactor box model logic into box-model.js.

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

Comment 79

[Julian Descottes [:jdescottes]]

Comment on attachment 8840021 [details]
Bug 1336198 - Part 8: Use the react/redux box model in the computed view.

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

Comment 80

[Julian Descottes [:jdescottes]]

Comment on attachment 8840022 [details]
Bug 1336198 - Part 9: Use existing box model tests with the new react/redux box model.

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

Comment 81

[Julian Descottes [:jdescottes]]

Comment on attachment 8840075 [details]
Bug 1336198 - part10: fix race condition in boxmodel tests;

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

Comment 82

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6710bb42058
Part 2: Remove deprecated boxmodel stylesheet. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/0461bc95ee7f
Part 3: Remove the deprecated box model source file. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c88ae1e9508
Part 4: Remove the deprecated box model markup. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d3c4bfc729
Part 5: Remove initialization of the deprecated box model. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0df0213c72b
Part 6: Refactor box model component into boxmodel/. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7e1c96ccf87
Part 7: Refactor box model logic into box-model.js. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/a174ae51463a
Part 8: Use the react/redux box model in the computed view. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1624f6a8087
Part 9: Use existing box model tests with the new react/redux box model. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd85a5aafa9f
part10: fix race condition in boxmodel tests;r=gl

Comment 83

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b0ddb1fd71
Backed out 9 changesets . r=backout

Comment 84

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Refactor box model logic into box-model.js. [3.0]

Comment 85

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Refactor box model logic into box-model.js. [4.0]

Comment 86

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f199785e14a4
Part 2: Remove deprecated boxmodel stylesheet. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/c856a31d1f71
Part 3: Remove the deprecated box model source file. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1781f03570d
Part 4: Remove the deprecated box model markup. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed4ff543cd8
Part 5: Remove initialization of the deprecated box model. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8c5cc596e19
Part 6: Refactor box model component into boxmodel/. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/e096ec418024
Part 7: Refactor box model logic into box-model.js. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/02b55f13dabd
Part 8: Use the react/redux box model in the computed view. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc27009b861
Part 9: Use existing box model tests with the new react/redux box model. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/87332bfc1a31
Part 10: Fix race condition in boxmodel tests. r=gl

Comment 87

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f199785e14a4
https://hg.mozilla.org/mozilla-central/rev/c856a31d1f71
https://hg.mozilla.org/mozilla-central/rev/e1781f03570d
https://hg.mozilla.org/mozilla-central/rev/6ed4ff543cd8
https://hg.mozilla.org/mozilla-central/rev/b8c5cc596e19
https://hg.mozilla.org/mozilla-central/rev/e096ec418024
https://hg.mozilla.org/mozilla-central/rev/02b55f13dabd
https://hg.mozilla.org/mozilla-central/rev/9dc27009b861
https://hg.mozilla.org/mozilla-central/rev/87332bfc1a31

Comment 88

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

This issue does not require manual testing anymore. However after a short regression testing, nothing seems to be affected.

Cheers!