1020291 - Clean up the styleinspector's tooltips and highlighters initialization

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

The styleinspector (rule and computed views) is getting more and more interactive with various tooltips appearing on mousemove of certain properties, and highlighters being displayed in the page on mousemove of other selectors or properties.

The code required for this suffers from a couple of problems:
- it has very little to do with the styleinspector's core functionality (displaying/editing properties)
- it is mostly duplicated between the rule and computed views

The code could be made a lot simpler to understand and maintain if we moved this part into a kind of styleinspector interaction manager.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug1020291-refactor-styleinspector-interactions v1.patch

v1

- now both rule and computed views have a getNodeInfo method that returns information about a given node in the view (whether it's a selector, property, value, url, ...)
- all the code related to highlighters and tooltips is now put in common in a new module style-inspector-overlays.js
- both views only require that module and initialize it
- I've modified a few tests to make them pass with these changes
- all the tests I made seem to show that this works.

Comment 2

[Patrick Brosset <:pbro>]

Try build for this patch: https://tbpl.mozilla.org/?tree=Try&rev=26fc0bbdb7fb

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8440712 [details] [diff] [review]
bug1020291-refactor-styleinspector-interactions v1.patch

Heather, this is a refactor patch that helps in sharing as much code as possible between the rule-view and computed-view when it comes to tooltips and highlighters.
There's a bunch of new tooltips/highlighters we could add in the future (starting with bug 971662) to these views, so this kind of refactoring is needed.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug1020291-refactor-styleinspector-interactions v2.patch

Sorry I missed a bug in the previous patch.
New try: https://tbpl.mozilla.org/?tree=Try&rev=bfda4d0b92ab

Comment 5

[Heather Arthur [:harth]]

Comment on attachment 8441383 [details] [diff] [review]
bug1020291-refactor-styleinspector-interactions v2.patch

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

This makes the code much simpler, nice!

::: browser/devtools/styleinspector/rule-view.js
@@ +1103,5 @@
>    this._showEmpty();
>  
> +  // Add the tooltips and highlighters to the view
> +  this.tooltips = new overlays.TooltipsOverlay(this);
> +  this.tooltips.start();

To me, I'm not sure what "start" means, maybe 'addTo<X>'. Up to you.

::: browser/devtools/styleinspector/style-inspector-overlays.js
@@ +68,5 @@
> +
> +exports.HighlightersOverlay = HighlightersOverlay;
> +
> +HighlightersOverlay.prototype = {
> +  start: function() {

Function comment headers would be nice in this file.

@@ +147,5 @@
> +
> +  _hideCurrent: function() {
> +    if (this.highlighterShown) {
> +      this._getHighlighter(this.highlighterShown).then(h => {
> +        h.hide();

change 'h' to a more descriptive var name, is it the highlighter?

@@ +252,5 @@
> +    if (type === VIEW_NODE_IMAGE_URL_TYPE && inspector.hasUrlToImageDataResolver) {
> +      let dim = Services.prefs.getIntPref(PREF_IMAGE_TOOLTIP_SIZE);
> +      let uri = CssLogic.getBackgroundImageUriFromProperty(prop.value,
> +        prop.sheetHref); // sheetHref is undefined for computed-view
> +                                   // properties, but we don't care as uris are

this line doesn't line up with the comment above.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug1020291-refactor-styleinspector-interactions v3.patch

Thanks Heather for the review.
Here's a new patch that addresses all review comments:
- added jsdoc comments
- renamed one-letter variables
- renamed start/stop functions
- also moved a couple of functions around to be simpler to read

Comment 7

[Patrick Brosset <:pbro>]

https://tbpl.mozilla.org/?tree=Try&rev=bfda4d0b92ab
https://hg.mozilla.org/integration/fx-team/rev/1140b6e20b84

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1140b6e20b84