Closed Bug 1336198 Opened 7 years ago Closed 7 years ago

Use the react/redux box model in the computed view

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 19 obsolete files)

9.73 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
2.05 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
Details
26.43 KB, patch
Details | Diff | Splinter Review
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
Blocks: 1150496
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.
Attachment #8835731 - Flags: review?(jdescottes)
Attachment #8835751 - Flags: review?(jdescottes)
Attachment #8835752 - Flags: review?(jdescottes)
Attachment #8835757 - Flags: review?(jdescottes)
Attachment #8835758 - Flags: review?(jdescottes)
Attachment #8835767 - Flags: review?(jdescottes)
Attachment #8835870 - Flags: review?(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?
Flags: needinfo?(gl)
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.
Attachment #8835731 - Flags: review?(jdescottes) → review+
Attachment #8835751 - Flags: review?(jdescottes) → review+
Attachment #8835752 - Flags: review?(jdescottes) → review+
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.
Attachment #8835757 - Flags: review?(jdescottes) → review+
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.
Attachment #8835758 - Flags: review?(jdescottes) → review+
Attachment #8835767 - Flags: review?(jdescottes) → review+
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 :))
Attachment #8835870 - Flags: review?(jdescottes)
Flags: needinfo?(gl)
Priority: -- → P3
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fee5872f8af4
Part 1: Redux store should be initialized at the Inspector level. r=jdescottes
Keywords: leave-open
Attachment #8835731 - Flags: checkin+
Attachment #8837020 - Flags: review?(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?
Attachment #8837020 - Flags: review?(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.
Flags: qe-verify+
Attachment #8835870 - Attachment is obsolete: true
Attachment #8837724 - Flags: review?(jdescottes)
Attachment #8837020 - Attachment is obsolete: true
Attachment #8837749 - Flags: review?(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?
Attachment #8837724 - Flags: review?(jdescottes) → review+
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
Attachment #8837749 - Flags: review?(jdescottes) → review+
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.
Attachment #8837769 - Flags: review?(jdescottes) → review+
Attachment #8837724 - Attachment is obsolete: true
Attachment #8838171 - Flags: review+
Rebased
Attachment #8837749 - Attachment is obsolete: true
Attachment #8838185 - Flags: review+
(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.
Attachment #8838185 - Attachment is obsolete: true
Attachment #8838193 - Flags: review+
Pushed by gabriel.luong@gmail.com:
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
(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.
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
Flags: needinfo?(gl)
Flags: needinfo?(gl)
Pushed by gabriel.luong@gmail.com:
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
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
Flags: needinfo?(gl)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbf5e2d59850
Backed out 8 changesets for devtool failures in browser_boxmodel.js
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.
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)
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.
Flags: needinfo?(gl)
Attachment #8835751 - Attachment is obsolete: true
Attachment #8835752 - Attachment is obsolete: true
Attachment #8835757 - Attachment is obsolete: true
Attachment #8835758 - Attachment is obsolete: true
Attachment #8838193 - Attachment is obsolete: true
Attachment #8835767 - Attachment is obsolete: true
Attachment #8838171 - Attachment is obsolete: true
Attachment #8839029 - Attachment is obsolete: true
Comment on attachment 8840015 [details]
Bug 1336198 - Part 2: Remove deprecated boxmodel stylesheet.

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

forwarding r+
Attachment #8840015 - Flags: review?(jdescottes) → review+
Comment on attachment 8840016 [details]
Bug 1336198 - Part 3: Remove the deprecated box model source file.

https://reviewboard.mozilla.org/r/114574/#review116014
Attachment #8840016 - Flags: review?(jdescottes) → review+
Comment on attachment 8840017 [details]
Bug 1336198 - Part 4: Remove the deprecated box model markup.

https://reviewboard.mozilla.org/r/114576/#review116016
Attachment #8840017 - Flags: review?(jdescottes) → review+
Comment on attachment 8840018 [details]
Bug 1336198 - Part 5: Remove initialization of the deprecated box model.

https://reviewboard.mozilla.org/r/114578/#review116018
Attachment #8840018 - Flags: review?(jdescottes) → review+
Comment on attachment 8840019 [details]
Bug 1336198 - Part 6: Refactor box model component into boxmodel/.

https://reviewboard.mozilla.org/r/114580/#review116020
Attachment #8840019 - Flags: review?(jdescottes) → review+
Comment on attachment 8840020 [details]
Bug 1336198 - Part 7: Refactor box model logic into box-model.js.

https://reviewboard.mozilla.org/r/114582/#review116022
Attachment #8840020 - Flags: review?(jdescottes) → review+
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
Attachment #8840021 - Flags: review?(jdescottes) → review+
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
Attachment #8840022 - Flags: review?(jdescottes) → review+
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
Attachment #8840075 - Flags: review?(gl) → review+
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
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
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.
Pushed by jdescottes@mozilla.com:
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
Attachment #8840746 - Attachment is obsolete: true
Attachment #8840747 - Flags: review+
Pushed by gabriel.luong@gmail.com:
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
Depends on: 1342941
Depends on: 1343167
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Depends on: 1348169
No longer depends on: 1348169
No longer depends on: 1343167
This issue does not require manual testing anymore. However after a short regression testing, nothing seems to be affected.

Cheers!
Flags: qe-verify+ → qe-verify-
Product: Firefox → DevTools
Component: Inspector: Computed → Inspector
You need to log in before you can comment on or make changes to this bug.