1302496 - Refactor LayoutView to a Box Model component

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Comment 1

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

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

Comment 2

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

Attachment: 1302496.patch [1.0]

Comment 3

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

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

Comment 4

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

Julian, this move to a box model component is to make room for making a new Layout view for the css grid and other layout tools. I could see possible arguments for "box-model-" or "boxmodel-". I don't really have preferences, but it can be easily changed.

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8791038 [details] [diff] [review]
1302496.patch [1.0]

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

My only concern is about using the components folder here. As we start using react in the inspector, I think it would be better to reserve the components folder for components actually using react? So maybe move it to computed/ or a new boxmodel/ folder? Not blocking the review on this though, so up to you.

R+ with a green try (two trivial tests to fix browser_computed_search-filter_clear.js & browser_computed_search-filter.js)

As a bonus, if you want to cleanup some more references to "layout-view" or "layout view", you can look in:
- devtools/server/actors/highlighters.js
- devtools/server/actors/inspector.js
- devtools/server/actors/styles.js
- devtools/client/inspector/inspector-panel.js
- devtools/client/inspector/components/test/head.js

Thanks for doing this by the way, was never sure how to refer to this panel before!

::: devtools/client/inspector/components/box-model.js
@@ +603,5 @@
>    update: function () {
>      let lastRequest = Task.spawn((function* () {
>        if (!this.isViewVisibleAndNodeValid()) {
>          this.wrapper.hidden = true;
> +        this.inspector.emit("boxmodel-updated");

Was there a good reason to use boxmodel-updated rather than boxmodelview-updated (which seems like the natural translation from layoutview-updated)?

::: devtools/client/locales/en-US/layoutview.dtd
@@ +13,5 @@
>    - documentation on web development on the web. -->
>  <!-- LOCALIZATION NOTE (*.tooltip): These tooltips are not regular tooltips.
>    -  The text appears on the bottom right corner of the layout view when
>    -  the corresponding box is hovered. -->
> +<!ENTITY boxModelTitle            "Box Model">

I would skip the string update here. I will migrate all the strings in layoutview.dtd in Bug 1294186, I will make sure to switch to a "BoxModel" compliant name there.

But in the meantime, no need to create additional work for localization teams.

Comment 6

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

Attachment: 1302496.patch [2.0]

Comment 7

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

https://hg.mozilla.org/integration/fx-team/rev/639635348b41780875198e5f702d5714078deeee
Bug 1302496 - Refactor LayoutView to a Box Model component r=jdescottes

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/639635348b41