The default bug view has changed. See this FAQ.

Add --imgur flag to upload to imgur.com to the screenshot command

RESOLVED FIXED in Firefox 42

Status

()

Firefox
Developer Tools: Graphic Commandline and Toolbar
--
enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Christian Heilmann, Assigned: Johan K. Jensen, Mentored)

Tracking

unspecified
Firefox 42
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
imgur has a CORS interface, so you can upload via JavaScript and an XHR. Demo code is available in https://github.com/codepo8/interaction-cam/blob/master/interactioncam.js function upload().

Comment 1

3 years ago
Can I have this bug? I am a new here and trying to learn.
Sure!

File you want too change is /toolkit/devtools/gcli/commands/screenshot.js (http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/screenshot.js).
Speaking of this some thoughts:

1. Instead of specialising on --imgur, we could instead add --share[=imgur] or --upload[=imgur] flag which gives us some flexibility. E.g. allow us to add more services in the future or move away from imgur when it dies;
2. In case of upload failure you’ll want to save the image to the local disk so the screenshot is not lost.
(In reply to Simonas Kazlauskas [:simukis] from comment #3)
> Speaking of this some thoughts:
> 
> 1. Instead of specialising on --imgur, we could instead add --share[=imgur]
> or --upload[=imgur] flag which gives us some flexibility. E.g. allow us to
> add more services in the future or move away from imgur when it dies;
> 2. In case of upload failure you’ll want to save the image to the local disk
> so the screenshot is not lost.

We've currently got 2 'destinations' (or 3 depending on how you look at it)

* file (e.g. `screenshot thing.png`)
* clipboard (e.g. `screenshot --clipboard`)
* auto generated filename (e.g. `screenshot`)

They interact in strange ways. `screenshot thing.png --clipboard` just ignores thing.png.

We should either mandate a single destination, or allow multiple, and do it properly. The single destination route looks like:

>> clipboard --dest [file|clipboard|imgur|flickr|...] [filename]

Where `--dest file` is the default and we check that the filename isn't mixed with `--dest clipboard` etc.

Or we could allow any number of destinations:

>> clipboard [filename] [--clipboard] [--imgur] [--flickr] [etc]

I'm not sure that actually allowing multiple destinations is a significant selling feature. Marginally useful in some cases maybe, but what makes me like this option is:

* It's simpler. ``clipboard --imgur`` is obvious if all you know is that we support Imgur, where ``clipboard --dest imgur`` requires you to remember if it was --dest rather than --share or --upload
* It's backwards compatible with the way things work now
Assigning to Tina from Ascend Project who is preparing to put up a patch for this.
Assignee: nobody → agustina.hinojosa
Whiteboard: [good first bug][lang=js]
(Reporter)

Comment 6

3 years ago
Lukas, that makes me really happy! Because I want the feature but also to see this part of this cool programme.

Comment 7

3 years ago
Created attachment 8499051 [details] [diff] [review]
adds anonymous imgur upload support for screenshot function.

So what I've done so far is add functionality to screenshot.js's grabScreen function to allow anonymous upload to imgur. It currently works with existing --delay, --chrome, --fullpage, and --selector but not --clipboard (clipboard will override --imgur). 

I'm curious about imgur's API key. It is currently using one that I registered to test with, but for full deployment I suggest writing to imgur and asking to be whitelisted (https://api.imgur.com/whitelist).

Thanks for any help making this patch better :)
Attachment #8499051 - Flags: feedback?(jwalker)
Comment on attachment 8499051 [details] [diff] [review]
adds anonymous imgur upload support for screenshot function.

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

Thanks for the patch.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ -3,4 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!-- LOCALIZATION NOTE : FILE This file contains the browser main menu items -->
> -<!-- LOCALIZATION NOTE : FILE Do not translate commandkeys --> 

It looks like the changes to this file are all trailing whitespace removals. While we do try to remove trailing whitespace, we do it on related changes.

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +128,4 @@
>  # LOCALIZATION NOTE (screenshotTooltip) Text displayed as tooltip for screenshot button in devtools ToolBox.
>  screenshotTooltip=Take a fullpage screenshot
>  
> +screenshotImgurDesc=Upload to imgur.com

I know it's repetitive, but please could you add comments like the other properties. My understanding is that some localizers have tools that show them just the comment and the thing to be translated, so it helps to have comments for all properties.

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +220,5 @@
> +            fd.append("image", data.split(',')[1]);
> +            fd.append("type", "base64");
> +            fd.append("title", filename);
> +            xhr.open("POST", "https://api.imgur.com/3/image");
> +            xhr.setRequestHeader('Authorization', 'Client-ID 0df414e888d7240');

2 thoughts:

1. The URL and the Client-ID look like something that could change. I think we should probably store them in a pref. (See the use of Services.prefs.getCharPref in inject.js for an example)
2. Maybe we should talk this through with someone from imgur to make sure we're doing the right thing.

@@ +233,5 @@
> +              }
> +            }
> +          } catch(ex) {
> +            div.textContent = gcli.lookup("screenshotImgurError");
> +          } throw new Task.Result(div);

I think we should move the return statements to the end, and concatenate a string instead. For example:

  reply += gcli.lookup("screenshotImgurError");

We'd need to make sure it all makes sense when put together.
Attachment #8499051 - Flags: feedback?(jwalker)
Hey Christian, do you know anyone from Imgur that we could touch base with to make sure that what we're doing is going to be OK with them?
Flags: needinfo?(cheilmann)
(Reporter)

Comment 10

3 years ago
I do not - yet. I will reach out.
Flags: needinfo?(cheilmann)
(Reporter)

Comment 11

3 years ago
Reached out on various channels https://groups.google.com/forum/?fromgroups#!topic/imgur/AJcTXtiOMPk

Comment 12

3 years ago
Created attachment 8499753 [details] [diff] [review]
moves imgur url and clientid to firefox prefs

Ok, So I was able to move the imgur Client ID and the upload URL to a firefox pref. I also added Localization Notes where you wanted them. Hopefully I did that right.

I'm unsure what you mean by retry += [stuff]. Could you please clarify?
Attachment #8499051 - Attachment is obsolete: true

Comment 13

3 years ago
Is it possible to get technical review of the patch while we wait on getting a whitelisted imgur api key?
Flags: needinfo?(jwalker)
(In reply to agustina.hinojosa from comment #12)
> Created attachment 8499753 [details] [diff] [review]
> moves imgur url and clientid to firefox prefs
> 
> Ok, So I was able to move the imgur Client ID and the upload URL to a
> firefox pref. I also added Localization Notes where you wanted them.
> Hopefully I did that right.
> 
> I'm unsure what you mean by retry += [stuff]. Could you please clarify?

So there are 2 things that happen to the div:

* We add a screenshot iff we're saving to a file. Why is that case special?
* All the other cases are just text

And we're throwing/returning the div at several points making it hard to take several save actions.

So I suggest:

    var destinations = [];

    if (clipboard) {
      // save to clipboard
      destinations += gcli.lookup("screenshotSaveClipboard");
    }

    if (imgur) {
      // save to imgur
      destinations += gcli.lookup("screenshotSaveImgur");
    }

    if (...) {
      // save to ...
      destinations += ...;
    }

    let div = document.createElementNS("http://www.w3.org/1999/xhtml", "div");
    // Add the image to the div

    destinations.forEach(function(destination) {
      div.textContent += destination;
    });
Flags: needinfo?(jwalker)

Comment 15

3 years ago
Created attachment 8502061 [details] [diff] [review]
implements makeDiv function and reroutes Task.results so there is only one.

This patch moves things around, namely: adding a function makeDiv that handles making the final div that is shown to the user. The same function is used after any type of destination (imgur, file, clipboard). 

There is one problem that I'm hoping to get help with: When uploading to imgur, after showing 'screenshotImgurUploading' text, the div doesn't resize to fit the image made in makeDiv(). The image shows up, but the div just isn't big enough and has scroll bars. Is there a way to tell the div message box to disappear so it can simply be remade the right size?
Attachment #8502061 - Flags: feedback?(jwalker)

Comment 16

3 years ago
fwiw, I don't think we need to block on communicating with imgur to land this, our developer population doesn't register compared to the imgur traffic.  We can talk to imgur concurrently with landing this.

Comment 17

3 years ago
(maybe not, though, looking more closely at their requirements - sorry for the noise)
Comment on attachment 8502061 [details] [diff] [review]
implements makeDiv function and reroutes Task.results so there is only one.

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

Looks excellent. I think we need to check that we're doing the right thing by imgur, if we are ok then r+
Thanks.

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +126,5 @@
> +          destinations.forEach(function(destination) {
> +            div.textContent += destination;
> +          });
> +
> +          div.addEventListener("click", function openFile() {

Could you wrap the addEventListener and the cursor='pointer' in an `if (!clipboard)` so we don't make the user think you can click when you can't.

@@ +132,5 @@
> +            if (saveToFile){
> +              file.reveal();
> +            } else if (imgur) {
> +              var tab = context.environment.chromeWindow.open();
> +              tab.location.href = xhr.response.data.link;

Nice touch!

@@ +225,5 @@
>            }
>            catch (ex) {
>              div.textContent = gcli.lookup("screenshotErrorCopying");
>            }
> +        } //end clipboard

Style nit: I get the desire for these, but it's not what we do in the rest of our codebase.
Attachment #8502061 - Flags: feedback?(jwalker) → feedback+

Comment 19

3 years ago
Perhaps, to get this landed, we could land a version that doesn't include an API key yet while we work out the specifics of the API key with imgur?

Comment 20

3 years ago
Created attachment 8505731 [details] [diff] [review]
adds conditional to prevent a clipboard image from appearing clickable

I'm still having the same problem with the box holding the image after uploading to imgur being too small. Any advice to fix that?
Attachment #8499753 - Attachment is obsolete: true
Attachment #8502061 - Attachment is obsolete: true
Attachment #8505731 - Flags: feedback?(jwalker)
Comment on attachment 8505731 [details] [diff] [review]
adds conditional to prevent a clipboard image from appearing clickable

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

The changes you've made look good to me, I'll ask someone to help you with the image size problem.
Attachment #8505731 - Flags: feedback?(jwalker) → feedback+
Hey Optimizer, Agustina has added support for uploading to imgur and wanted a bit of help with the preview image size being wrong, do you have time to take a quick look?
Flags: needinfo?(scrapmachines)
(Not actually test yet but) It seems like an XUL bug that it does not think that it needs to recalculate layout. Doing a forced reflow should work.

Some code like

let a = <the containing node>.offsetWidth;
Flags: needinfo?(scrapmachines)

Comment 24

3 years ago
Created attachment 8507127 [details] [diff] [review]
Works around gcli prompt resize problem - removes image preview when using --imgur

This version of the patch removes the image preview when uploading to imgur in favor of just displaying the url (and letting it be clickable still) to the user.

Should I just go ahead and ask imgur to whitelist this application? I don't want to step on anyone's toes with this, but someone should fill out https://api.imgur.com/whitelist before the patch lands.
Sorry for the delay. I tried to reach Imgur over IRC a few weeks ago, but failed to get a response. I've just filled in the form you linked to, and we'll see what they say.
Depends on: 1173158
Joe, I assume Imgur has never replied to you?
Flags: needinfo?(jwalker)
From the commit log, a hidden version of Imgur support seems to have appeared as part of the giant migration in bug 1128988.

I was trying to track down when it was added, but I guess there was no separate commit for it.
I've spoken to imgur and they've whitelisted our key. I spoke to Jasdev Singh (@jasdev), so we're good as far as that's concerned.
Flags: needinfo?(jwalker)
(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #28)
> I've spoken to imgur and they've whitelisted our key. I spoke to Jasdev
> Singh (@jasdev), so we're good as far as that's concerned.

Okay great!  In that case, should we use this bug to un-hide the "--imgur" option for the screenshot command?  I think that's the only thing left to do.
Flags: needinfo?(jwalker)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> from comment #28)
> > I've spoken to imgur and they've whitelisted our key. I spoke to Jasdev
> > Singh (@jasdev), so we're good as far as that's concerned.
> 
> Okay great!  In that case, should we use this bug to un-hide the "--imgur"
> option for the screenshot command?  I think that's the only thing left to do.

Sounds good to me.
Flags: needinfo?(jwalker)
Johan, maybe you'd like to un-hide the option?
Assignee: agustina.hinojosa → nobody
Flags: needinfo?(jj)
Mentor: jryans@gmail.com
(Assignee)

Comment 32

2 years ago
Created attachment 8634936 [details] [diff] [review]
bug992386_change_hidden_property_of_imgur_to_true.patch

Let’s get this feature out there. :)

Seems to pass my try-test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=644276757a8a
Assignee: nobody → jj
Status: NEW → ASSIGNED
Flags: needinfo?(jj)
Attachment #8634936 - Flags: review?(jryans)
Comment on attachment 8634936 [details] [diff] [review]
bug992386_change_hidden_property_of_imgur_to_true.patch

Hmm, it looks like the patch is empty?  Not sure what happened exactly.
Attachment #8634936 - Flags: review?(jryans)
(Assignee)

Comment 34

2 years ago
Created attachment 8635147 [details] [diff] [review]
bug992386_change_hidden_property_of_imgur_to_true.patch

Let’s try again.
Attachment #8634936 - Attachment is obsolete: true
Attachment #8635147 - Flags: review?(jryans)
Comment on attachment 8635147 [details] [diff] [review]
bug992386_change_hidden_property_of_imgur_to_true.patch

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

You can carry over my r+ after making the change below.

Be sure to re-run try as well before marking checkin-needed, as the sheriffs will expect a try link in the bug.

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +54,5 @@
>        manual: l10n.lookup("screenshotClipboardManual")
>      },
>      {
>        name: "imgur",
> +      hidden: false,

I believe you can just remove the line entirely, since visible is the default state.
Attachment #8635147 - Flags: review?(jryans) → review+
(Assignee)

Comment 36

2 years ago
Created attachment 8635795 [details] [diff] [review]
bug992386_change_hidden_property_of_imgur_to_true_rev2.patch

Looks okay on the try-server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62186c0b283a
Attachment #8635147 - Attachment is obsolete: true
Attachment #8635795 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 37

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/5e27c99556cb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e27c99556cb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.