932937 - [inspector] Image tooltips should be resized down on the server before sending to frontend

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

The calls to getImageData can slow things down, since the request is made when the MarkupContainer is initialized and the string length can be quite long.

We should be limiting the size of the getImageData canvas to whatever the max size of the tooltip image is going to be to prevent sending a bigger string than necessary across the protocol (it gets resized on the frontend anyway).

You can see this become an issue if you inspect a large image like this: http://photojournal.jpl.nasa.gov/jpeg/PIA17550.jpg.  It is a little slow to load the markup container, and when you hover over the 'src' the image won't load for a few seconds.  If we resized it on the server to max size of 400x400 it would help (could be customizable just as maxDim on setImageContent for the tooltip is).

One issue this would cause is the label on the tooltip is showing the width and height of the dataURL being passed in.  We could either pass in the original dimensions, or somehow return them along with the data URL from the server.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug932937-getimagedata-server-resize

This patch brins the following changes:
- The getImageData actor method now resizes the image before sending the data on the wire
- On top of sending the image data, it also now sends information about the size of the image
- There's now only one tooltip instance for the whole markup-view, there were one per MarkupContainer (a line) before.
- I've added a mochitest-chrome test for the getImageData method.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug932937-getimagedata-server-resize.patch

Re-uploading the same patch, with the right content type this time.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug932937-getimagedata-server-resize.patch

And with a last minute correction ...

Comment 4

[Patrick Brosset <:pbro>]

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=6bd3aa2e1dc2

Comment 5

[Heather Arthur [:harth]]

Comment on attachment 826765 [details] [diff] [review]
bug932937-getimagedata-server-resize.patch

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

Nice Catch Brian. Looks good Patrick.

::: browser/devtools/markupview/markup-view.js
@@ +1157,5 @@
> +            this.tooltipData.size = data.size;
> +            data.data.string().then(str => {
> +              this.tooltipData.data = str
> +              def.resolve();
> +              delete this.tooltipData.dataPromise;

There's a slightly cleaner pattern for this that I've seen around our code. Have tooltipData.data always be a promise. First you set it to `def.promise` and when the data becomes available set it like so:

this.tooltipData.data = resolve(str);

This means you don't have to have both 'data' and 'dataPromise', and you don't have to case for both.

@@ +1168,4 @@
>  
> +  _buildTooltipContent: function(target, tooltip) {
> +    if (this.tooltipData && target === this.tooltipData.target) {
> +      if (this.tooltipData.dataPromise) {

See above on how you can get rid of this conditional.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +275,2 @@
>     */
> +  setImageContent: function(imageUrl, options) {

options={} default param?

I'm suspecting that an incompatible server could lead to the options argument being undefined.

::: toolkit/devtools/server/actors/inspector.js
@@ +315,2 @@
>      response: {
> +      data: RetVal("imageData")

add 'size' entry here?

Comment 6

[Heather Arthur [:harth]]

(In reply to Patrick Brosset from comment #4)
> Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=6bd3aa2e1dc2

Just verifying, are the tests you added Browser Chrome (bc) tests? I only ask because I haven't worked with these kinds of mochitests in toolkit and I know there are different kinds of mochitests.

Comment 7

[Patrick Brosset <:pbro>]

Thanks Heather for the review!

> ::: browser/devtools/markupview/markup-view.js
> @@ +1157,5 @@
> > +            this.tooltipData.size = data.size;
> > +            data.data.string().then(str => {
> > +              this.tooltipData.data = str
> > +              def.resolve();
> > +              delete this.tooltipData.dataPromise;
> 
> There's a slightly cleaner pattern for this that I've seen around our code.
> Have tooltipData.data always be a promise. First you set it to `def.promise`
> and when the data becomes available set it like so:
> 
> this.tooltipData.data = resolve(str);
> 
> This means you don't have to have both 'data' and 'dataPromise', and you
> don't have to case for both.

Wow! Awesome pattern indeed, that simplifies the code a whole bunch.

> Just verifying, are the tests you added Browser Chrome (bc) tests? I only
> ask because I haven't worked with these kinds of mochitests in toolkit and I
> know there are different kinds of mochitests.

Yes I added one chrome mochitest. It is indeed quite different from the browser mochitest. For a start, the test is written in an HTML document where test dependencies are loaded via script tags. When it comes to inspector chrome mochitest tests, they are even more different in the sense that they use an inspector-test-helper that provides some sort of a test framework.

Comment 8

[Patrick Brosset <:pbro>]

New patch created with corrections from review. Ongoing try build:
https://tbpl.mozilla.org/?tree=Try&rev=9ba553df6ad6

Comment 9

[Patrick Brosset <:pbro>]

Attachment: bug932937-getimagedata-server-resize.patch

Comment 10

[Patrick Brosset <:pbro>]

Last try build green.
R+
Ready for check-in!

Comment 11

[Heather Arthur [:harth]]

(In reply to Patrick Brosset from comment #7) 
> Yes I added one chrome mochitest. It is indeed quite different from the
> browser mochitest.

I ask because it looks we only ran the browser chrome (bc) tests in the try run. Can you verify that this will also run your new chrome test? From the wiki I'm checking out (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing#Mochitest-other_(Moth)) it looks like it might be in mochitest-other.

Comment 12

[Heather Arthur [:harth]]

If I'm mistaken, I apologize, and add "checkin-needed" back.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I actually pushed this awhile ago but forgot to mark it as such. Fingers crossed :P
https://hg.mozilla.org/integration/fx-team/rev/9ce4de7db650

Comment 14

[Patrick Brosset <:pbro>]

Good point Heather! Sorry I though mochitest-bc meant that it would run both mochitest-browser and mochitest-chrome tests.

Comment 15

[Patrick Brosset <:pbro>]

So here is a try build to run mochitest-other: https://tbpl.mozilla.org/?tree=Try&rev=a13e727e5d5e

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Patrick Brosset <:pbro>]

The mochitest-other try build is green.