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

RESOLVED FIXED in Firefox 28

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: bgrins, Assigned: pbro)

Tracking

unspecified
Firefox 28
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

6 years ago
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.
Assignee: nobody → pbrosset
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.
Attachment #826759 - Flags: review?(fayearthur)
Re-uploading the same patch, with the right content type this time.
Attachment #826759 - Attachment is obsolete: true
Attachment #826759 - Flags: review?(fayearthur)
Attachment #826760 - Flags: review?(fayearthur)
And with a last minute correction ...
Attachment #826760 - Attachment is obsolete: true
Attachment #826760 - Flags: review?(fayearthur)
Attachment #826765 - Flags: review?(fayearthur)
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?
Attachment #826765 - Flags: review?(fayearthur) → review+
(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.
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.
New patch created with corrections from review. Ongoing try build:
https://tbpl.mozilla.org/?tree=Try&rev=9ba553df6ad6
Last try build green.
R+
Ready for check-in!
Keywords: checkin-needed
(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.
If I'm mistaken, I apologize, and add "checkin-needed" back.
Keywords: checkin-needed
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
Whiteboard: [fixed-in-fx-team]
Good point Heather! Sorry I though mochitest-bc meant that it would run both mochitest-browser and mochitest-chrome tests.
So here is a try build to run mochitest-other: https://tbpl.mozilla.org/?tree=Try&rev=a13e727e5d5e
https://hg.mozilla.org/mozilla-central/rev/9ce4de7db650
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
The mochitest-other try build is green.

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.