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

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, enhancement)

Description

[Christian Heilmann]

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

[Sushrut Bhalla]

Can I have this bug? I am a new here and trying to learn.

Comment 2

[Simonas Kazlauskas [:nagisa/:simukis]]

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).

Comment 3

[Simonas Kazlauskas [:nagisa/:simukis]]

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.

Comment 4

[Joe Walker [:jwalker]]

(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

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning to Tina from Ascend Project who is preparing to put up a patch for this.

Comment 6

[Christian Heilmann]

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

Comment 7

[agustina.hinojosa]

Attachment: 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 :)

Comment 8

[Joe Walker [: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.

Comment 9

[Joe Walker [: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?

Comment 10

[Christian Heilmann]

I do not - yet. I will reach out.

Comment 11

[Christian Heilmann]

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

Comment 12

[agustina.hinojosa]

Attachment: 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?

Comment 13

[agustina.hinojosa]

Is it possible to get technical review of the patch while we wait on getting a whitelisted imgur api key?

Comment 14

[Joe Walker [: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;
    });

Comment 15

[agustina.hinojosa]

Attachment: 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?

Comment 16

[Dave Camp (:dcamp)]

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

[Dave Camp (:dcamp)]

(maybe not, though, looking more closely at their requirements - sorry for the noise)

Comment 18

[Joe Walker [:jwalker]]

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.

Comment 19

[Dave Camp (:dcamp)]

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

[agustina.hinojosa]

Attachment: 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?

Comment 21

[Joe Walker [: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.

Comment 22

[Joe Walker [:jwalker]]

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?

Comment 23

[Girish Sharma [:Optimizer]]

(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;

Comment 24

[agustina.hinojosa]

Attachment: 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.

Comment 25

[Joe Walker [:jwalker]]

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.

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Joe, I assume Imgur has never replied to you?

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 28

[Joe Walker [:jwalker]]

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.

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 30

[Joe Walker [: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.

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Johan, maybe you'd like to un-hide the option?

Comment 32

[Johan K. Jensen :johankj]

Attachment: 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

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 34

[Johan K. Jensen :johankj]

Attachment: bug992386_change_hidden_property_of_imgur_to_true.patch

Let’s try again.

Comment 35

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 36

[Johan K. Jensen :johankj]

Attachment: 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

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5e27c99556cb

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e27c99556cb