932896 - Image preview popup doesn't work for remote targets

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Paul Rouget [:paul]]

Attachment: screenshot

The popup says "could not load the image".

That's understandable. The image URL can only be resolved by the remote gecko.

Comment 1

[Paul Rouget [:paul]]

For Firefox OS targets, we send images (app icons) as dataURLs. If we implement a inspector specific method (`resolveURLAsDataURL()`), maybe we can reconsider implementing a "remote URL resolver" actor that would transform any URL into a dataURL with the right content type.

See comments 1 to 5 in bug 898000. There's even a working patch (comment 1).

Comment 2

[Paul Rouget [:paul]]

It only works in the markupview with <img> and <canvas> tags. It doesn't work in the rule view and the computed view.

Comment 3

[Paul Rouget [:paul]]

Attachment: draft

Comment 4

[Paul Rouget [:paul]]

I still need to figure out how to set the right preview size (400 if local, 200 if remote). And also to write a test.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8333856 [details] [diff] [review]
draft

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

::: browser/devtools/styleinspector/computed-view.js
@@ +524,5 @@
> +
> +    let uri = CssLogic.getBackgroundImageUriFromProperty(target.textContent);
> +    if (uri) {
> +      let inspectorFront = this.styleInspector.inspector.inspector;
> +      inspectorFront.getImageDataFromURL(uri, IMAGE_PREVIEW_MAX_DIM).then(res => {

See my comment about code duplication and possibly moving this code to tooltip.js

::: browser/devtools/styleinspector/rule-view.js
@@ +1145,5 @@
>      let href = property.rule.domRule.href;
>  
>      // Fill some content
> +
> +    // FIXME - a pref?

A pref would indeed make sense here.
In which case `const IMAGE_PREVIEW_MAX_DIM = 400;` from markup-view.js should be set to that pref too.

@@ +1151,5 @@
> +
> +    let uri = CssLogic.getBackgroundImageUriFromProperty(property.value, href);
> +    if (uri) {
> +      let inspectorFront = this.inspector.inspector;
> +      inspectorFront.getImageDataFromURL(uri, IMAGE_PREVIEW_MAX_DIM).then(res => {

What do you think about keeping a function in tooltip.js like `setImageContentFromUri(uri, inspectorFront)` ? This way, everywhere we need an image preview tooltip but only have a uri, we can call that function and avoid duplicating this code?

Comment 6

[Paul Rouget [:paul]]

Patrick, do you think you can take over this bug? I don't think I will have time to work on this anytime soon.

Comment 7

[Patrick Brosset <:pbro>]

Done!
I should be able to work on this after I'm done with bug 952277.

Comment 8

[Patrick Brosset <:pbro>]

I've started working on this again.
- The inspector actor now has a getImageDataFromURL method.
- Trait added to the root actor to let the toolbox know if it can call the method.
- Moved the rule-view and computed-view code that retrieves the data-url from the server to Tooltip.js so it's not duplicated.
- Made it so that if the image is already a data-url, no round trip to the server is made.
- Made it work on XUL content too (wasn't working until now as images are copied into a canvas to get the data-url from there, but the canvas tag wasn't created with the XHTML namespace).

I identified the scenario that fails on fxos:
- apps may define background images in css
- the image urls are relative: ../my/image.png
- looking at the computed-view tells us that the absolute path for this is something like:
  app://<some_id_>/my/image.png
- this absolute URL can't be called from the toolbox, it can only work if called from within the target app.

This can be tested by installing this app: https://github.com/robnyman/Firefox-OS-Boilerplate-App
and inspecting the <header> node.

Comment 9

[Patrick Brosset <:pbro>]

Attachment: bug932896-part1-image-tooltip-remote-target v1.patch

Contains the changes described in my last comment.

Comment 10

[Patrick Brosset <:pbro>]

Attachment: bug932896-part2-reformat-file v1.patch

Nothing much to review here, I noticed that inspector.js and rule-view.js had mixed formatting styles, so this patch only makes the formatting consistent.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug932896-part3-tooltip-image-size-pref.patch

Adds a user pref for managing the tooltip image default size.

Comment 12

[Patrick Brosset <:pbro>]

Ongoing try build for the 3 patches above: https://tbpl.mozilla.org/?tree=Try&rev=b7ac84552b1b

Comment 13

[Joe Walker [:jwalker]]

Comment on attachment 8366588 [details] [diff] [review]
bug932896-part3-tooltip-image-size-pref.patch

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

::: browser/app/profile/firefox.js
@@ -733,5 @@
>  
>  // At startup, if the handler service notices that the version number in the
>  // region.properties file is newer than the version number in the handler
>  // service datastore, it will add any new handlers it finds in the prefs (as
> -// seeded by this file) to its datastore.  

firefox.js is outside of the devtools module [1] so technically we should get a review from a Browser peer before making this change [2].

I think we should flag up to a browser peer what's going on (perhaps Tim Taubert would be good), but we should avoid unnecessary changes in another module. I also think there is an argument that the list of prefs is actually part of devtools anyway, so maybe formal review isn't needed.

[1]: See "Source Dir(s)" on https://wiki.mozilla.org/Modules/All#Devtools for example
[2]: "A module owner's OK is required to check code into that module" from https://www.mozilla.org/hacking/module-ownership.html

Comment 14

[Joe Walker [:jwalker]]

Comment on attachment 8366586 [details] [diff] [review]
bug932896-part1-image-tooltip-remote-target v1.patch

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

::: browser/devtools/shared/widgets/Tooltip.js
@@ +504,5 @@
>      this.panel.setAttribute("clamped-dimensions", "");
>    },
>  
>    /**
> +   * Uses the provided inspectorFront's getImageDataFromURL's method to resolve

* Uses the provided inspectorFront's getImageDataFromURL method to resolve
?

@@ +511,5 @@
> +   *
> +   * @return a promise that resolves when the image is shown in the tooltip
> +   */
> +  setRelativeImageContent: function(imageUrl, inspectorFront, maxDim) {
> +    if (imageUrl && inspectorFront) {

You don't need an inspectorFront with a data url, so maybe we can ditch this if?

@@ +526,5 @@
> +            });
> +          }
> +        });
> +      }
> +    }

Maybe "return promise.resolve();" at the end, so users can setRelativeImageContent().then(...) safely.

::: browser/devtools/styleinspector/computed-view.js
@@ +25,4 @@
>  const HTML_NS = "http://www.w3.org/1999/xhtml";
>  const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +// FIXME: move to a pref just like the computed-view and markup-view
> +const IMAGE_PREVIEW_MAX_DIM = 400;

Technically this is part of patch 3, so we'll need to merge the patches before commit to ensure we're rebase safe.

@@ +535,5 @@
>        let propValue = target;
>        let propName = target.parentNode.querySelector(".property-name");
>        if (propName.textContent === "transform") {
> +        return this.tooltip.setCssTransformContent(propValue.textContent,
> +          this.pageStyle, this.viewedElement);

Not sure what to do with this because my un-easiness pre-exists your changes.

I've 2 complaints: _buildTooltipContent sometimes returns a promise and sometimes it doesn't, so you need a promise.resolve on every call. Also setCssTransformContent is an async setter. I'd be tempted to call it updateCssTransformContent to prevent anyone assuming sync.

This means that callers to _buildTooltipContent can't assume a promise because sometimes it doesn't return one, but also must check for a promise because the guts could be async.

Perhaps we should at least "return promise.resolve(undefined)" by default?

::: toolkit/devtools/server/actors/inspector.js
@@ +2344,5 @@
> +    let onLoad = () => {
> +      img.removeEventListener("load", onLoad, false);
> +      let imageData = imageToImageData(img, maxDim);
> +      if (!imageData) {
> +        deferred.resolve(null);

I can't help thinking we should reject here. Maybe even move the try/catch from imageToImageData to here also?
Aren't users of this API going to do something different if they can't get any image data?

::: toolkit/devtools/styleinspector/css-logic.js
@@ +46,5 @@
>  const RX_CONNECTORS = /\s*[\s>+~]\s*/g;
>  const RX_ID = /\s*#\w+\s*/g;
>  const RX_CLASS_OR_ATTRIBUTE = /\s*(?:\.\w+|\[.+?\])\s*/g;
>  const RX_PSEUDO = /\s*:?:([\w-]+)(\(?\)?)\s*/g;
> +const RX_BACKGROUND_IMAGE = /url\([\'\"]?(.*?)[\'\"]?\)/;

We don't need to escape the ' or the " do we?

  const RX_BACKGROUND_IMAGE = /url\(['"]?(.*?)['"]?\)/;

I'm unclear on CSS escaping rules. Do you know if this is valid?

  background-image: url("http://example.com/blah().png");

Comment 15

[Patrick Brosset <:pbro>]

Thanks Joe for the reviews. I will address your comments in the next couple of days.
The try build is green.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8366588 [details] [diff] [review]
bug932896-part3-tooltip-image-size-pref.patch

I've added a new pref for a change in the DevTools related to the size of image tooltips in the inspector.
This patch therefore touches a file outside of the DevTools module.
Could you please review that change?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8366588 [details] [diff] [review]
bug932896-part3-tooltip-image-size-pref.patch

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

I don't mind having those white space fixes too much but I wonder whether you have some kind of editor setting that removes trailing white space automatically? It's usually best to keep the patch as small as possible and not override too much blame information.

::: browser/app/profile/firefox.js
@@ +1103,3 @@
>  pref("devtools.inspector.show_pseudo_elements", true);
> +// The default size for image preview tooltips in the rule-view/computed-view/markup-view
> +pref("devtools.inspector.imagePreviewTooltipSize", 300);

I didn't read the bug to see if that was already considered but a static image size seems suboptimal. You could make it depend on the screen or window size but that might be hard to change for users then. I'll just leave that here as food for thought.

Comment 18

[Patrick Brosset <:pbro>]

Thanks Joe. I will attach a patch soon.
Please see my comments inline below:

(In reply to Joe Walker [:jwalker] from comment #14)
> Comment on attachment 8366586 [details] [diff] [review]
> bug932896-part1-image-tooltip-remote-target v1.patch
> 
> Review of attachment 8366586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +504,5 @@
> >      this.panel.setAttribute("clamped-dimensions", "");
> >    },
> >  
> >    /**
> > +   * Uses the provided inspectorFront's getImageDataFromURL's method to resolve
> 
> * Uses the provided inspectorFront's getImageDataFromURL method to resolve
> ?
Done.
> @@ +511,5 @@
> > +   *
> > +   * @return a promise that resolves when the image is shown in the tooltip
> > +   */
> > +  setRelativeImageContent: function(imageUrl, inspectorFront, maxDim) {
> > +    if (imageUrl && inspectorFront) {
> 
> You don't need an inspectorFront with a data url, so maybe we can ditch this
> if?
Yes you're right. Done.
> @@ +526,5 @@
> > +            });
> > +          }
> > +        });
> > +      }
> > +    }
> 
> Maybe "return promise.resolve();" at the end, so users can
> setRelativeImageContent().then(...) safely.
Done.
> ::: browser/devtools/styleinspector/computed-view.js
> @@ +25,4 @@
> >  const HTML_NS = "http://www.w3.org/1999/xhtml";
> >  const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> > +// FIXME: move to a pref just like the computed-view and markup-view
> > +const IMAGE_PREVIEW_MAX_DIM = 400;
> 
> Technically this is part of patch 3, so we'll need to merge the patches
> before commit to ensure we're rebase safe.
Ok, I will fold the 3 patches before checking this in.
> @@ +535,5 @@
> >        let propValue = target;
> >        let propName = target.parentNode.querySelector(".property-name");
> >        if (propName.textContent === "transform") {
> > +        return this.tooltip.setCssTransformContent(propValue.textContent,
> > +          this.pageStyle, this.viewedElement);
> 
> Not sure what to do with this because my un-easiness pre-exists your changes.
> 
> I've 2 complaints: _buildTooltipContent sometimes returns a promise and
> sometimes it doesn't, so you need a promise.resolve on every call. Also
> setCssTransformContent is an async setter. I'd be tempted to call it
> updateCssTransformContent to prevent anyone assuming sync.
> 
> This means that callers to _buildTooltipContent can't assume a promise
> because sometimes it doesn't return one, but also must check for a promise
> because the guts could be async.
> 
> Perhaps we should at least "return promise.resolve(undefined)" by default?
I think one problem here was the naming of _buildTooltipContent. It really should be called something like _onTooltipTargetHover, because it is a callback executed by the Tooltip class whenever a potential "tooltip-able" target is hovered. This callback is supposed to either return true/false to signal if the tooltip should be shown or not on the target, or return a promise that resolves or rejects.
So I've changed the name and made sure it always returns a promise.
> ::: toolkit/devtools/server/actors/inspector.js
> @@ +2344,5 @@
> > +    let onLoad = () => {
> > +      img.removeEventListener("load", onLoad, false);
> > +      let imageData = imageToImageData(img, maxDim);
> > +      if (!imageData) {
> > +        deferred.resolve(null);
> 
> I can't help thinking we should reject here. Maybe even move the try/catch
> from imageToImageData to here also?
> Aren't users of this API going to do something different if they can't get
> any image data?
You're right, I've changed this to reject instead and moved the try/catch out of imageToImageData and up into the callers.
Working on that part made me realized that, since the image URL could be anything (not necessarily a known/cached resource), we have to get ready for 2 things:
- 404: easy to catch with an img.onerror = () => deferred.reject();
- hanging network request. This is bad because since protocol.js queues up requests (at least for the same actor), it means that as long as the promise hasn't resolved/rejected, no other request can go through.
Essentially, this means if you quickly hover over an invalid background-image URL, then move your mouse away to another valid URL, you won't see any tooltip.
I guess the request will finally be dropped, but I don't know if the onerror event handler will be called.
In any case, for the tooltip usecase, we shouldn't wait very long. So perhaps I should just start a timeout and reject the promise at some stage.
> ::: toolkit/devtools/styleinspector/css-logic.js
> @@ +46,5 @@
> >  const RX_CONNECTORS = /\s*[\s>+~]\s*/g;
> >  const RX_ID = /\s*#\w+\s*/g;
> >  const RX_CLASS_OR_ATTRIBUTE = /\s*(?:\.\w+|\[.+?\])\s*/g;
> >  const RX_PSEUDO = /\s*:?:([\w-]+)(\(?\)?)\s*/g;
> > +const RX_BACKGROUND_IMAGE = /url\([\'\"]?(.*?)[\'\"]?\)/;
> 
> We don't need to escape the ' or the " do we?
> 
>   const RX_BACKGROUND_IMAGE = /url\(['"]?(.*?)['"]?\)/;
Done
> I'm unclear on CSS escaping rules. Do you know if this is valid?
> 
>   background-image: url("http://example.com/blah().png");
The example is valid CSS. Turns out our url parser (in output-parser I believe) fails to parse this (funny enough, Chrome has the exact same bug).

Comment 19

[Patrick Brosset <:pbro>]

Attachment: bug932896-part3-tooltip-image-size-pref v2.patch

Thanks Tim for the quick review. 
I've reverted the whitespace changes in firefox.js to avoid overriding blame info. You're right, my editor does this.
(In reply to Tim Taubert [:ttaubert] from comment #17)
> Comment on attachment 8366588 [details] [diff] [review]
> bug932896-part3-tooltip-image-size-pref.patch
> 
> Review of attachment 8366588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't mind having those white space fixes too much but I wonder whether
> you have some kind of editor setting that removes trailing white space
> automatically? It's usually best to keep the patch as small as possible and
> not override too much blame information.
> 
> ::: browser/app/profile/firefox.js
> @@ +1103,3 @@
> >  pref("devtools.inspector.show_pseudo_elements", true);
> > +// The default size for image preview tooltips in the rule-view/computed-view/markup-view
> > +pref("devtools.inspector.imagePreviewTooltipSize", 300);
> 
> I didn't read the bug to see if that was already considered but a static
> image size seems suboptimal. You could make it depend on the screen or
> window size but that might be hard to change for users then. I'll just leave
> that here as food for thought.
This hasn't been considered so far, that's an interesting point. 
The image is supposed to be displayed in a popup panel though and serves as a small image preview in the devtools' inspector, so don't think making it depend on the screen or window size would make a lot of sense. But it could make sense to not use the same size when debugging locally or remotely, as transferring image data-uri over a remote connection may be slow.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: bug932896-part2-reformat-file v2.patch

Just rebased this formatting patch.

Comment 21

[Patrick Brosset <:pbro>]

Attachment: bug932896-part1-image-tooltip-remote-target v2.patch

Before merging the patches together, here is a new part1 patch (the main one) up for review.
It contains the following changes as compared to the previous version:

- the getImageDataFromURL inspector method now uses a setTimeout to avoid queuing up server requests unnecessarily if the image can't be retrieved from the network.
- the background url parsing done in css-logic and output-parser is not handled with regex anymore and several test cases have been added to make sure all urls are parsed correctly.

Going to upload part2/3 patches too after rebase.

Comment 22

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #21)
> Created attachment 8369426 [details] [diff] [review]
> bug932896-part1-image-tooltip-remote-target v2.patch
> 
> Before merging the patches together, here is a new part1 patch (the main
> one) up for review.
> It contains the following changes as compared to the previous version:
> 
> - the getImageDataFromURL inspector method now uses a setTimeout to avoid
> queuing up server requests unnecessarily if the image can't be retrieved
> from the network.
> - the background url parsing done in css-logic and output-parser is not
> handled with regex anymore and several test cases have been added to make
> sure all urls are parsed correctly.
This patch also contains changes as per Joe's review in comment 14

Comment 23

[Patrick Brosset <:pbro>]

Attachment: bug932896-part2-reformat-file.patch

rebased

Comment 24

[Patrick Brosset <:pbro>]

Attachment: bug932896-part3-tooltip-image-size-pref.patch

rebased

Comment 25

[Patrick Brosset <:pbro>]

Attachment: bug932896-image-tooltip-remote-target.patch

This patch is a qfold of the part1/2/3 that have been attached and reviewed previously.
Ongoing try build for this new patch: https://tbpl.mozilla.org/?tree=Try&rev=8dfe579b48d6

Comment 26

[Patrick Brosset <:pbro>]

Try build is green apart from a few failures on linux debug that aren't new apparently.
I'm going to rebase the patch if needed and check it in.

Comment 27

[Patrick Brosset <:pbro>]

Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/18d70a3136f5

Comment 28

[Carsten Book [:Tomcat]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #27)
> Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/18d70a3136f5

backed out for bustage in mochitest-other like https://tbpl.mozilla.org/php/getParsedLog.php?id=34123702&tree=Fx-Team

Comment 29

[Patrick Brosset <:pbro>]

Fixed the failing m-oth test, re-run the whole suite, and pushed again: https://hg.mozilla.org/integration/fx-team/rev/d3841eb2ecc8

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d3841eb2ecc8

Comment 31

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I can confirm this works while remote debugging Thunderbird, I can see the image for a list-style-image rule that uses a chrome:// uri to an image in Thunderbird. Removing verifyme, I'm not sure if you need any other things set.