1277264 - Migrate netmonitor image preview tooltip to HTML Tooltip

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Julian Descottes [:jdescottes]]

The goal of this bug is to migrate the previewTooltip of the TooltipsOverlay class to a HTMLTooltip instance. 

After Bug 1276876 is resolved, the net-monitor will be the last client using the XUL tooltip for Image Previews. After migrating the net-monitor, we will be able to remove the "setImageContent" implementation in the XUL Tooltip.js class.

Comment 1

[Julian Descottes [:jdescottes]]

A new tooltip is being added to the net monitor in Bug 1134073. Let's wait until this is implemented before migrating the netmonitor to use the HTML Tooltip.

Comment 2

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 1: Migrate netmonitor tooltips to HTML Tooltip

Comment 3

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 2: Remove setImageContent method from Tooltip.js

Comment 4

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 1: Migrate netmonitor tooltips to HTML Tooltip

- migrated netmonitor tooltips to HTMLTooltip
- moved styles to tooltips.css
- fine-tuned the stack trace CSS (flex, ellipsis)

Comment 5

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e4410902ac4

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8761589 [details] [diff] [review]
Part 2: Remove setImageContent method from Tooltip.js

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

Cheers!

One small thing: we can also get rid of the l10n dependency loaded at the end of Tooltip.js
r=me with this change.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8761609 [details] [diff] [review]
Part 1: Migrate netmonitor tooltips to HTML Tooltip

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

Thanks for taking this one, great work!

A few minor nits, keeping my review flag to discuss about closeOnEvents.

Also ... I can see what you meant about the confusing documents. 
It's true that we need to pass around a lot of document objects to create elements when using the HTMLTooltip.

I will think about ways to improve that. In the meantime I just want to avoid having to many callers relying on properties of the HTML Tooltip, in case we change them later, which is why I encourage you to use "document" directly here. But I agree there's an overall issue here.

::: devtools/client/netmonitor/netmonitor-view.js
@@ +19,5 @@
>  });
>  
>  const {ToolSidebar} = require("devtools/client/framework/sidebar");
> +const {HTMLTooltip} = require("devtools/client/shared/widgets/HTMLTooltip");
> +const { setImageTooltip, getImageDimensions } =

nit: no spaces after and before the braces (for consistency within the file, although this is not consistent across the codebase :/ )

@@ +470,1 @@
>        closeOnEvents: [{

closeOnEvents is not implemented for the HTML tooltip. We need to either reimplement it or emulate it here. 

For now I think we could just add an event listener on "scroll" in the netmonitor-view, that calls this.tooltip.hide() if the tooltip is visible. In the future, we might move this feature back to the HTMLTooltip, because most of our tooltips are not hidden on scroll, and it's actually annoying.

@@ +2322,3 @@
>      let src = formDataURI(mimeType, encoding, string);
> +    let maxDim = REQUESTS_TOOLTIP_IMAGE_MAX_DIM;
> +    let { naturalWidth, naturalHeight } = yield getImageDimensions(tooltip.doc, src);

Instead of tooltip.doc, you can use the "document" of the netmonitor-view directly here.

@@ +2322,5 @@
>      let src = formDataURI(mimeType, encoding, string);
> +    let maxDim = REQUESTS_TOOLTIP_IMAGE_MAX_DIM;
> +    let { naturalWidth, naturalHeight } = yield getImageDimensions(tooltip.doc, src);
> +    let options = { maxDim, naturalWidth, naturalHeight };
> +    yield setImageTooltip(tooltip, tooltip.doc, src, options);

nit: setImageTooltip is no longer async, so no need to yield.
Same remark about using document vs tooltip.doc.

@@ +2335,5 @@
>        return false;
>      }
>  
> +    let doc = tooltip.panel.ownerDocument;
> +    let el = doc.createElementNS(HTML_NS, "div");

Instead of tooltip.panel.ownerDocument, you can use the "document" of the netmonitor-view directly here.

@@ +2374,5 @@
>  
>        el.appendChild(frameEl);
>      }
>  
> +    yield tooltip.setContent(el, REQUESTS_TOOLTIP_STACK_TRACE_WIDTH);

nit: setContent is no longer asynchronous, no need to yield.

I know the JSDoc is misleading about this (@return {Promise}), I have an upcoming commit that fixes it. But feel free to remove it here if you want.

Comment 8

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64f0fff40aef

Comment 9

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 1: Migrate netmonitor tooltips to HTML Tooltip

- as closeOnEvents is no longer supported, I'm adding and removing the scroll listener manually
- no longer setting obsolete property tooltip.defaultPosition
- not using yield on setContent and other methods that are no longer async
- fixed the CSS styling to show margin on all sides when the stack trace is scrolling
- in browser_net_image-tooltip mochitest, I'm hiding the tooltip explicitly to prevent window leaks

I continue to use the tooltip.doc for creating elements, as discussed on IRC

Comment 10

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 2: Remove setImageContent method from Tooltip.js

- removed the l10n dead code, too

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8761976 [details] [diff] [review]
Part 1: Migrate netmonitor tooltips to HTML Tooltip

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

Great work Jarda! Thanks for migrating the netmonitor tooltips.

See my comment regarding calling destroy on the tooltip instance of the netmonitor.
It is not directly related to this bug, so let me know if you prefer to log a follow up.

::: devtools/client/netmonitor/test/browser_net_image-tooltip.js
@@ +55,5 @@
> +
> +    // Hide the tooltip to prevent window leaks
> +    let onHidden = tooltip.once("hidden");
> +    tooltip.hide();
> +    yield onHidden;

It's good to hide the tooltip here to be safe. r=me as long as try is green.

Panels using tooltips are usually calling destroy() on their tooltip in their own destroy method. I think we should do it in the netmonitor too. You can either try to do this here, or we can file a follow up.

Comment 12

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ad9f07bea1e

Comment 13

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 1: Migrate netmonitor tooltips to HTML Tooltip

- destroy the tooltip in RequestsMenuView.destroy
- rebased

Comment 14

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 2: Remove setImageContent method from Tooltip.js

- rebased

Comment 15

[Jarda Snajdr [:jsnajdr]]

(In reply to Julian Descottes [:jdescottes] from comment #11)
> It's good to hide the tooltip here to be safe. r=me as long as try is green.

I removed the code that was hiding the tooltip, because the right way how to fix the window leaks is to destroy the tooltip in the RequestsMenuView destructor, not to cover it up in the test.

> Panels using tooltips are usually calling destroy() on their tooltip in
> their own destroy method. I think we should do it in the netmonitor too. You
> can either try to do this here, or we can file a follow up.

Yes, that's actually the right way to fix the window leaks. I did it and the try run is green.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9e5f228d7cc7
Part 1: Migrate netmonitor tooltips to HTML Tooltip. r=jdescottes
https://hg.mozilla.org/integration/fx-team/rev/728187896762
Part 2: Remove setImageContent method from Tooltip.js r=jdescottes

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e5f228d7cc7
https://hg.mozilla.org/mozilla-central/rev/728187896762

Comment 18

[Ada [:adalucinet]]

Verified with latest Nightly 50.0a1, across platforms [1]. 
Found 1 issue → bug 1284851. Jarda, what's your input regarding it? Thanks in advance!

Marking here accordingly since the new bug is already tracked separately. 

[1] Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit