966209 - Add "Save Image As" in request list context menu

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

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

This context menu should contain "Open in New Tab" and "Copy as Data URI" menu items.

Comment 1

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

A "Save As" menu item would be useful as well!

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Do we need this in the response panel? We have it on the list item.

Comment 3

[Giovanny Gongora [:gioyik]]

The context menu has "Open in New Tab" and "Copy as Data URI" menu items. The unique to be implemented is "Save As". This could be a good "mentored bug" if you think "Save as" should be implemented. What you think?

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Giovanny Gongora [:gioyik] from comment #3)
> The context menu has "Open in New Tab" and "Copy as Data URI" menu items.
> The unique to be implemented is "Save As". This could be a good "mentored
> bug" if you think "Save as" should be implemented. What you think?

Yeah, agreed then.

Comment 5

[Tim Nguyen :ntim]

Resetting, 2 years without activity.

Comment 6

[Vangelis Katsikaros]

Attachment: skeleton.patch

A skeleton patch that takes the image data and saves it on disk. I have validated (with hexdump) that the correct binary data is saved.

I would like some hints/assistance with things I ignored in this patch:
* the path of the file is hardcoded ("/home/lala")
* the filename is hardcoded ("lala.png")
* the extension is hardcoded ("lala.png")

Happy to continue the work to complete this ticket.

Comment 7

[Vangelis Katsikaros]

adding to https://bugzilla.mozilla.org/show_bug.cgi?id=966209#c6 to the list of things I ignored in the patch and for which feedback is welcome:
* coding style
* tests

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Hi Vangelis Katsikaros,

Thanks your work! I can help you about this part so please ask any questions here :)

Some comments relate to your patch:

We're trying to use Firefox privilege APIs and instead, embrace web standard APIs if possible. File saving can be replaced by file-saver.js [1] and here is an example to show you how we implement "Save All As HAR" [2].

file-saver will ask user to save the file in default download path which is defined in user profile, so we don't have to worry about the path and import const {Cu} = require("chrome"); things in our codebase. That way looks more elegant, isn't? :)

[1] http://searchfox.org/mozilla-central/source/devtools/client/shared/file-saver.js
[2] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/har/har-exporter.js#65-89

Comment 9

[Ricky Chien [:rickychien] (inactive)]

btw, please set me as reviewer.

Comment 10

[Vangelis Katsikaros]

Attachment: skeleton.patch

Ricky, thanks for the detailed feedback!

I replaced the OS.File with file-saver.js as you suggested. From the "Save As" dialog, the image can be downloaded and opened with a program. I checked this
* with hexdump, ie the correct bytes are saved on disk
* and via the "open with", ie an image viewer can show the image properly

I am happy to address any further comments. Again, I ignored testing for this feature.

Some issues I spotted:

1) In my "Browser Console" I see, despite everything being saved or opened fine,  the following:

[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource:///modules/DownloadsCommon.jsm :: onDownloadChanged :: line 800"  data: no]  (unknown)
	onDownloadChanged resource:///modules/DownloadsCommon.jsm:800:13
	_notifyAllViews resource://gre/modules/DownloadList.jsm:208:11
	DL_change resource://gre/modules/DownloadList.jsm:131:5
	bound DL_change self-hosted:962:17
	D_notifyChange resource://gre/modules/DownloadCore.jsm:310:9
	task_D_start resource://gre/modules/DownloadCore.jsm:553:11
	InterpretGeneratorResume self-hosted:1219:8
	next self-hosted:1126:9
	TaskImpl_run resource://gre/modules/Task.jsm:319:42
	bound TaskImpl_run self-hosted:964:17
	process resource://gre/modules/Promise-backend.js:922:23
	walkerLoop resource://gre/modules/Promise-backend.js:806:7
	bound walkerLoop self-hosted:920:17
	bound bound walkerLoop self-hosted:920:17


2) I couldn't eslint the file due to a node version constraint (that apparently didn't notice when I did my initial "python bootstrap.py")

mozilla-central$ /usr/bin/node --version
v4.4.2
mozilla-central$ ./mach eslint --setup
nodejs v6.9.1 is either not installed or is installed to a non-standard path.
Please install nodejs from https://nodejs.org and try again.

Valid installation paths:
  - /usr/bin/node
A failure occured in the eslint linter.
✖ 1 problem (0 errors, 0 warnings, 1 failure)

Not sure if I should just go ahead and install node myself.

Comment 11

[Vangelis Katsikaros]

Attachment: demo.png

Demo of the menu item.

Comment 12

[Ricky Chien [:rickychien] (inactive)]

(In reply to Vangelis Katsikaros from comment #10)
> 1) In my "Browser Console" I see, despite everything being saved or opened
> fine,  the following:
> 
> [Exception... "Component returned failure code: 0x80070057
> (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]" 
> nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
> resource:///modules/DownloadsCommon.jsm :: onDownloadChanged :: line 800" 
> data: no]  (unknown)
> 	onDownloadChanged resource:///modules/DownloadsCommon.jsm:800:13
> 	_notifyAllViews resource://gre/modules/DownloadList.jsm:208:11
> 	DL_change resource://gre/modules/DownloadList.jsm:131:5
> 	bound DL_change self-hosted:962:17
> 	D_notifyChange resource://gre/modules/DownloadCore.jsm:310:9
> 	task_D_start resource://gre/modules/DownloadCore.jsm:553:11
> 	InterpretGeneratorResume self-hosted:1219:8
> 	next self-hosted:1126:9
> 	TaskImpl_run resource://gre/modules/Task.jsm:319:42
> 	bound TaskImpl_run self-hosted:964:17
> 	process resource://gre/modules/Promise-backend.js:922:23
> 	walkerLoop resource://gre/modules/Promise-backend.js:806:7
> 	bound walkerLoop self-hosted:920:17
> 	bound bound walkerLoop self-hosted:920:17

Yep, good catch! It seems like a error message threw by our download manager but it's harmless. I also see this error message when we perform "Save All As HAR". Let's ignore this.

> 2) I couldn't eslint the file due to a node version constraint (that
> apparently didn't notice when I did my initial "python bootstrap.py")
> 
> mozilla-central$ /usr/bin/node --version
> v4.4.2
> mozilla-central$ ./mach eslint --setup
> nodejs v6.9.1 is either not installed or is installed to a non-standard path.
> Please install nodejs from https://nodejs.org and try again.
> 
> Valid installation paths:
>   - /usr/bin/node
> A failure occured in the eslint linter.
> ✖ 1 problem (0 errors, 0 warnings, 1 failure)
> 
> Not sure if I should just go ahead and install node myself.

Hmm. our eslint setup requires node v6.9.1 but you still in v4.4.2. Could you try to upgrade your node or install nvm / n (https://github.com/tj/n) for managing multiple node version?

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8856278 [details] [diff] [review]
skeleton.patch

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

::: devtools/client/netmonitor/src/request-list-context-menu.js
@@ +13,4 @@
>  const { HarExporter } = require("./har/har-exporter");
>  const { getLongString } = require("./utils/client");
>  const { L10N } = require("./utils/l10n");
> +const base64 = require("sdk/base64");

base64 is useless, please remove.

@@ +13,5 @@
>  const { HarExporter } = require("./har/har-exporter");
>  const { getLongString } = require("./utils/client");
>  const { L10N } = require("./utils/l10n");
> +const base64 = require("sdk/base64");
> +const FileSaver = require("devtools/client/shared/file-saver");

nit: move FileSaver to between MenuItem and clipboardHelper

const MenuItem = require("devtools/client/framework/menu-item");
const FileSaver = require("devtools/client/shared/file-saver");
const clipboardHelper = require("devtools/shared/platform/clipboard");

@@ +164,5 @@
> +               selectedRequest.responseContent &&
> +               selectedRequest.responseContent.content.mimeType.includes("image/")),
> +      click: () => this.saveImageAs(),
> +    }));
> +

nit: remove this additional empty line.

@@ +338,5 @@
>    /**
> +   * Save image as.
> +   */
> +  saveImageAs() {
> +    const { mimeType, text, encoding } = this.selectedRequest.responseContent.content;

nit: mineType and encoding are not used. Please remove them. and assign text to decoded like:

let decoded = atob(this.selectedRequest.responseContent.content.text);

@@ +343,5 @@
> +    let decoded = atob(text);
> +    let data = new Uint8Array(decoded.length);
> +    for (var i = 0, e = decoded.length; i < e; ++i) {
> +      data[i] = decoded.charCodeAt(i);
> +    }

nit: 
After setting up eslint properly, we can see a suggestion about using let instead of var.
And `e` is not highly readable, so we can get rid of it by

for (let i = 0; i < decoded.length; ++i) {

Comment 14

[Vangelis Katsikaros]

Attachment: skeleton.patch

Ricky, thanks for the node hint and the feedback! I fixed all your suggestions.

However I spotted an additional issue, sorry for the back and forth :( If the response is "Content-Type image/svg+xml" then as expected, atob() can fail with "InvalidCharacterError: String contains an invalid character". So, I used the encoding, to either decode the base64, or use the response's content text as is. Let me know what you think.

PS 1
mozilla-central$ ./mach eslint devtools/client/netmonitor/src/request-list-context-menu.js
✖ 0 problems (0 errors, 0 warnings)

PS 2 One additional question, so that I don't repeat this again in the future. What is the motive behind:
> nit: move FileSaver to between MenuItem and clipboardHelper

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8856367 [details] [diff] [review]
skeleton.patch

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

Nice job! You fix a potential issue for image/svg+xml! So far so good, I like your patch.

> nit: move FileSaver to between MenuItem and clipboardHelper

We have some loose coding style convention when requiring module. Module path name will order by alphabetical order

const { Curl } = require("devtools/client/shared/curl"); // shared script beyond netmonitor folder
const { gDevTools } = require("devtools/client/framework/devtools");
const Menu = require("devtools/client/framework/menu");
const MenuItem = require("devtools/client/framework/menu-item");    // require order will arrange by path's alphabetical order
...
const { HarExporter } = require("./har/har-exporter"); // scripts inside netmonitor folder
...

// Components
const SharedComponent_1 = require("devtools/client/shared/components/component_1"); // shared component
const Component_2 = require("./components/component_2"); // netmonitor's component

const { div, span } = DOM; // React.DOM alias
const CONSTANT = "123"; // constant definition

::: devtools/client/netmonitor/src/request-list-context-menu.js
@@ +339,5 @@
> +  saveImageAs() {
> +    const { encoding, text } = this.selectedRequest.responseContent.content;
> +    let fileName = this.selectedRequest.urlDetails.baseNameWithQuery;
> +    let data;
> +    if (encoding == "base64") {

nit: use ===

Comment 16

[Vangelis Katsikaros]

Attachment: skeleton.patch

Ricky, thanks for the prompt review and detailed explanations.

Since we're at it, a question to improve my JS skills:
>nit: use ===
The "===" operator is needed as it doesn't do any conversions, like the "==" operator can perform?

PS
mozilla-central$ ./mach eslint devtools/client/netmonitor/src/components/request-list-item.js
✖ 0 problems (0 errors, 0 warnings)

Comment 17

[Vangelis Katsikaros]

PS I've done in the past the process of "commit access level 1" although I've never used it. Let me know if it's less hassle if I do this (TBH I am not sure if I miss something in my setup or credentials).

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8856389 [details] [diff] [review]
skeleton.patch

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

Good question:) In most of the case, they are both equivalent. If we can make sure `encoding` is a type of string, using === would bring more performance improvement since it will not cause additional type conversion in JS engine.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Cool! Once you got "Commit access level 1", you could try to setup mozreview (http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html) which is a new powerful review tool. It also bring you a convenient Web UI to submit a try push.

Comment 20

[Vangelis Katsikaros]

Attachment: Bug 966209 - Add a context menu for images in the Response panel.

Review commit: https://reviewboard.mozilla.org/r/128358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128358/

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8856407 [details]
Bug 966209 - Add a context menu for images in the Response panel.

https://reviewboard.mozilla.org/r/128358/#review130846

LGTM. please trigger a try push to make sure it's all green. I recommend "try: -b o -p linux64,macosx64,win32 -u mochitest-clipboard,mochitest-dt,mochitest-e10s-devtools-chrome -t none --artifact" for verifying all devtools related chagnes.

As soon as your try result is green, you can add checkin-needed in bugzilla keyword filed to ask an autoland push.

Comment 22

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8856389 [details] [diff] [review]
skeleton.patch

obsolete this patch since you've used mozreview.

Comment 23

[Vangelis Katsikaros]

Comment on attachment 8856407 [details]
Bug 966209 - Add a context menu for images in the Response panel.

https://reviewboard.mozilla.org/r/128358/#review130846

Ricky, I triggered a push try as you said. It now is on 100% but I see that some failed. In treeherder I don't see any "red" ones, but there is a "blue" one. I am not sure if I can/must click on it and then "Repeat the selected job"

Comment 24

[Tim Nguyen :ntim]

(In reply to Vangelis Katsikaros from comment #23)
> Comment on attachment 8856407 [details]
> Bug 966209 - Add a context menu for images in the Response panel.
> 
> https://reviewboard.mozilla.org/r/128358/#review130846
> 
> Ricky, I triggered a push try as you said. It now is on 100% but I see that
> some failed. In treeherder I don't see any "red" ones, but there is a "blue"
> one. I am not sure if I can/must click on it and then "Repeat the selected
> job"

The try push is fine.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e1d8afeade42f4e1f9e5cec7efd7103d876cae2

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd7433e1cae3
Add a context menu for images in the Response panel. r=rickychien

Comment 26

[Ricky Chien [:rickychien] (inactive)]

Bug title changed for reflecting the implementation. This bug was reported 3 years ago and at that time might not support "Open in new tab" in request list context menu.

Current implementation has offered various ways to download image like "Open in new tab" and then download image in new tab or using this "Save Image As" to achieve the same effect. I think both are very convenient for users, so I'd suggest to not introduce new context menu for response panel in the time.

Vangelis Katsikaros, thanks for your help! Your patch has approved and it's in landing process (comment 25). Once it passed all tests in autoland, you will see this new feature appears in latest Firefox Nightly :)

Comment 27

[Vangelis Katsikaros]

Ricky thanks for the feedback and the guidance on mozreview and the workflow!

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Emil Ghitta, QA [:emilghitta]]

I’ve reproduced this issue using Firefox 55.0a1 (Build Id:20170408030204) and verified that the “Save Image As” option is available in the context menu for the Network request list using Firefox 55.0a1 (Build Id:20170411030208) on Windows 10 64bit, Mac OS 10.11.6 and Ubuntu 16.04 64bit.

Comment 30

[Saheda Reza [:Antora]]

I reproduced this issue on Firefox nightly according to (2014-01-31)

Fixing bug is verified on Latest Nightly-- Build ID: (20170411030208), User Agent: Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0

Tested OS-- Windows10 32bit

[bugday-20170405]