964015 - Add "Copy as data URI" for images in the Network Monitor

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

This is useful.

Comment 1

[Patrick Brosset <:pbro>]

Bug 964014 aims at doing the same for the markup-view.
We should probably align the 2 bugs in terms of wording: "Copy Image as Data-URI" or something like this.

Also, I think there's an easy way to tackle this one. 
Since a couple of weeks, we have the inspector actor front at toolbox level, so available from anywhere.
And it turns out that, after bug 932220 lands, the inspector actor will have a new method that transforms an image URL into data-uri (it's called getDataUriFromURL or something like that).

That could be an easy way to fix this.

Comment 2

[Victor Porof [:vporof][:vp]]

Since we already have the image data as a string, we could convert that to a data URI directly without fiddling with the Inspector actor.

Comment 3

[Victor Porof [:vporof][:vp]]

This is an easy bug to fix. If anyone's interested, feel free. This involves just adding an item in a context menu and copying to clipboard (code for this is already available).

Comment 4

[Peiyong Lin[:lpy](UTC+8)]

I would like to take this bug, could you please point the direction for me? Which part of code should I start to read? Thank you :)

Comment 5

[Victor Porof [:vporof][:vp]]

Hi! Thanks for the interest.

This should get you started with building Firefox from source, and tell you a few things about running automated tests: [0] Let me know if you need any help with that.

In this bug, you'll need to add an item in the context menu here: [1] The strings there are localized, so add an entry here as well [2]. You'll have to make sure the menu item isn't shown for all requests, only for images; this should be done here [3]. Here's an example how to copy a string to clipboard: [4], and here's a place where we build a data uri for an image: [5] 

After implementing the new context menu item, you'll have to create an automated test to make sure this won't break in the future. Here's something you can use as inspiration: [6]. You can clone that test and modify it to suit your needs.

Let me know if you need any additional help!

[0] https://wiki.mozilla.org/DevTools/Hacking
[1] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor.xul#24
[2] http://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd?from=netmonitor.dtd#178
[3] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#1274
[4] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#419
[5] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#2051
[6] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/browser_net_copy_url.js

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug964015.patch

Attach the patch.
The problem is, when I tried to run the test under netmonitor, the browser just said that "XML Parsing Error: undefined entity" with <menuitem id="request-menu-context-copy-image-as-data-uri"

I could not figure out what was wrong. Could you please help me? Thank you

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

I found it. Thank you

Comment 8

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug964015-V2.patch

Hi. I write a test, and I set |RequestsMenu.selectedItem = requestItembut| and when I call |RequestsMenu.copyImageAsDataUri()|, I got "Full message: TypeError: this.selectedItem.attachment.responseContent is undefined". I run the test manually, and it did select the right one, which was an image.
I couldn't figure it out. Please help. Thank you.

Comment 9

[Peiyong Lin[:lpy](UTC+8)]

Correct: I mean |RequestsMenu.selectedItem = requestItem|

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8367848 [details] [diff] [review]
bug964015-V2.patch

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

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +431,5 @@
> +    let { mimeType, text, encoding } = this.selectedItem.attachment.responseContent.content;
> +    return gNetwork.getString(text).then(aString => {
> +      clipboardHelper.copyString("data:" + mimeType + ";" + encoding + "," + aString,
> +                                 document);
> +    }).then(() => window.emit(EVENTS.RESPONSE_BODY_DISPLAYED));

We don't need to emit any events here, and shouldn't return a promise.

Here's how I would write this:

copyImageAsDataUri: function() {
  let selected = this.selectedItem.attachment;
  let { mimeType, text, encoding } = attachment.responseContent.content;

  gNetwork.getString(text).then(aString => {
    let data = "data:" + mimeType + ";" + encoding + "," + aString;
    clipboardHelper.copyString(data, document);
  });
}

@@ +1292,5 @@
>      let copyUrlElement = $("#request-menu-context-copy-url");
>      copyUrlElement.hidden = !this.selectedItem;
>  
> +    let copyImageAsDataUriElement = $("#request-menu-context-copy-image-as-data-uri");
> +    copyImageAsDataUriElement.hidden = !this.selectedItem

You should have a couple more checks here. What if the response content isn't available yet when you right click, for example? Let's cache this.selectedItem in this entire function, and write this as

copyImageAsDataUriElement.hidden = !selectedItem ||
  !selectedItem.attachment.responseContent ||
  !selectedItem.attachment.responseContent.content.mimeType.contains("image/");

::: browser/devtools/netmonitor/test/browser_net_copy_image_as_data_uri.js
@@ +9,5 @@
> +  initNetMonitor(CONTENT_TYPE_URL).then(([aTab, aDebuggee, aMonitor]) => {
> +    info("Starting test... ");
> +
> +    let { NetMonitorView } = aMonitor.panelWin;
> +    let { RequestsMenu } = NetMonitorView;

You should add a RequestsMenu.lazyUpdate = false; here to avoid responseContent being undefined below.

@@ +27,5 @@
> +        cleanUp();
> +      });
> +    });
> +
> +    aDebuggee.performRequests(1);

No need to pass 1 here.

::: browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd
@@ +182,5 @@
>  <!ENTITY netmonitorUI.context.copyUrl.accesskey  "C">
>  
> +<!-- LOCALIZATION NOTE (netmonitorUI.context.copyImageAsDataUri): This is the label displayed
> +  -  on the context menu that copies the selected image as data uri -->
> +<!ENTITY netmonitorUI.context.copyImageAsDataUri "Copy Image As Data URI">

The proper capitalization is "Copy Image as Data URI". Neat tool: http://titlecapitalization.com/

Comment 11

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug964015-V3.patch

The test can pass when run it alone, but it will timeout when run together with other tests under netmonitor/test
When run together, the test couldn't fetch the image, and kept waiting, which caused timeout. Any help please? Thank you.

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 8368475 [details] [diff] [review]
bug964015-V3.patch

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

Sometimes caching does weird things. I'll push to try, and if things are green we'll land it.

Comment 13

[Victor Porof [:vporof][:vp]]

https://tbpl.mozilla.org/?tree=Try&rev=3343bd1f6a5e

Comment 14

[Victor Porof [:vporof][:vp]]

Ok, this also fails on try. We should look into it.

Comment 15

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug964015-V4.patch

Thank you for your guide.

Comment 16

[Victor Porof [:vporof][:vp]]

Comment on attachment 8368571 [details] [diff] [review]
bug964015-V4.patch

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

Do all tests pass now when run locally?

Comment 17

[Victor Porof [:vporof][:vp]]

New try run with updated patch: https://tbpl.mozilla.org/?tree=Try&rev=67658a9c7b74

Comment 18

[Victor Porof [:vporof][:vp]]

This looks like an unrelated orange. Pushing again just to be sure.

Comment 19

[Victor Porof [:vporof][:vp]]

https://tbpl.mozilla.org/?tree=Try&rev=50c356cd2e6e

Comment 20

[Peiyong Lin[:lpy](UTC+8)]

Yes, I am using OSX 10.9, all tests pass when run  locally.

Comment 21

[Victor Porof [:vporof][:vp]]

Everything looks good! Let's land this.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This has numerous conflicts with fx-team. Please rebase.

Comment 23

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug946015-V5.patch

update the patch

Comment 24

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/4e95343c93e6

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4e95343c93e6