1494162 - Improve the Inspector load speed

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Comment 1

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

Attachment: Part 1: Lazy load the HTMLEditor in the markup view. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=e61a896c934ac0fc60b69e7cc555494c2a57fb9f&newProject=try&newRevision=2b5062c57abfc61af012bfc1f07cd3a307b54067&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=12

Comment 2

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

Attachment: Part 2: Lazy load the UndoStack in the markup view. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=e61a896c934ac0fc60b69e7cc555494c2a57fb9f&newProject=try&newRevision=573f853ce2ee16b0986aa9333614acdfefcadad5&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=12

Comment 3

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

DAMP For Part 1+2 https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=c86728971ef1e41a3ad06f2b131a0b5f77836b9d&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 4

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

Attachment: Part 3: Lazy load the SlottedNodeContainer in the markup view.

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=55ff48c2fb15f821797e68d048a99dd966e30dd9&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 5

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

Attachment: Part 4: Lazy load the modules in the TextEditor. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=93a87a65e38613f3bcf411c6f2bca9413ed23de1&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Part 3 and 4 DAMPs are pretty WOW.

Comment 6

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

Attachment: Part 5: Lazy load modules in ElementEditor. [1.0]

Comment 7

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

(In reply to Gabriel [:gl] (ΦωΦ) from comment #6)
> Created attachment 9012017 [details] [diff] [review]
> Part 5: Lazy load modules in ElementEditor. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=918ba89d0ae785bd4e09068123abb13635de5171&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 8

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

Attachment: Part 6: Lazy load the modules in the ElementContainer. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=e0c03a4afab0785bca2fcb68ed2732f6bee6e900&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 9

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

Attachment: Part 7: Lazy load modules in the shared inspector utils. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=f35d37eb19e6ec3cd54900a552c1935eae9fbc16&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 10

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

Attachment: Part 8: Lazy load the AutocompletePopup in the markup view. [1.0]

Comment 11

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

(In reply to Gabriel [:gl] (ΦωΦ) from comment #10)
> Created attachment 9012030 [details] [diff] [review]
> Part 8: Lazy load the AutocompletePopup in the markup view. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=abc4d8e869f8d59407a2016ff586401ebce3feab&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 12

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

Attachment: Part 9: Lazy load the modules in the InspectorSearch. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=473fa646856b8548395802194513133bebb8069f&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 13

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

Attachment: Part 10: Lazy initialize the event details tooltip. [1.0]

Comment 14

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

(In reply to Gabriel [:gl] (ΦωΦ) from comment #13)
> Created attachment 9012033 [details] [diff] [review]
> Part 10: Lazy initialize the event details tooltip. [1.0]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=a03197600016020317e4cec38627326295fe80e3&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 15

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

Attachment: Part 11: Lazy load promiseWarn in the rules model. [1.0]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=e2746b73d98a2f42a0679a4be13ee6ece9bd534c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 16

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

Attachment: Part 9: Lazy load the modules in the InspectorSearch. [2.0]

Comment 17

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

Attachment: Part 12: Lazy load modules in RuleEditor. [1.0]

Comment 18

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

(In reply to Gabriel [:gl] (ΦωΦ) from comment #17)
> Created attachment 9012040 [details] [diff] [review]
> Part 12: Lazy load modules in RuleEditor. [1.0]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=c070dba3778170dd326ae00dfeb810da2d4b3e58&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 19

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

Attachment: Part 13: Lazy load modules in the TextPropertyEditor. [1.0]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1370d67930c901e24ac1effc8cfb2c988abb0412&newProject=try&newRevision=64c9d44142565db9254e31caad1b48253acaf300&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 9012009 [details] [diff] [review]
Part 1: Lazy load the HTMLEditor in the markup view. [1.0]

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

I don't feel for or against this change. It seems like it shouldn't have any effect performance-wise. Before the change, we used a normal require, but only when needed. And after the change, we use a lazyRequire.
Is this for consistency reasons?

::: devtools/client/inspector/markup/views/html-editor.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const Services = require("Services");

This change seems unrelated and unneeded. I would advise against changing this because it tends to polute the HG history for not much reason.

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 9012013 [details] [diff] [review]
Part 3: Lazy load the SlottedNodeContainer in the markup view.

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

::: devtools/client/inspector/markup/views/slotted-node-editor.js
@@ +5,5 @@
>  "use strict";
>  
>  const {LocalizationHelper} = require("devtools/shared/l10n");
>  const INSPECTOR_L10N =
> +  new LocalizationHelper("devtools/client/locales/inspector.properties");

This change is unrelated and will pollute the HG history unnecessarily. I advise reverting this change.
If there are many of these little formatting changes you want to make, better make them all in one initial commit, with a commit message that explains what was changed.

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 9012015 [details] [diff] [review]
Part 4: Lazy load the modules in the TextEditor. [1.0]

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

::: devtools/client/inspector/markup/views/text-editor.js
@@ +12,3 @@
>  
>  const INSPECTOR_L10N =
> +  new LocalizationHelper("devtools/client/locales/inspector.properties");

Same comment as before about unrelated formatting changes.

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 9012029 [details] [diff] [review]
Part 7: Lazy load modules in the shared inspector utils. [1.0]

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

::: devtools/client/inspector/shared/utils.js
@@ +9,1 @@
>  const promise = require("promise");

I guess this one could also be lazily required, right?

Comment 24

[Patrick Brosset <:pbro>]

Comment on attachment 9012033 [details] [diff] [review]
Part 10: Lazy initialize the event details tooltip. [1.0]

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

::: devtools/client/inspector/markup/markup.js
@@ +190,1 @@
>      this.imagePreviewTooltip = new HTMLTooltip(this.toolbox.doc, {

Why not also use this commit to lazy load the image preview tooltip?
It seems arbitrary to me that we would lazy load one tooltip type, but not the other one.
Unless there's a technical limitation that prevents us from doing it on the image tooltip.

Comment 25

[Patrick Brosset <:pbro>]

Here's a DAMP compare URL for the whole patch series: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=e61a896c934ac0fc60b69e7cc555494c2a57fb9f&newProject=try&newRevision=64c9d44142565db9254e31caad1b48253acaf300&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

So inspector open and reload on simple documents improved a lot.
These tests are good at showing improvements (or regressions) in the tool UI itself, so those are really good, and trusted, improvements.
Now, the other open and reload tests did not improve. This is most probably because they use more complex documents that require much more processing (especially server side), and the time needed for that dwarfs the improvements you've made.

Comment 26

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

Attachment: Part 14: Lazy load parseNamedDeclarations in Rule. [1.0]

Comment 27

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

Attachment: Part 15: Use the cached css properties in the rules view. [1.0]

Comment 28

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

Attachment: Part 15: Use the cached css properties in the rules view. [2.0]

Comment 29

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

Attachment: Part 10: Lazy initialize the event details and image tooltip in the markup view. [2.0]

We had to initialize the image tooltip differently because it needs to call _enableImagePreviewTooltip() which calls startTogglingOnHover() which allows the image tooltip to toggle on hover.

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 9012154 [details] [diff] [review]
Part 10: Lazy initialize the event details and image tooltip in the markup view. [2.0]

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

::: devtools/client/inspector/markup/markup.js
@@ +122,5 @@
> +  if (flags.testing) {
> +    // In tests, we start listening immediately to avoid having to simulate a mousemove.
> +    this._initTooltips();
> +  } else {
> +    this._elt.addEventListener("mousemose", () => {

typo: mousemose is not a valid event name :)

Comment 31

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

Attachment: Part 16: Lazy load the TimeScale module in the animation reducer. [1.0]

Comment 32

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

Attachment: Part 17: Lazy load the redux middlewares in create-store. [1.0]

Comment 33

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

Attachment: Part 18: Lazy load modules in the css-logic. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=082ef76c515e5b7317c5b427306c4aa812c085d5&newProject=try&newRevision=03f5df106ff9954178be7966baf5303712287cfb&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

From perfhtml, we shave around 1.7ms.

Comment 34

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

Attachment: Part 19: Directly import getCssPath, getXpath and findCssSelector. [1.0]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=082ef76c515e5b7317c5b427306c4aa812c085d5&newProject=try&newRevision=139b3d61ffe3ed56bdddec10ca8bde4f2dec2a5e&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

We shave around 2.2ms for both the client and server

Comment 35

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

Attachment: Part 20: Lazy load font-utils in the fonts reducer. [1.0]

Very minor 0.1ms savings

Comment 36

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

Attachment: Part 21: Lazy initialize KeyShortcuts on focus of the HTMLBreadcrumbs. [1.0]

Comment 37

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

Attachment: Part 22: Remove onUnderflow call in ArrowScrollBox init. [1.0]

This removes around 2.2ms. See profile screenshot to be attached.

Comment 38

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

Attachment: Profile of Part 22

Comment 39

[Patrick Brosset <:pbro>]

I was curious to see how this patch series would perform on my low-ish end MacBook, so I ran a few tests.

1. on about:blank
  a. without the patches
     cold open of the inspector: 400ms
     re-open of the inspector: 290ms
  b. with the patches
     cold open: 380ms --> 5% win
     re-open: 280ms   --> 3.4% win

These tests show some interesting perf wins. On this simple page, there are almost no DOM nodes and no stylesheets, so the server-side processing is down to a minimum. So these tests do show UI improvements more easily.
I would have expected a much bigger difference, but it's a start.
As discussed yesterday, the URL in comment 25 is most probably wrong, and the perf improvements highlighted there are not the ones these patches will lead to.

2. on a complex web page (www.lemonde.fr)
  a. without the patches
     cold open: 780ms
     re-open: 440ms
  b. with the patches
     cold open: 850ms --> -9% regression
     re-open: 430ms   --> 2.3% win

The cold open test had quite a lot of variance on this complex web page, so I don't know how much we can trust it, but it seems to show that there's either a very small win, or actually an important regression on cold open.

I think one of these 2 things could explain it:
- the server-side processing on this page is quite important when the inspector starts, and so it takes most of the time when the panel opens up, and this dwarfs the wins made by lazy loading stuff in the UI
- or (or maybe and) the code paths that now lazy require dependencies are actually ran when the inspector starts. As we discussed yesterday, that ends up actually being slower than just non-lazily requiring those dependencies.

Comment 40

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

Attachment: Part 23: Lazy load the redux middleware modules. [1.0]

This shaves off around 1.4ms for loading some of these middlewares that also have a lot of requires attached to them.

Comment 41

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

Attachment: Part 24: Lazily get the preview tooltip to avoid loading HTMLTooltip. [1.0]

This shaves off around 2.3ms from the TooltipsOverlay.addToView method

Comment 42

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

Attachment: Part 25: Lazy load the modules in HTMLTooltip. [1.0]

Comment 43

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

Attachment: Part 25: Lazy load the modules in HTMLTooltip [2.0]

I am seeing 2.5ms savings on the initial load of HTMLTooltip from the MenuButton.js in perfhtml

Comment 44

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

Attachment: Part 26: Lazy load accessibility utils. [1.0]

This saves us 0.5ms in the server.

Comment 45

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

Attachment: Part 27: Lazy initialize the tooltip in AutocompletePopup. [1.0]

The point of this was to lazy load HTMLTooltip. I saw a combination of 1.1ms saved between markup and rules.

Comment 46

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

Attachment: Part 28: Lazy initialize TooltipToggle in HTMLTooltip. [1.0]

Comment 47

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

Attachment: Part 28: Lazy initialize TooltipToggle in HTMLTooltip. [2.0]

Saw 1.1ms in savings

Comment 48

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

Attachment: Part 29: Avoid calling toolsidebar show with an id when the default tab is already selected. [1.0]

When we add the tabs, we are already specifying the defaultTab that should be selected. We should avoid passing an id into toolsidebar.show() so we can avoid an unnecessary setState with the selected tab id, and just show the tabbar. 

I am seeing a 6-15ms shaving here.

Comment 49

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

Attachment: Part 27: Lazy initialize the tooltip in AutocompletePopup. [2.0]

Comment 50

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

Attachment: Part 30: Lazily get the highlighters-overlay only when needed. [1.0]

Comment 51

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

Attachment: Part 31: Lazy load the Reps in the layout panel. [1.0]

Comment 52

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

Attachment: Part 31: Lazy load the Reps in the layout panel.

Comment 53

[Patrick Brosset <:pbro>]

Comment on attachment 9012319 [details] [diff] [review]
Part 16: Lazy load the TimeScale module in the animation reducer. [1.0]

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

Are you sure the TimeScale module isn't needed right from the start?
If the animation panel is loaded, then I *think* it executes a UPDATE_ANIMATIONS action, which will need this module. So lazy-loading it at that point might be slower.
Or maybe the reducer is always loaded when the inspector starts? If so, then that change makes sense.

Comment 54

[Patrick Brosset <:pbro>]

Comment on attachment 9012455 [details] [diff] [review]
Part 20: Lazy load font-utils in the fonts reducer. [1.0]

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

R+ even though I have a comment below. Because no matter what the answer is, I won't need to re-review this change.

::: devtools/client/inspector/fonts/reducers/font-editor.js
@@ +15,5 @@
>  } = require("../actions/index");
>  
> +const { getStr } = require("../utils/l10n");
> +
> +loader.lazyRequireGetter(this, "parseFontVariationAxes", "devtools/client/inspector/fonts/utils/font-utils", true);

Same comment as in one of my previous reviews: parseFontVariationAxes is used in the UPDATE_EDITOR_STATE action, which sounds like an action we probably execute always, right from the start. So is it actually useful to lazy load this function? Or is it going to end up making it slower?

Comment 55

[Patrick Brosset <:pbro>]

Comment on attachment 9012594 [details] [diff] [review]
Part 23: Lazy load the redux middleware modules. [1.0]

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

I'm not familiar at all with this code.
Are you 100% confident that these dependencies aren't needed from the start? (not just of the inspector btw, but of any tool that uses react/redux/middlewares).
I just want to make sure that this doesn't end up being slower.
Moving this Honza for the final R+ on this as I just don't know enough here.

Comment 56

[Patrick Brosset <:pbro>]

Comment on attachment 9012603 [details] [diff] [review]
Part 24: Lazily get the preview tooltip to avoid loading HTMLTooltip. [1.0]

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

::: devtools/client/inspector/shared/tooltips-overlay.js
@@ +87,5 @@
>      this._isStarted = true;
>  
>      // For now, preview tooltip has to be instantiated on startup in order to
>      // call tooltip.startTogglingOnHover. Ideally startTogglingOnHover wouldn't be part
>      // of HTMLTooltip and offer a way to lazy load this tooltip.

Now, with your change, this comment is out of date. What changed that we can now avoid instantiating the tooltip on startup? The comment said that we needed to do this because of startTogglingOnHover. So, did something changed that we don't need to do this anymore?
If so, please adjust the comment.

Comment 57

[Patrick Brosset <:pbro>]

Comment on attachment 9012784 [details] [diff] [review]
Part 30: Lazily get the highlighters-overlay only when needed. [1.0]

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

::: devtools/client/inspector/inspector.js
@@ +202,5 @@
>      return this.toolbox.highlighter;
>    },
>  
>    get highlighters() {
> +    console.trace();

Please remove this left-over debugging trace.

::: devtools/client/inspector/markup/views/element-editor.js
@@ +341,5 @@
>      this._displayBadge.textContent = displayType;
>      this._displayBadge.dataset.display = displayType;
>      this._displayBadge.title = DISPLAY_TYPES[displayType];
>      this._displayBadge.classList.toggle("active",
> +      !!this._highlighters &&

I find it a bit confusing that we access both the _highlighters and highlighters properties around the same lines.
It makes it harder for someone reading this for the first time to know which one does what, and when they should use one vs. the other.

I like that we have a nice getter to access this service, and that it hides away the complexity of getting it lazily if needed.
I don't like that we then expose this complexity to the rest of the file.

Could we avoid accessing _highlighters at all? Maybe by adding another getter that just returns _highlighters, whether its been initialized or not.
Of perhaps by just adding something like:

get isHighlightersReady() {
  return !!this._highlighters;
}

@@ +721,5 @@
>      this.markup.inspector.once("markupmutation", onMutations);
>    },
>  
>    startTrackingFlexboxHighlighterEvents() {
> +    if (!this._highlighters) {

Same comment for all these start/stop functions. You could replace those ifs with:
if (!this.isHighlightersReady) {
  return;
}

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +537,5 @@
>  
>      const gridToggle = this.valueSpan.querySelector(".ruleview-grid");
>      if (gridToggle) {
>        gridToggle.setAttribute("title", l10n("rule.gridToggle.tooltip"));
> +      if (this._ruleView._highlighters &&

everywhere else you wrote this.ruleView but wrote this._ruleView here. Probably a typo.

Other than that, same comment as in the previous file. It might be good for the rule-view to expose a getter like:

get isHighlightersReady() {}

or something like this.

Comment 58

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

DAMP of Part 31 only https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b5074c5cfe57cee36155eb47940f7c652c1bcefd&newProject=try&newRevision=4e3461cf49d94f8017668cfa25b071420f58f76c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 59

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

DAMP of Part 29 only https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=84e329d01191f92d1dbcc5e0ad98f4662876a7a8&newProject=try&newRevision=c6653baa9cf4bb7ecdf206a992b14ed7a2aa0b5f&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c13ab5db4b
Part 26: Lazy load accessibility utils. r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a9c00cdfef
Part 31: Lazy load the Reps in the layout panel. r=Honza

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df9e8ceddde
Part 22: Remove onUnderflow call in ArrowScrollBox init. r=pbro

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd66696c9f2
Part 2: Lazy load the UndoStack in the markup view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f6a1098cf9e
Part 3: Lazy load the SlottedNodeContainer in the markup view. r=pbro

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef609de4cd5c
Part 4: Lazy load the modules in the TextEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f077ffe76ca5
Part 5: Lazy load modules in ElementEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e187182a0f
Part 6: Lazy load the modules in the ElementContainer. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/4880d9571aa7
Part 7: Lazy load modules in the shared inspector utils. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2b02dc4b47
Part 10: Lazy initialize the event details and image tooltip in the markup view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f700e44ea63f
Part 11: Lazy load promiseWarn in the rules model. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/10143ae649e5
Part 12: Lazy load modules in RuleEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee129a8c5e00
Part 13: Lazy load modules in the TextPropertyEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f90da1a0201b
Part 14: Lazy load parseNamedDeclarations in Rule. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/c90d929f87f2
Part 15: Use the cached css properties in the rules view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1dac4bffc06
Part 16: Lazy load the TimeScale module in the animation reducer. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbbf97a5802e
Part 17: Lazy load the redux middlewares in create-store. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c531e6f8d4
Part 19: Directly import getCssPath, getXpath and findCssSelector. r=pbro

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea5d67d3c2ae
Part 15 Followup: Fix eslint error. r=me CLOSED TREE

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d5a593adfda
Part 15 Followup followup: Lazy require isCssVariable. r=me CLOSED TREE

Comment 66

[Noemi Erli[:noemi_erli]]

Backed out 12 changesets (Bug 1493563) for failures in test_css-logic-getCssPath.html CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception&classifiedState=unclassified&selectedJob=202268123&revision=d2e83655082f82409ef48e97d9fac1cb4e76a606

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=202268123&repo=mozilla-inbound&lineNumber=142725

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1681bd622c63ad7cf045cb70d487e0b538564d3f

Comment 67

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14272a8b7f32
Backed out 16 changesets (bug 1494162, bug 1464192) for failures in test_css-logic-getCssPath.html CLOSED TREE

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a73764c4497
Part 4: Lazy load the modules in the TextEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/94c2d8174021
Part 5: Lazy load modules in ElementEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5dc92d8a56c
Part 6: Lazy load the modules in the ElementContainer. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/757dc50f41b2
Part 7: Lazy load modules in the shared inspector utils. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe637f705fd2
Part 10: Lazy initialize the event details and image tooltip in the markup view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5b33662379
Part 11: Lazy load promiseWarn in the rules model. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/29de4ed0f9e4
Part 12: Lazy load modules in RuleEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbf24eaaa6ec
Part 13: Lazy load modules in the TextPropertyEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/023390ca8ad2
Part 14: Lazy load parseNamedDeclarations in Rule. r=pbro

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61dd67d8c191
Part 15: Use the cached css properties in the rules view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/81c88e41f78b
Part 16: Lazy load the TimeScale module in the animation reducer. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/63e5313bfc6d
Part 17: Lazy load the redux middlewares in create-store. r=pbro

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd32a2fbc66
Part 19: Directly import getCssPath, getXpath and findCssSelector. r=pbro

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6297b63aa220
Part 19 Follow up: Fix test failures in test_css-logic-getCssPath.html. r=me

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a69fe45e0de
Part 19: Backed out changeset 5fd32a2fbc66. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/409be898a58d
Part 19: Backed out changeset 6297b63aa220. r=me

Comment 73

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

Another Part 29 DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=5eae4773db0461343f4efe88a65691f81218815a&newProject=try&newRevision=fd56575f14aa8aacbd6d9fd4e552ed6d006b3379&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0104010671
Part 29: Avoid calling toolsidebar show with an id when the default tab is already selected. r=Honza

Comment 75

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/7a73764c4497
https://hg.mozilla.org/mozilla-central/rev/94c2d8174021
https://hg.mozilla.org/mozilla-central/rev/f5dc92d8a56c
https://hg.mozilla.org/mozilla-central/rev/757dc50f41b2
https://hg.mozilla.org/mozilla-central/rev/fe637f705fd2
https://hg.mozilla.org/mozilla-central/rev/9c5b33662379
https://hg.mozilla.org/mozilla-central/rev/29de4ed0f9e4
https://hg.mozilla.org/mozilla-central/rev/dbf24eaaa6ec
https://hg.mozilla.org/mozilla-central/rev/023390ca8ad2
https://hg.mozilla.org/mozilla-central/rev/61dd67d8c191
https://hg.mozilla.org/mozilla-central/rev/81c88e41f78b
https://hg.mozilla.org/mozilla-central/rev/63e5313bfc6d
https://hg.mozilla.org/mozilla-central/rev/5fd32a2fbc66
https://hg.mozilla.org/mozilla-central/rev/6297b63aa220
https://hg.mozilla.org/mozilla-central/rev/4a69fe45e0de
https://hg.mozilla.org/mozilla-central/rev/409be898a58d
https://hg.mozilla.org/mozilla-central/rev/3f0104010671

Comment 76

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

Attachment: Part 32: Lazy load Grid components. [1.0]

Comment 77

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

Attachment: Part 35: Lazy load MenuButton, MenuItem, and MenuList components in ToolboxTabs. [1.0]

Comment 78

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

Attachment: Part 37: Speed up getTypes in node-attribute-parser.js by using keys instead of looping over an array. [1.0]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1a30eb11637e43d054a1f3e6d5120ddcda7fbdee&newProject=try&newRevision=2514945ae8ef120ba2cc63d9a2756a3b3e122c59&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 79

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

Attachment: Part 38: Use the cached css properties in the inspector. [1.0]

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3fd96af0ca2
Part 19: Directly import getCssPath, getXpath and findCssSelector. r=pbro

Comment 81

[Natalia Csoregi [:nataliaCs]]

Backed out for several clipboard and devtools failures on devtools/client e.g devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/be070bf5d08a5fa7efb83751b7b27d5f1269e5da

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception,usercancel,runnable&revision=d3fd96af0ca264b2637b88107420070ea9400700&group_state=expanded

e.g failure log clipboard: https://treeherder.mozilla.org/logviewer.html#?job_id=202409847&repo=mozilla-inbound&lineNumber=2905

[task 2018-09-29T20:32:13.038Z] 20:32:13     INFO - Opening the inspector
[task 2018-09-29T20:32:13.038Z] 20:32:13     INFO - Opening the toolbox
[task 2018-09-29T20:32:13.584Z] 20:32:13     INFO - GECKO(4063) | console.error: "Error while calling actor 'domnode's method 'getUniqueSelector'" "findCssSelector is not a function"
[task 2018-09-29T20:32:13.585Z] 20:32:13     INFO - GECKO(4063) | console.error: "getUniqueSelector@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/node.js:565:12\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1198:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1313:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:892:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:313:12\nonConnect</<@resource://devtools/server/startup/frame.js:50:22\nonConnect<@resource://devtools/server/startup/frame.js:49:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5\n@resource://devtools/server/startup/frame.js:19:4\n"
[task 2018-09-29T20:32:13.585Z] 20:32:13     INFO - GECKO(4063) | console.error: "Protocol error (unknownError): findCssSelector is not a function"
[task 2018-09-29T20:32:13.663Z] 20:32:13     INFO - Toolbox opened and focused
[task 2018-09-29T20:32:13.699Z] 20:32:13     INFO - Find and expand the test-component shadow DOM host.
[task 2018-09-29T20:32:13.715Z] 20:32:13     INFO - Expand the shadow root
[task 2018-09-29T20:32:13.731Z] 20:32:13     INFO - Select the div under the shadow root
[task 2018-09-29T20:32:13.732Z] 20:32:13     INFO - Selecting the node for '[Front for domnode/server1.conn2.child1/domnode39]'
[task 2018-09-29T20:32:13.753Z] 20:32:13     INFO - GECKO(4063) | console.error: "Error while calling actor 'domnode's method 'getUniqueSelector'" "findCssSelector is not a function"
[task 2018-09-29T20:32:13.754Z] 20:32:13     INFO - GECKO(4063) | console.error: "getUniqueSelector@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/node.js:565:12\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1198:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1313:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:892:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:313:12\nonConnect</<@resource://devtools/server/startup/frame.js:50:22\nonConnect<@resource://devtools/server/startup/frame.js:49:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5\n@resource://devtools/server/startup/frame.js:19:4\n"
[task 2018-09-29T20:32:13.757Z] 20:32:13     INFO - GECKO(4063) | console.error: "Protocol error (unknownError): findCssSelector is not a function"
[task 2018-09-29T20:32:13.764Z] 20:32:13     INFO - Check the copied values for the various copy*Path helpers
[task 2018-09-29T20:32:13.768Z] 20:32:13     INFO - GECKO(4063) | console.error: "Error while calling actor 'domnode's method 'getXPath'" "getXPath is not a function"
[task 2018-09-29T20:32:13.769Z] 20:32:13     INFO - GECKO(4063) | console.error: "getXPath@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/node.js:589:12\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1198:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1313:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:892:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:313:12\nonConnect</<@resource://devtools/server/startup/frame.js:50:22\nonConnect<@resource://devtools/server/startup/frame.js:49:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5\n@resource://devtools/server/startup/frame.js:19:4\n"
[task 2018-09-29T20:32:13.770Z] 20:32:13     INFO - GECKO(4063) | console.error: "Protocol error (unknownError): getXPath is not a function"
[task 2018-09-29T20:32:18.803Z] 20:32:18     INFO - TEST-INFO | started process screentopng
[task 2018-09-29T20:32:19.102Z] 20:32:19     INFO - TEST-INFO | screentopng: exit 0
[task 2018-09-29T20:32:19.103Z] 20:32:19     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js | Timed out while polling clipboard for pasted data, got: waitForClipboard-known-value-0.47431154726991565 - 
[task 2018-09-29T20:32:19.103Z] 20:32:19     INFO - Stack trace:
[task 2018-09-29T20:32:19.103Z] 20:32:19     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:putAndVerify:1025
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.promiseClipboardChange:1036
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard:969
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochitests/content/browser/devtools/client/shared/test/shared-head.js:waitForClipboardPromise/<:583
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochitests/content/browser/devtools/client/shared/test/shared-head.js:waitForClipboardPromise:582
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochitests/content/browser/devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js:null:50
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
[task 2018-09-29T20:32:19.105Z] 20:32:19     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
[task 2018-09-29T20:32:19.105Z] 20:32:19     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:986
[task 2018-09-29T20:32:19.105Z] 20:32:19     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-09-29T20:32:19.105Z] 20:32:19     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-09-29T20:32:19.106Z] 20:32:19     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js | Uncaught exception - failed
[task 2018-09-29T20:32:19.106Z] 20:32:19     INFO - Leaving test bound 
[task 2018-09-29T20:32:19.106Z] 20:32:19     INFO - Removing tab.
[task 2018-09-29T20:32:19.106Z] 20:32:19     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-09-29T20:32:19.107Z] 20:32:19     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-09-29T20:32:19.107Z] 20:32:19     INFO - Tab removed and finished closing
[task 2018-09-29T20:32:19.107Z] 20:32:19     INFO - GECKO(4063) | MEMORY STAT | vsize 617MB | residentFast 272MB | heapAllocated 92MB
[task 2018-09-29T20:32:19.108Z] 20:32:19     INFO - TEST-OK | devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js | took 6111ms

e.g devtools failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=202409743&repo=mozilla-inbound&lineNumber=6444

task 2018-09-29T20:26:44.439Z] 20:26:44     INFO - TEST-PASS | devtools/client/inspector/markup/test/browser_markup_flex_display_badge.js | No flexbox highlighter is shown. - 
[task 2018-09-29T20:26:44.441Z] 20:26:44     INFO - Toggling ON the flexbox highlighter from the flex display badge.
[task 2018-09-29T20:26:44.443Z] 20:26:44     INFO - Waiting for state predicate "state => state.flexbox.highlighted"
[task 2018-09-29T20:26:44.484Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_FLEXBOX"
[task 2018-09-29T20:26:44.563Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_OFFSET_PARENT"
[task 2018-09-29T20:26:44.579Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_LAYOUT"
[task 2018-09-29T20:26:44.728Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_FLEXBOX"
[task 2018-09-29T20:26:44.729Z] 20:26:44     INFO - GECKO(1429) | console.error: "Error while calling actor 'domnode's method 'getUniqueSelector'" "findCssSelector is not a function"
[task 2018-09-29T20:26:44.730Z] 20:26:44     INFO - GECKO(1429) | console.error: "getUniqueSelector@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/node.js:565:12\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1198:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1313:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:892:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:313:12\nonConnect</<@resource://devtools/server/startup/frame.js:50:22\nonConnect<@resource://devtools/server/startup/frame.js:49:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5\n@resource://devtools/server/startup/frame.js:19:4\n"
[task 2018-09-29T20:26:44.751Z] 20:26:44     INFO - GECKO(1429) | console.error: "Protocol error (unknownError): findCssSelector is not a function"
[task 2018-09-29T20:26:44.752Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_OFFSET_PARENT"
[task 2018-09-29T20:26:44.771Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_LAYOUT"
[task 2018-09-29T20:26:44.805Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_OFFSET_PARENT"
[task 2018-09-29T20:26:44.842Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_LAYOUT"
[task 2018-09-29T20:26:44.979Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_FLEXBOX"
[task 2018-09-29T20:27:28.164Z] 20:27:28     INFO - TEST-INFO | started process screentopng
[task 2018-09-29T20:27:28.707Z] 20:27:28     INFO - TEST-INFO | screentopng: exit 0
[task 2018-09-29T20:27:28.707Z] 20:27:28     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_flex_display_badge.js | Test timed out - 
[task 2018-09-29T20:27:28.707Z] 20:27:28     INFO - Removing tab.
[task 2018-09-29T20:27:28.707Z] 20:27:28     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-09-29T20:27:28.708Z] 20:27:28     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-09-29T20:27:28.709Z] 20:27:28     INFO - Tab removed and finished closing
[task 2018-09-29T20:27:28.711Z] 20:27:28     INFO - GECKO(1429) | MEMORY STAT | vsize 717MB | residentFast 311MB | heapAllocated 101MB
[task 2018-09-29T20:27:28.712Z] 20:27:28     INFO - TEST-OK | devtools/client/inspector/markup/test/browser_markup_flex_display_badge.js | took 45386ms

Comment 82

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

Attachment: Part 24: Lazily get the preview tooltip to avoid loading HTMLTooltip. [2.0]

Comment 83

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

Attachment: Part 27: Lazy initialize the tooltip in AutocompletePopup. [3.0]

Comment 84

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

Attachment: Part 41: Lazy load the font inspector l10n properties. [1.0]

This shaves off around 2.5-3ms by converting the initial state into a function so that we can lazily call getStr which allows us to avoid loading and reading the font-inspector.properties file until it is needed.

Comment 85

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

Attachment: Part 44: Lazy load findDOMNode. [1.0]

Comment 86

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

Comment on attachment 9012594 [details] [diff] [review]
Part 23: Lazy load the redux middleware modules. [1.0]

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

Looks good to me, thanks for the patch Gabriel!

Honza

Comment 87

[Julian Descottes [:jdescottes]]

Comment on attachment 9013094 [details] [diff] [review]
Part 35: Lazy load MenuButton, MenuItem, and MenuList components in ToolboxTabs. [1.0]

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

::: devtools/client/framework/components/ToolboxTabs.js
@@ +12,1 @@
>  const ToolboxTab = createFactory(require("devtools/client/framework/components/ToolboxTab"));

Should we lazy load ToolboxTab as well?

Comment 88

[Patrick Brosset <:pbro>]

Comment on attachment 9013095 [details] [diff] [review]
Part 37: Speed up getTypes in node-attribute-parser.js by using keys instead of looping over an array. [1.0]

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

::: devtools/client/shared/node-attribute-parser.js
@@ +306,2 @@
>      const containsAttribute = attributeName === typeData.attributeName ||
>                              typeData.attributeName === "*";

This line is weird, we don't have any attributeName in the array above that is *. So it looks like it's a feature we (or I, in that case) implemented but never used.
Maybe just remove this part of the condition.

And if you do, then remove the whole containsAttribute condition, because you're already just looping over the items of ATTRIBUTES[attributeName], so you're bound to just have these.

And therefore, also remove the attibuteName property from all of the objects in the list above. Since they already appear as keys of the ATTRIBUTE_TYPES object.

Comment 89

[Patrick Brosset <:pbro>]

Comment on attachment 9013093 [details] [diff] [review]
Part 32: Lazy load Grid components. [1.0]

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

::: devtools/client/inspector/grids/components/Grid.js
@@ +15,5 @@
> +loader.lazyGetter(this, "GridList", function() {
> +  return createFactory(require("./GridList"));
> +});
> +loader.lazyGetter(this, "GridOutline", function() {
> +  return createFactory(require("./GridOutline"));

These 3 components are used in the render function, so that means they're actually needed as soon as the Grid component is needed. Unless there is no grid in the page.
Can you confirm that this is the reason you are lazy loading these components? And if so, please add a comment above the loader.lazyGetter calls to explain this.

Comment 90

[Patrick Brosset <:pbro>]

Comment on attachment 9013207 [details] [diff] [review]
Part 44: Lazy load findDOMNode. [1.0]

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

::: devtools/client/framework/components/ToolboxTabs.js
@@ +20,5 @@
>  loader.lazyGetter(this, "MenuList", function() {
>    return createFactory(require("devtools/client/shared/components/menu/MenuList"));
>  });
>  
> +loader.lazyRequireGetter(this, "findDOMNode", "devtools/client/shared/vendor/react-dom", true);

The 3 other components you changed look good, but this one is the only one that needs findDOMNode in onComponentDidMount. And the toolbox tabs are always visible. So this means that we're lazy requiring a module for nothing since it's actually needed when we start.

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50008b82b8ec
Part 23: Lazy load the redux middleware modules. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c00644cf715
Part 24: Lazily get the preview tooltip to avoid loading HTMLTooltip. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e25b5302003
Part 25: Lazy load the modules in HTMLTooltip. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ba9d86cee4
Part 28: Lazy initialize TooltipToggle in HTMLTooltip. r=jdescottes

Comment 92

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c68f9435308
Part 38: Use the cached css properties in the inspector. r=pbro

Comment 93

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

Attachment: Part 37: Speed up getTypes in node-attribute-parser.js by using keys instead of looping over an array. [2.0]

Comment 94

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67753603319f
Part 35: Lazy load MenuButton, MenuItem, and MenuList components in ToolboxTabs. r=jdescottes

Comment 95

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

Comment on attachment 9013094 [details] [diff] [review]
Part 35: Lazy load MenuButton, MenuItem, and MenuList components in ToolboxTabs. [1.0]

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

::: devtools/client/framework/components/ToolboxTabs.js
@@ +12,1 @@
>  const ToolboxTab = createFactory(require("devtools/client/framework/components/ToolboxTab"));

No, we shouldn't lazy load things that will actually run since lazyGetter has a negative consequences of around 3% if the codepath is actually hit.

Comment 96

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/50008b82b8ec
https://hg.mozilla.org/mozilla-central/rev/8c00644cf715
https://hg.mozilla.org/mozilla-central/rev/0e25b5302003
https://hg.mozilla.org/mozilla-central/rev/d8ba9d86cee4
https://hg.mozilla.org/mozilla-central/rev/2c68f9435308

Comment 97

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

Comment on attachment 9013093 [details] [diff] [review]
Part 32: Lazy load Grid components. [1.0]

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

Yes, we are lazily loading these components because we are assuming that grid is less likely to appear on the page.

Comment 98

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

Attachment: Part 44: Use refs instead of findDOMNode. [1.0]

Comment 99

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

Attachment: Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [1.0]

Comment 100

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/67753603319f

Comment 101

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

Attachment: Part 44: Use refs instead of findDOMNode. [2.0]

Comment 102

[Patrick Brosset <:pbro>]

Comment on attachment 9013512 [details] [diff] [review]
Part 44: Use refs instead of findDOMNode. [2.0]

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

This looks good, I think, given my limited React experience. I'd feel more comfortable if you asked another person to take a quick look though.
Just 2 questions: are refs faster than findDOM? And, why did you not use createRef in the SplitBox component, like in the other ones?

::: devtools/client/shared/components/splitter/SplitBox.js
@@ +248,5 @@
>      return (
> +      dom.div(
> +        {
> +          className: classNames.join(" "),
> +          ref: div => {

I'm not familiar enough with React's ref to understand why you didn't use React.createRef in this component, and instead provided a function here.

Comment 103

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4aea32bbca
Part 37: Speed up getTypes in node-attribute-parser.js by using keys instead of looping over an array. r=pbro

Comment 104

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/116b7931a196
Part 32: Lazy load Grid components. r=pbro

Comment 105

[Julian Descottes [:jdescottes]]

Comment on attachment 9013121 [details] [diff] [review]
Part 27: Lazy initialize the tooltip in AutocompletePopup. [3.0]

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

::: devtools/client/shared/autocomplete-popup.js
@@ +246,3 @@
>  
> +      this._list = null;
> +      this._listPadding = null;

nit: do we really need to nullify this?

Comment 106

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce8b77ec351
Part 44: Use refs instead of findDOMNode. r=pbro

Comment 107

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef38ce41818
Part 27: Lazy initialize the tooltip in AutocompletePopup. r=jdescottes

Comment 108

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/fc4aea32bbca
https://hg.mozilla.org/mozilla-central/rev/116b7931a196
https://hg.mozilla.org/mozilla-central/rev/7ce8b77ec351

Comment 109

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

Attachment: Part 46: Lazy load openDocLink and assert in MeatballMenu. [1.0]

Comment 110

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

Attachment: Part 48: Lazy load getUnicodeUrl in ToolboxToolbar. [1.0]

Comment 111

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

Attachment: Part 45: Lazy load Menu and MenuItem in TabBar. [1.0]

Comment 112

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

Attachment: Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [2.0]

Comment 113

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

Attachment: Part 50: Avoid destroying the markup iframe on navigation. [1.0]

Comment 114

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9ef38ce41818

Comment 115

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 9013898 [details] [diff] [review]
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [2.0]

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

::: devtools/client/shared/components/menu/MenuButton.js
@@ +75,5 @@
>  
>      this.state = {
>        expanded: false,
> +      // Whether or not the menu is initialized.
> +      isMenuInitialized: flags.testing || false,

I feel like this comment might be a little redundant ;)

If anything, it might be worth explaining what flags.testing is used for here.

@@ +87,5 @@
>      this.initializeTooltip();
> +
> +    if (!this.state.isMenuInitialized) {
> +      // Initialize the menu when the button is focused or moused over.
> +      for (const event of ["focus", "mousemove"]) {

Out of curiosity, any reason to prefer this over the requestIdleCallback approach we discussed?

The reason I ask is this approach is not going to help touch-screen users. Fortunately I don't think too many devs are triggering DevTools menus with touch-screens at this stage. (I mean I do, but I can live with the menu being less snappy in that case--I'm normally just glad if my fat fingers actually hit the right button.)

I assume we'll still get a focus event for touch screens so this won't completely break them... actually let me start up a build and see.

Comment 116

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/9ef38ce41818

Comment 117

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a30a201d8cd
Part 46: Lazy load openDocLink and assert in MeatballMenu. r=jdescottes

Comment 118

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2a30a201d8cd

Comment 119

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

Attachment: Part 52: Don't show the empty message in the CssRuleView constructor. [1.0]

Comment 120

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 9013898 [details] [diff] [review]
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [2.0]

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

r=me with the comment fix

::: devtools/client/shared/components/menu/MenuButton.js
@@ +87,5 @@
>      this.initializeTooltip();
> +
> +    if (!this.state.isMenuInitialized) {
> +      // Initialize the menu when the button is focused or moused over.
> +      for (const event of ["focus", "mousemove"]) {

I checked this locally and it seems like even if we call preventDefault() on the mousedown/touchstart event (a common technique for prevent buttons from stealing the focus) the menu still opens correctly on a touch device. So I think this is fine.

I'm still curious as to why you didn't go with the requestIdleCallback approach.

Comment 121

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

Attachment: Part 53: Use Promise all to parallelize getting the page style, css properties and default node. [1.0]

Comment 122

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

Attachment: Part 54: Stop loading codemirror.bundle.js in the markup. [1.0]

Comment 123

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

Attachment: Part 55: Remove traits for getCssPath and getXPath. [1.0]

Comment 124

[Patrick Brosset <:pbro>]

Hi Gabriel, I think you should land what remains to be landed and mark this bug as fixed.
The types of patches you're doing now have less to do with lazyloading, and more with perceived performance. 
They are likely to be taking much more time than the previous ones, and could very much use their own dedicated bugs.
Also, we're getting close to the 64 nightly code freeze, and I wouldn't want a bug to span over 2 releases if some of its patches have already landed in the first one.

Comment 125

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a5ee779a0d
Part 48: Lazy load getUnicodeUrl in ToolboxToolbar. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/465b5d63240a
Part 52: Don't show the empty message in the CssRuleView constructor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/a06f75df15b5
Part 55: Remove traits for getCssPath and getXPath. r=rcaliman

Comment 126

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01ac27238f7f
Part 45: Lazy load Menu and MenuItem in TabBar. r=pbro

Comment 127

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9ecb747f9d
Part 54: Stop loading codemirror.bundle.js in the markup. r=jdescottes

Comment 128

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

Comment on attachment 9013898 [details] [diff] [review]
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [2.0]

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

I did consider the requestIdleCallback approach after your initial review. It becomes a harder to see where the win for the requestIdleCallback is for the MenuButton since it still gets executed eventually but ideally it would execute after everything else is loaded (ie the inspector is fully loaded). That is not the case tho. The mousemove/focus initialization shows very clear win on DAMP. It is a bit hard to assess the benefits of requestIdleCallback with our current metrics. So, I am going with the clear winner for now.

Here are the 2 DAMPs for a comparison.

requestIdleCallback https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b7feccb2a19100c217d7fde95d5727defc2934c5&newProject=try&newRevision=d57a6b3356e607c19763c316cb25fed3d0ca8d38&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

mousemove/focus initialization https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b7feccb2a19100c217d7fde95d5727defc2934c5&newProject=try&newRevision=a5171ae968db10dd232ce159d852181b70cb0d06&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 129

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f12b32a5bb3c
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. r=birtles

Comment 130

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

Attachment: Part 47: Remove duplicate requires of addPseudoClassLock and removePseudoClassLock. [1.0]

Comment 131

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d6a5ee779a0d
https://hg.mozilla.org/mozilla-central/rev/465b5d63240a
https://hg.mozilla.org/mozilla-central/rev/a06f75df15b5
https://hg.mozilla.org/mozilla-central/rev/01ac27238f7f
https://hg.mozilla.org/mozilla-central/rev/dd9ecb747f9d
https://hg.mozilla.org/mozilla-central/rev/f12b32a5bb3c

Comment 132

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

Attachment: Part 8: Lazy loadgetAngleValueInDegrees in color.js. [1.0]

Comment 133

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

Attachment: Part 56: Reorder the function calls in deferredOpen to prioritize initializing the markup and sidebar panels. [1.0]

Comment 134

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

Attachment: Part 57: Set the node front before the components are initialized to unblock it from waiting until the markup is loaded before initializing. [1.0]

Comment 135

[Patrick Brosset <:pbro>]

Comment on attachment 9014670 [details] [diff] [review]
Part 56: Reorder the function calls in deferredOpen to prioritize initializing the markup and sidebar panels. [1.0]

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

I'm amazed if try is still green after this! But this change makes a ton of sense anyway, so R+

Comment 136

[Patrick Brosset <:pbro>]

Comment on attachment 9014672 [details] [diff] [review]
Part 57: Set the node front before the components are initialized to unblock it from waiting until the markup is loaded before initializing. [1.0]

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

Looks good overall. I have one comment about simplifying the code to make it more readable.

::: devtools/client/inspector/inspector.js
@@ +293,5 @@
>      this.setupSearchBox();
>      await this.setupToolbar();
>  
> +    // Since we set the nodeFront before all the components are initialized. We perform
> +    // the actions of this.onNewSelection here.

This comment sounds a bit odd. It makes a lot of sense if you know what the code was before this change. But in a while, people will have a hard time understanding it.
In fact, it would be so much more natural to just call this.onNewSelection() right here. Could we try to do that?

Comment 137

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/708061fd15ce
Part 53: Use Promise all to parallelize getting the page style, css properties and default node. r=pbro

Comment 138

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ae43bf692e
Part 56: Reorder the function calls in deferredOpen to prioritize initializing the markup and sidebar panels. r=pbro

Comment 139

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

Attachment: Part 57: Set the node front before the components are initialized to unblock it from waiting until the markup is loaded before initializing. [2.0]

Comment 140

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a095b6e9b7d
Part 8: Lazy loadgetAngleValueInDegrees in color.js. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a52c822ebce
Part 47: Remove duplicate requires of addPseudoClassLock and removePseudoClassLock. r=rcaliman

Comment 141

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/289fbd180e44
Part 57: Set the node front before the components are initialized to unblock it from waiting until the markup is loaded before initializing. r=pbro

Comment 142

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

Attachment: Part 58: Remove the await call to block on markuploaded. [1.0]

Comment 143

[Patrick Brosset <:pbro>]

Comment on attachment 9014883 [details] [diff] [review]
Part 58: Remove the await call to block on markuploaded. [1.0]

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

Hmm, alright, sure, ok, as long as try is still green and the inspector still loads ok.
I'm uneasy about those major changes in the load sequence of the inspector, knowing that it's by nature quite racy, and that it's the panel people go to first the most. But if it helps load perf and still works, then great!

Comment 144

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/708061fd15ce
https://hg.mozilla.org/mozilla-central/rev/c0ae43bf692e
https://hg.mozilla.org/mozilla-central/rev/0a095b6e9b7d
https://hg.mozilla.org/mozilla-central/rev/3a52c822ebce
https://hg.mozilla.org/mozilla-central/rev/289fbd180e44

Comment 145

[Alexandre Poirot [:ochameau]]

Comment on attachment 9012749 [details] [diff] [review]
Part 29: Avoid calling toolsidebar show with an id when the default tab is already selected. [1.0]

This one patch seems to have had a particular impact on performance:

== Change summary for alert #16535 (as of Fri, 05 Oct 2018 22:23:18 GMT) ==

Improvements:

 20%  damp complicated.inspector.open.DAMP windows7-32 opt e10s stylo      493.34 -> 395.02
 19%  damp complicated.inspector.open.DAMP windows7-32 pgo e10s stylo      446.66 -> 363.35
  9%  damp complicated.inspector.close.DAMP windows7-32 opt e10s stylo     58.30 -> 52.84
  4%  damp inspector.layout.open windows7-32 opt e10s stylo                422.07 -> 404.70
  4%  damp simple.inspector.open.DAMP windows7-32 opt e10s stylo           279.88 -> 269.52
  4%  damp cold.inspector.open.DAMP windows7-32 opt e10s stylo             526.28 -> 506.80

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16535

Comment 146

[Alexandre Poirot [:ochameau]]

Comment on attachment 9013898 [details] [diff] [review]
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [2.0]

This particular changeset, or some other landed at the same time introduced such improvements to performance:
== Change summary for alert #16480 (as of Thu, 04 Oct 2018 17:46:51 GMT) ==

Regressions:

  2%  damp console.bulklog windows10-64-qr opt e10s stylo     120.94 -> 123.39

Improvements:

  8%  damp custom.jsdebugger.pause.DAMP windows10-64-qr opt e10s stylo         556.36 -> 510.65
  5%  damp simple.netmonitor.open.DAMP windows7-32 opt e10s stylo              234.09 -> 222.40
  4%  damp complicated.netmonitor.open.DAMP windows10-64-qr opt e10s stylo     295.39 -> 283.67
  3%  damp complicated.netmonitor.open.DAMP windows7-32 pgo e10s stylo         260.28 -> 251.31
  3%  damp simple.styleeditor.open.DAMP linux64-qr opt e10s stylo              262.09 -> 253.59
  3%  damp complicated.netmonitor.open.DAMP linux64-qr opt e10s stylo          315.17 -> 305.12
  3%  damp simple.styleeditor.open.DAMP linux64 pgo e10s stylo                 228.26 -> 221.06
  3%  damp simple.styleeditor.open.DAMP windows7-32 opt e10s stylo             228.25 -> 221.28
  3%  damp complicated.netmonitor.open.DAMP windows10-64 opt e10s stylo        289.07 -> 280.50
  3%  damp complicated.webconsole.open.DAMP windows10-64 opt e10s stylo        345.82 -> 335.87
  3%  damp simple.styleeditor.open.DAMP windows10-64 opt e10s stylo            233.57 -> 227.04
  3%  damp simple.styleeditor.open.DAMP windows10-64-qr opt e10s stylo         230.30 -> 224.06
  3%  damp complicated.netmonitor.open.DAMP windows7-32 opt e10s stylo         287.67 -> 279.87
  3%  damp console.openwithcache.open.DAMP linux64-qr opt e10s stylo           333.80 -> 324.78
  3%  damp complicated.webconsole.open.DAMP windows10-64-qr opt e10s stylo     353.40 -> 343.93
  3%  damp complicated.webconsole.open.DAMP windows7-32 opt e10s stylo         339.23 -> 330.38
  3%  damp console.openwithcache.open.DAMP windows10-64-qr opt e10s stylo      315.89 -> 307.66
  3%  damp complicated.webconsole.open.DAMP windows7-32 pgo e10s stylo         312.05 -> 304.00
  2%  damp simple.styleeditor.open.DAMP windows7-32 pgo e10s stylo             209.38 -> 204.81

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16480