Closed Bug 1320939 Opened 8 years ago Closed 7 years ago

Tooltip modules/setup is reported by the profiler while opening the inspector

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

toolbox-without-gcli.json
1.13 MB, text/plain
Details
Bug 1320939 - Lazy load all tooltip widgets until each is really used.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1320939 - Lazy load tooltip event helpers.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
Once I striped gcli from toolbox startup (bug 1320149), the second biggest code is the tooltip modules. Some are not very useful, like cubic bezier or filter and shouldn't be loaded at all. And even the color picker should be loaded only once we display a rule.

I'm attaching a simple profile done on a simple firefox build (with gcli buttons disabled to remove some noise), where I wait firefox to settle after startup, then just open a toolbox on the inspector while google is opened.

You will immediately see Tooltip modules as the topmost consuming part if you open the tree on the first item: PromiseWalker.scheduleWalkerLoop.
This patch may break tests pretty badly but allows to strip down inspector loading from 7.3 to 4.8seconds!
Clearly, we shouldn't consider profiler durations seriously. The "Total time" column. As this was reported in the profiler as taking only 400ms.
But the order in which things are sorted make a lot of sense.
Actually, I've been mislead in comment 2. I rather get something around 5% with 100 to 200ms difference only. It is really hard to prove the value of any performance tweak as firefox itself has extremelly variable behavior. The exact same build will open the toolbox in 5s and sometime later in 7s.
Summary: Tooltip modules/setup takes about 35% of the toolbox load time on the inspector → Tooltip modules/setup is reported by the profiler while opening the inspector
Using time diff in-code is the only trustable way to probe something. And the real cost seems to be around 120ms. So we are rather at 2 to 3%. Which is close to what I saw in comment 5.
Attachment #8815246 - Attachment is obsolete: true
Comment on attachment 8815306 [details]
Bug 1320939 - Lazy load tooltips-overlay from rule view.

Julian, What do you think? Sorry if my report is fuzzy. I have hard time measuring the real impact of my tweaks. I imagine if we combine all of them we would end up with a faster inspector/toolbox.
Could you play with that patch and see if you feel any difference?
Otherwise does this patch make sense to you?
Attachment #8815306 - Flags: feedback?(jdescottes)
Comment on attachment 8815306 [details]
Bug 1320939 - Lazy load tooltips-overlay from rule view.

https://reviewboard.mozilla.org/r/96286/#review96794

Thanks for the patch! 

I am currently testing it in an OSX build. I profiled the execution time of showToolbox (which should be similar to what DAMP measures). I tried 2 scenarios :
- open the toolbox (on the inspector) for the first time after starting the browser
- open the toolbox repeatedly

Without patch / With patch :
- first open : 1180ms / 1130ms (50ms or 4% improvement)
- repeat open: 480ms / 450ms (30ms or 6% improvement)

(for every scenario these figures are an average of 10runs, excluding runs where I obviously ran into a GC pause)

I can't say from a user perspective I felt a huge difference, but I don't see a better hotspot worth optimizing at the moment. And 5% is still a nice win. Can you get some figures from Talos to see if it goes in the same direction? If yes I think we should carry on with your patch.

The overall changeset makes sense, I just want to make sure the added complexity is worth it.

Some comments in the patch itself, but I might have more later (spent most of my time profiling ... sorry about that).

::: devtools/client/inspector/shared/tooltips-overlay.js
(Diff revision 2)
>    VIEW_NODE_VALUE_TYPE,
>    VIEW_NODE_IMAGE_URL_TYPE,
>  } = require("devtools/client/inspector/shared/node-types");
>  const { getColor } = require("devtools/client/shared/theme");
>  const { getCssProperties } = require("devtools/shared/fronts/css-properties");
> -const CssDocsTooltip = require("devtools/client/shared/widgets/tooltip/CssDocsTooltip");

Do we really need to add lazy loading inside of tooltips-overlay if this file is already lazily loaded. 

If the goal is to allow loading one tooltip (say colorpicker) without loading filter, bezier and css widget, I think we could leave this for a second step?

::: devtools/client/inspector/shared/tooltips-overlay.js:91
(Diff revision 2)
> -      // Color picker tooltip
> -      this.colorPicker = new SwatchColorPickerTooltip(toolbox.doc, this.view.inspector);
> -      // Cubic bezier tooltip
> -      this.cubicBezier = new SwatchCubicBezierTooltip(toolbox.doc);
> -      // Filter editor tooltip
> -      this.filterEditor = new SwatchFilterTooltip(toolbox.doc,
> +   * Lazily fetch a tooltip instance
> +   *
> +   * @param {String} name
> +   *        Identifier name for the tooltip
> +   */
> +  get: function (name) {

let's use lazy getters for each tooltip to avoid impacts on the consumer code (same as what you did for get tooltips() / _tooltips)
Attachment #8815306 - Flags: feedback?(jdescottes) → feedback+
(In reply to Julian Descottes [:jdescottes] from comment #11)
> Some comments in the patch itself, but I might have more later (spent most
> of my time profiling ... sorry about that).

Actually, thanks a lot for testing it, that conforms me in my usage of the profiler!

> 
> ::: devtools/client/inspector/shared/tooltips-overlay.js
> (Diff revision 2)
> >    VIEW_NODE_VALUE_TYPE,
> >    VIEW_NODE_IMAGE_URL_TYPE,
> >  } = require("devtools/client/inspector/shared/node-types");
> >  const { getColor } = require("devtools/client/shared/theme");
> >  const { getCssProperties } = require("devtools/shared/fronts/css-properties");
> > -const CssDocsTooltip = require("devtools/client/shared/widgets/tooltip/CssDocsTooltip");
> 
> Do we really need to add lazy loading inside of tooltips-overlay if this
> file is already lazily loaded. 
> 
> If the goal is to allow loading one tooltip (say colorpicker) without
> loading filter, bezier and css widget, I think we could leave this for a
> second step?

I think both are importants. But we can definitely do it in two changesets.
Dependencies pulled by each Tooltip implementation is pretty significant.
I'm expecting it to just split the win in two, depending on the targeted document, if it has colors, filter and cubic rules or not.

> ::: devtools/client/inspector/shared/tooltips-overlay.js:91
> (Diff revision 2)
> > -      // Color picker tooltip
> > -      this.colorPicker = new SwatchColorPickerTooltip(toolbox.doc, this.view.inspector);
> > -      // Cubic bezier tooltip
> > -      this.cubicBezier = new SwatchCubicBezierTooltip(toolbox.doc);
> > -      // Filter editor tooltip
> > -      this.filterEditor = new SwatchFilterTooltip(toolbox.doc,
> > +   * Lazily fetch a tooltip instance
> > +   *
> > +   * @param {String} name
> > +   *        Identifier name for the tooltip
> > +   */
> > +  get: function (name) {
> 
> let's use lazy getters for each tooltip to avoid impacts on the consumer
> code (same as what you did for get tooltips() / _tooltips)

Do you find it easier to process? I tried both approaches which end up with the same result.
I was wondering if there was other ways to handle lazyness. The tricky usecase is when you want to do something with tooltips, but only if it has been instanciated. Here, it is when we use "_tooltips".
Priority: -- → P2
Now that gcli is gone, this is the next significant code load during inspector startup.

First focus around tooltips-overlay.js:
  https://new.cleopatra.io/public/d464c24f920febb8118ce8e891ee9344bab2d898/calltree/?jsOnly&search=tooltips-overlay&thread=0
  36ms for tooltips-overlay.js module loading from computed.js
  26ms for tooltips-overlay.js:addToView called from rules.js

Then on tooltip widgets modules:
  https://new.cleopatra.io/public/d464c24f920febb8118ce8e891ee9344bab2d898/calltree/?jsOnly&search=widgets%2Ftool&thread=0
  9+7+6+4=26ms for loading all widgets/tooltip/*.js during tooltips-overlay.js module loading
  32ms for loading widget/tooltip/EventTooltipHelper.js from element-container.js (slow because of its acorn dependency) Looks like another thing to optimize...
  10+5+4+4=23ms for calling all tooltip widget constructors from addToView

We should both prevent loading these modules and also prevent calling tooltip constuctor until it is explicitely needed.
I've added one more changeset to prevent loading EventTooltipHelper.
I split the previous patch I r? into two changesets. One focused on delaying tooltips-overlay load, the other one delaying loading each tooltip widget.

I didn't changed the way I'm lazy loading the tooltip widgets. I would like you to answer my last question from comment 12 before.
Here is some profiler results with these patches:

# On: data:text/html,foo
No more tooltips-overlay, nor any tooltip widget: 
https://new.cleopatra.io/public/838f9956a9ef38452edffaf5184a1ef482481ba0/calltree/?jsOnly&search=tooltip&thread=0
(there is still some occurence of "tooltip" from definitions.js

# On: mozilla.org
Just tooltips-overlay and SwatchColorPicker:
https://new.cleopatra.io/public/d403b65c78fa9e24b370110a4376c8e61679b41e/calltree/?jsOnly&search=tooltip&thread=0

And in both cases, no more eventTooltipHelper.
Assignee: nobody → poirot.alex
Comment on attachment 8815306 [details]
Bug 1320939 - Lazy load tooltips-overlay from rule view.

https://reviewboard.mozilla.org/r/96286/#review109952

Looks good to me! Thanks for improving this.

::: devtools/client/inspector/computed/computed.js:220
(Diff revision 4)
> -  // Add the tooltips and highlightersoverlay
> -  this.tooltips = new TooltipsOverlay(this);
> +  // Lazily add the tooltips to the view.
> +  // Use `tooltips` if you want to automatically create an instance
> +  // Use `_tooltips` if you want to check if one already exists
> +  Object.defineProperty(this, "tooltips", {
> +    get() {
> +      delete this.tooltips;

This pattern is a bit similar to the lazyGetter/defineLazyGetter available in Loader and builtin-modules, but allows us to check if the property has been set by checking _tooltips on the object. Which is very nice and could be helpful in other places.

On one hand I think I'd like to have this wrapped in a shared helper, but then if "_tooltips" is created automatically it makes it a bit hard to understand. 

Maybe we could have a helper that takes both a "public" property name, and a "private" one:

> defineLazyProperty(this, "tooltips", "_tooltips", {
>   const TooltipsOverlay = require("devtools/client/inspector/shared/tooltips-overlay");
>   let tooltips = new TooltipsOverlay(this);
>   tooltips.addToView();
>   return tooltips;
> });

Not necessarily something that should be done in this patch, but if we reuse the pattern more and more that could be nice to have.

::: devtools/client/inspector/computed/computed.js:223
(Diff revision 4)
> +  Object.defineProperty(this, "tooltips", {
> +    get() {
> +      delete this.tooltips;
> +      const TooltipsOverlay = require("devtools/client/inspector/shared/tooltips-overlay");
> +      this.tooltips = this._tooltips = new TooltipsOverlay(this);
> -  this.tooltips.addToView();
> +      this.tooltips.addToView();

Not related to your patch but I think it doesn't make sense to keep addToView as a separate call. It should be merged with the TooltipsOverlay constructor (or called from it).

::: devtools/client/inspector/rules/views/text-property-editor.js:92
(Diff revision 4)
>    /**
>     * Boolean indicating if the name or value is being currently edited.
>     */
>    get editing() {
>      return !!(this.nameSpan.inplaceEditor || this.valueSpan.inplaceEditor ||
> -      this.ruleView.tooltips.isEditing) || this.popup.isOpen;
> +      (this.ruleView._tooltips && this.ruleView._tooltips.isEditing)) ||

I would prefer not to leak the tooltips/_tooltips approach outside of rules.js. Can we add a isEditing method in rules.js that does this ?
Attachment #8815306 - Flags: review?(jdescottes) → review+
Comment on attachment 8832046 [details]
Bug 1320939 - Lazy load all tooltip widgets until each is really used.

https://reviewboard.mozilla.org/r/108458/#review109966

Looks good, but keeping the review flag to finish our discussion on the pattern to use for lazy loading.

::: devtools/client/inspector/rules/test/browser_rules_authored_color.js:41
(Diff revision 2)
>      let selector = "#" + color.id;
>      yield selectNode(selector, inspector);
>  
>      let swatch = getRuleViewProperty(view, "element", "color").valueSpan
>          .querySelector(".ruleview-colorswatch");
> +    let cPicker = view.tooltips.colorPicker;

If we go with the get approach, this should be accessed through view.tooltips.get("colorPicker"). The toolips.colorPicker property should really remain private to tooltips-overlay and used only when we want to avoid instanciation.

::: devtools/client/inspector/shared/tooltips-overlay.js:85
(Diff revision 2)
>        this._onPreviewTooltipTargetHover.bind(this));
>  
> -    // MDN CSS help tooltip
> -    this.cssDocs = new CssDocsTooltip(toolbox.doc);
> +    this._isStarted = true;
> +  },
>  
> -    if (this.isRuleView) {
> +  /**

this.isRuleView is no longer used anywhere in this file and should be removed from the constructor.

::: devtools/client/inspector/shared/tooltips-overlay.js:121
(Diff revision 2)
> +        tooltip = new CssDocsTooltip(doc);
> +        break;
> +      default:
> +        throw new Error(`Unsupported tooltip '${name}'`);
>      }
> -
> +    this[name] = tooltip;

Let's resume our discussion about using this get method or use defineProperty with "tooltipName" (and "_tooltipName" for the private version).

There are a few reasons I'd prefer to use the same approach as in tooltips-overlay. 

First for consistency. The same result can be achieved with the same strategy so I find it confusing to not reuse it here. 

Then when I read the patch the first time I was confused about the lines containing "this.colorPicker". Where is it defined? Where does it come from. Then I realized the get function was writing this[name] on the object. I don't expect getters to actually set things on objects. I know that with lazy-*ing it's to be expected. But that's why sticking to one pattern would helpful in my opinion. 

Finally no impact on existing consumer code, which is nice.

I'll leave this open as an issue, let me know what you think.
Comment on attachment 8832046 [details]
Bug 1320939 - Lazy load all tooltip widgets until each is really used.

https://reviewboard.mozilla.org/r/108458/#review109974

::: devtools/client/inspector/shared/tooltips-overlay.js:115
(Diff revision 2)
> +          require("devtools/client/shared/widgets/tooltip/SwatchFilterTooltip");
> +        tooltip = new SwatchFilterTooltip(doc,
> -        this._cssProperties.getValidityChecker(this.view.inspector.panelDoc));
> +          this._cssProperties.getValidityChecker(this.view.inspector.panelDoc));
> +        break;
> +      case "cssDocs":
> +        const CssDocsTooltip = require("devtools/client/shared/widgets/tooltip/CssDocsTooltip");

eslint failure here (max-len)
Comment on attachment 8832047 [details]
Bug 1320939 - Lazy load tooltip event helpers.

https://reviewboard.mozilla.org/r/108460/#review109972

Looks good. 
Was there a reason why you didn't use a lazyRequireGetter here? 
Is it for fear of devtools-html compatibility? If so I'd be happy to file a bug to make our webpack config support this.

::: devtools/client/inspector/markup/views/element-container.js:15
(Diff revision 2)
>  const Services = require("Services");
>  const Heritage = require("sdk/core/heritage");
>  const {Task} = require("devtools/shared/task");
>  const nodeConstants = require("devtools/shared/dom-node-constants");
>  const clipboardHelper = require("devtools/shared/platform/clipboard");
>  const {setImageTooltip, setBrokenImageTooltip} =

I think setImageTooltip and setBrokenImageTooltip are also called only on mouse events, but I guess the file is simple enough (and has no dependencies) so it probably doesn't need lazy loading.
Attachment #8832047 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #23)
> Let's resume our discussion about using this get method or use
> defineProperty with "tooltipName" (and "_tooltipName" for the private
> version).
> 
> There are a few reasons I'd prefer to use the same approach as in
> tooltips-overlay. 
>
> First for consistency. The same result can be achieved with the same
> strategy so I find it confusing to not reuse it here. 

Yes. I kept the two distinct approaches to discuss them but you are right that would be great to be consistant.
I had the "get" version later, while working on the second patch.
I tend to prefer the more explicit "get" version. Magic lazy getter tend to be too magic.
So I would suggest using the "get" approach in both patches.
Note that there is a significant different between the two patches.
For tooltips there is only one thing to lazify, which tends to go in favor of the _tooltip magic getter,
whereas for widgets, there is many, which, to me, better fit the getter function.

> Then when I read the patch the first time I was confused about the lines
> containing "this.colorPicker". Where is it defined? Where does it come from.
> Then I realized the get function was writing this[name] on the object. I
> don't expect getters to actually set things on objects. I know that with
> lazy-*ing it's to be expected. But that's why sticking to one pattern would
> helpful in my opinion. 

You are right, that is misleading. May be we should have an "has()" method and always use "get()".
So that:
-  if (this.colorPicker && this.colorPicker.xxx)
becomes:
+  if (this.has("colorPicker") && this.get("colorPicker").xxx)

The cons are that it is definitely more verbose, it may also be harder to share any helper to implement such pattern?
The pro is that there is no magic, things are explicit. You know where has/get are and comes from.

If you think this get/has think doesn't really help, I'll just go use magic getters.
(In reply to Julian Descottes [:jdescottes] from comment #25)
> Comment on attachment 8832047 [details]
> Bug 1320939 - Lazy load tooltip event helpers.
> 
> https://reviewboard.mozilla.org/r/108460/#review109972
> 
> Looks good. 
> Was there a reason why you didn't use a lazyRequireGetter here? 
> Is it for fear of devtools-html compatibility? If so I'd be happy to file a
> bug to make our webpack config support this.

Yes, that's just for that! I would be happy to rebase my patch over that.
If we could have lazyRequireGetter and lazyGetter that would be helpful.
I could use lazyGetter to implement the magic lazy getter if you end up choosing that over get/has functions.
(In reply to Alexandre Poirot [:ochameau] from comment #27)
> 
> Yes, that's just for that! I would be happy to rebase my patch over that.
> If we could have lazyRequireGetter and lazyGetter that would be helpful.
> I could use lazyGetter to implement the magic lazy getter if you end up
> choosing that over get/has functions.

Ah! Just go ahead with using lazy*Getters, I'll fix the webpack config accordingly (won't prevent testing/landing anyway).

(In reply to Alexandre Poirot [:ochameau] from comment #26)
> 
> You are right, that is misleading. May be we should have an "has()" method
> and always use "get()".
> So that:
> -  if (this.colorPicker && this.colorPicker.xxx)
> becomes:
> +  if (this.has("colorPicker") && this.get("colorPicker").xxx)
> 
> The cons are that it is definitely more verbose, it may also be harder to
> share any helper to implement such pattern?
> The pro is that there is no magic, things are explicit. You know where
> has/get are and comes from.
> 
> If you think this get/has think doesn't really help, I'll just go use magic
> getters.

I don't like magic either, and I actually try to avoid getters as much as possible. Your get/has proposal sounds pretty good. I think the naming is a bit too broad, but I can't think of a good short name (best I have is getLazyProperty, which is not great).

The only other thing is that this pattern is not transparent for consumers that used to simply access the property from outside of the class. The lazy nature of the properties becomes part of the contract, which is not necessarily bad. In general we should avoid directly accessing properties of other objects, but it's not really a rule we follow in devtools codebase. If we are ok with leaking the lazyness, then I'm ok with get/has as well.
Comment on attachment 8832046 [details]
Bug 1320939 - Lazy load all tooltip widgets until each is really used.

https://reviewboard.mozilla.org/r/108458/#review112780

I am clearing my review flag so that you can update the patch based on our discussion.
I just realized that it's easy to miss updates to a review if the reviewer simpl leaves the review flag as is.
Attachment #8832046 - Flags: review?(jdescottes)
(In reply to Julian Descottes [:jdescottes] from comment #22)
> ::: devtools/client/inspector/rules/views/text-property-editor.js:92
> (Diff revision 4)
> >    /**
> >     * Boolean indicating if the name or value is being currently edited.
> >     */
> >    get editing() {
> >      return !!(this.nameSpan.inplaceEditor || this.valueSpan.inplaceEditor ||
> > -      this.ruleView.tooltips.isEditing) || this.popup.isOpen;
> > +      (this.ruleView._tooltips && this.ruleView._tooltips.isEditing)) ||
> 
> I would prefer not to leak the tooltips/_tooltips approach outside of
> rules.js. Can we add a isEditing method in rules.js that does this ?

RuleView already implements isEditing, but it is different.

http://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#524
  get isEditing() {
    return this.tooltips.isEditing ||
      this.element.querySelectorAll(".styleinspector-propertyeditor")
        .length > 0;
  },

If I understand  styleinspector-propertyeditor check correctly it looks if there is *any* inplace editor.
So that TextPropertyEditor.editing becomes wrong as its ".editing" field becomes true if any other property is being edited.

Still. Using ruleView internals looks wrong, so I came up view RuleView.isAnyTooltipEditing getter...
But this check for "is any tooltip currently editing" from a given property instance is weak.
The editing tooltip may be one for another property.
Ideally, we would check if any of the tooltip spawn for this current property is editing.
We may be able to do that by toggling some "isOurTooltipCurrentlyEditing" state in _onStartEditing and on _onSwatchCommit/onSwatchRevert.

Thoughts?
(In reply to Julian Descottes [:jdescottes] (PTO until 27 March) from comment #25)
> Comment on attachment 8832047 [details]
> Bug 1320939 - Lazy load tooltip event helpers.
> 
> https://reviewboard.mozilla.org/r/108460/#review109972
> 
> Looks good. 
> Was there a reason why you didn't use a lazyRequireGetter here? 
> Is it for fear of devtools-html compatibility? If so I'd be happy to file a
> bug to make our webpack config support this.

Yes. I switched to lazyRequireGetter.
(In reply to Julian Descottes [:jdescottes] (PTO until 27 March) from comment #28)
> I don't like magic either, and I actually try to avoid getters as much as
> possible. Your get/has proposal sounds pretty good. I think the naming is a
> bit too broad, but I can't think of a good short name (best I have is
> getLazyProperty, which is not great).

I ended up using a much more explicit name: "getTooltip".
That's the only one public method being used at the end.

> The only other thing is that this pattern is not transparent for consumers
> that used to simply access the property from outside of the class. The lazy
> nature of the properties becomes part of the contract, which is not
> necessarily bad. In general we should avoid directly accessing properties of
> other objects, but it's not really a rule we follow in devtools codebase. If
> we are ok with leaking the lazyness, then I'm ok with get/has as well.

Then, I stopped using any of the existing getters. Now you always have to go through getTooltip.
Except privately, within TooltipOverlay.
But instead of storing them as magic attributes on the TooltipOverlay instance,
it is now stored in a map, which becomes super handy when we have to call the same method on all currently instanciated tooltips! It looks much better from tooltips-overlay.js.
But in order to do that, I had to patch SwatchColorPicker, SwatchBasedColorPicker, HTMLTooltip and CssDocsTooltip in order to ensure exposing all these hide/revert/destroy/isEditing method on all tooltip instances.
Comment on attachment 8832046 [details]
Bug 1320939 - Lazy load all tooltip widgets until each is really used.

https://reviewboard.mozilla.org/r/108458/#review126414

The image preview and font preview tooltips no longer work in the rule view.

The issue is that the preview tooltips are created in _onPreviewTooltipTargetHover, which is only called as the callback for startTogglingOnHover, which is only attached after creating the tooltips.

So here I guess we would have to:
- decouple the Tooltip and the TooltipToggle (startTogglingOnHover is only used in markup.js, tooltips-overlay.js, request-list-content.js and shadereditor.js)
- allow tooltip toggle to use a "tooltip getter" as a constructor argument, so that we don't have to pass the tooltip instance right away

The other option is to initialize the preview tooltip from the beginning for now, if this already yields significant perf improvements (we would still be skipping the load of all the swatchbased tooltips and their related widgets.

The other thing that seems a bit crazy is that this was not caught by any test. I'll log a bug for that.

::: devtools/client/inspector/shared/tooltips-overlay.js:44
(Diff revision 3)
>   *        Either the rule-view or computed-view panel
>   */
>  function TooltipsOverlay(view) {
>    this.view = view;
>  
>    let {CssRuleView} = require("devtools/client/inspector/rules/rules");

no longer used ?

::: devtools/client/inspector/shared/tooltips-overlay.js:74
(Diff revision 3)
>    addToView: function () {
>      if (this._isStarted || this._isDestroyed) {
>        return;
>      }
>  
> -    let { toolbox } = this.view.inspector;
> +    this._isStarted = true;

not related to this changeset, but that makes me realize the addToView/removeFromView logic is completely flawed.

This class is used in the rule & computed views. When we call "removeFromView" all the shared tooltip instances are destroyed, so we can't really remove it from 1 view while keeping it for another ... Luckily at the moment we just removeFromView for both views at the same time so this is not an issue.

::: devtools/client/inspector/shared/tooltips-overlay.js:196
(Diff revision 3)
>        // There is no tooltip type defined for the hovered node
>        return false;
>      }
>  
> -    if (this.isRuleView && this.colorPicker.tooltip.isVisible()) {
> -      this.colorPicker.revert();
> +    for (let [, tooltip] of this._instances) {
> +      if (tooltip.isVisible()) {

In the previous implementation, we were hiding all tooltips except the previewTooltip. 

Feature is broken at the moment so we'll need to check if this is an issue after we restore it.
Attachment #8832046 - Flags: review?(jdescottes)
See Also: → 1351266
Attachment #8815306 - Attachment is obsolete: true
Ok, so. I ended up dropping the lazy loading of tooltip-overlay.js.
Instead I made it load most of its dependencies lazily.
That's because I do want to always call getTooltip("previewTooltip") in addToView(),
so that loading tooltip-overlay lazily prevents always calling it...

I think I would like to proceed with this patch even if the previewTooltip still fetches some extra dependencies.

Also I removed the call to addToView from rules.js and computed.js.
Instead, I call it from TooltipsOverlay constructor.
Comment on attachment 8832046 [details]
Bug 1320939 - Lazy load all tooltip widgets until each is really used.

https://reviewboard.mozilla.org/r/108458/#review127344

Looks good to me! 
Just one last issue I didn't think about the first time, sorry.

::: devtools/client/inspector/shared/tooltips-overlay.js:58
(Diff revision 4)
>  }
>  
>  TooltipsOverlay.prototype = {
> +  get _cssProperties() {
> +    delete TooltipsOverlay.prototype._cssProperties;
> +    return TooltipsOverlay.prototype._cssProperties =

eslint failure here: `Return statement should not contain assignment.`

::: devtools/client/inspector/shared/tooltips-overlay.js:64
(Diff revision 4)
> +      getCssProperties(this.view.inspector.toolbox);
> +  },
> +
>    get isEditing() {
> -    return this.colorPicker.tooltip.isVisible() ||
> -           this.colorPicker.eyedropperOpen ||
> +    for (let [, tooltip] of this._instances) {
> +      if (tooltip.isVisible() ||

I think it would be nice to have a base isEditing defined in SwatchBasedEditorTooltip that would return true if isVisible() and override it in the colorPicker version.

This way we can simply do 

> if (typeof tooltip.isEditing === "function" && tooltip.isEditing()) { ... }

here. And also we won't include the previewTooltip in the list of tested tooltips, which is more consistent with the previous implementation.
Attachment #8832046 - Flags: review?(jdescottes) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c44d096d361a
Lazy load tooltip event helpers. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/35a4c18b0b75
Lazy load all tooltip widgets until each is really used. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/c44d096d361a
https://hg.mozilla.org/mozilla-central/rev/35a4c18b0b75
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: