Add "Save Image As" in request list context menu

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
VERIFIED FIXED
4 years ago
5 months ago

People

(Reporter: vporof, Assigned: Vangelis Katsikaros, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-needed, good-first-bug})

unspecified
Firefox 55
dev-doc-needed, good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [netmonitor-reserve])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
This context menu should contain "Open in New Tab" and "Copy as Data URI" menu items.
(Reporter)

Updated

4 years ago
Depends on: 964015
(Reporter)

Comment 1

4 years ago
A "Save As" menu item would be useful as well!
Do we need this in the response panel? We have it on the list item.
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?
Flags: needinfo?(vporof)
Flags: needinfo?(jgriffiths)
(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.
Flags: needinfo?(jgriffiths)
Assignee: nobody → gioyik
(Reporter)

Updated

3 years ago
Flags: needinfo?(vporof)

Updated

9 months ago
Blocks: 1307743
Whiteboard: [netmonitor]

Updated

9 months ago
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]

Updated

9 months ago
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu

Updated

8 months ago
Blocks: 1348737
No longer blocks: 1307743

Comment 5

8 months ago
Resetting, 2 years without activity.
Assignee: gioyik → nobody
Status: ASSIGNED → NEW

Updated

8 months ago
Keywords: good-first-bug
(Assignee)

Comment 6

8 months ago
Created attachment 8856217 [details] [diff] [review]
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.
Attachment #8856217 - Flags: feedback+
(Assignee)

Comment 7

8 months ago
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

Updated

8 months ago
Attachment #8856217 - Flags: feedback+ → feedback?(rchien)
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
Assignee: nobody → vkatsikaros
Mentor: rchien@mozilla.com
Status: NEW → ASSIGNED
Attachment #8856217 - Flags: feedback?(rchien)
btw, please set me as reviewer.
(Assignee)

Comment 10

8 months ago
Created attachment 8856278 [details] [diff] [review]
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.
Attachment #8856217 - Attachment is obsolete: true
Attachment #8856278 - Flags: feedback?(rchien)
(Assignee)

Comment 11

8 months ago
Created attachment 8856279 [details]
demo.png

Demo of the menu item.
(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 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) {
Attachment #8856278 - Flags: feedback?(rchien) → feedback+
(Assignee)

Comment 14

8 months ago
Created attachment 8856367 [details] [diff] [review]
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
Attachment #8856278 - Attachment is obsolete: true
Attachment #8856367 - Flags: review?(rchien)
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 ===
Attachment #8856367 - Flags: review?(rchien) → review+
(Assignee)

Comment 16

8 months ago
Created attachment 8856389 [details] [diff] [review]
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)
Attachment #8856367 - Attachment is obsolete: true
Attachment #8856389 - Flags: review?(rchien)
(Assignee)

Comment 17

8 months ago
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 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.
Attachment #8856389 - Flags: review?(rchien) → review+
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 hidden (mozreview-request)

Comment 21

8 months ago
mozreview-review
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.
Attachment #8856407 - Flags: review?(rchien) → review+
Comment on attachment 8856389 [details] [diff] [review]
skeleton.patch

obsolete this patch since you've used mozreview.
Attachment #8856389 - Attachment is obsolete: true
(Assignee)

Comment 23

8 months ago
mozreview-review-reply
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

8 months ago
(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

8 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dd7433e1cae3
Add a context menu for images in the Response panel. r=rickychien
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 :)
Summary: Add a context menu for images in the Response panel → Add "Save Image As" in request list context menu
(Assignee)

Comment 27

8 months ago
Ricky thanks for the feedback and the guidance on mozreview and the workflow!

Comment 28

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd7433e1cae3
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

8 months ago
Iteration: --- → 55.3 - Apr 17
Priority: P3 → P1
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
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]
Status: VERIFIED → RESOLVED
Last Resolved: 8 months ago7 months ago
status-firefox55: verified → fixed

Updated

7 months ago
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified

Updated

7 months ago
Keywords: dev-doc-needed
Depends on: 1372737
You need to log in before you can comment on or make changes to this bug.