1173158 - screenshot --imgur in gcli always shows error 'Could not reach imgur API' despite successful upload and response

Status: RESOLVED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox Nightly 41.0a1 20150608 and Aurora 40.0a2 20150605 on Windows 8.1

Bug 1128988 also landed the patch from bug 992386 (and didn't close it) which added support for uploading screenshots to imgur.com. Using |screenshot --imgur| always shows the error message 'Could not reach imgur API', but logging the request and response headers with the browser console shows that the server sends a successful upload response including the url with escaped slashes.

Comment 1

[Brian Grinstead [:bgrins]]

I remember we folded that into Bug 1128988 since that screenshot command needed to be completely rewritten.  This was intended to be a hidden option until we were ready to complete the feature, but now I notice that it shows up when running `help screenshot`.

I think hidden options should not be showing up in the help popup, but it appears they do.  I believe we needed to figure something out with regard to API keys and whitelisting on imgur to get Bug 992386 to land.  In the meantime we should probably just comment out the param entirely so it doesn't show up anywhere.

Comment 2

[Joe Walker [:jwalker]]

So 2 parts to the problem. The help command [1] seems to be not filtering out hidden options like it should. Probably line 312 is missing a check for param.hidden. (Parameter is defined at [2])

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/source/lib/gcli/commands/help.js
[2]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/source/lib/gcli/commands/commands.js#225

Also the screenshot [3] command is incorrectly reporting an error, probably it's not interpreting the response from imgur correctly.

[3]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/screenshot.js#385

Comment 3

[Johan K. Jensen :johankj]

Attachment: bug1173158_screenshot_imgur__johankj.patch

1) Rather than adding inline ifs all the necessary places I filtered out the hidden options as the first thing when asking for the help-menu.
2) The screenshot command used the `screenshotImgurError`-locale on both success and error. Using the `screenshotImgurUploaded` on success shows the URL. I also added clicking on the thumbnail to open a new window (perhaps a new tab?) with the imgur-url.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8632053 [details] [diff] [review]
bug1173158_screenshot_imgur__johankj.patch

Flagging for review

Comment 5

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

Comment on attachment 8632053 [details] [diff] [review]
bug1173158_screenshot_imgur__johankj.patch

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

Overall, this seems good, just a few tweaks!

For future bugs when you attach a first patch, it's best to ensure you pick some person on the team as a reviewer, instead of just setting the flag with no person.  It's possible your patch would be missed if no one is assigned to review it.  You don't have to get the person right, we can always redirect to the correct reviewer.

Please adjust your commit message[1].  The bug number is missing, and the message should end with "r=jryans".

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +137,4 @@
>          root.style.cursor = "pointer";
>          root.addEventListener("click", () => {
> +          if (imageSummary.href) {
> +            var tab = context.environment.chromeWindow.open();

I would suggest:

context.environment.chromeWindow.gBrowser.addTab(<URL>);

for this step.

@@ +138,5 @@
>          root.addEventListener("click", () => {
> +          if (imageSummary.href) {
> +            var tab = context.environment.chromeWindow.open();
> +            tab.location.href = imageSummary.href;
> +          } else  if (imageSummary.filename) {

Only one space between else and if.

Consider enabling ESLint in your editor to follow our coding standards[1].

[1]: https://wiki.mozilla.org/DevTools/CodingStandards

Comment 6

[Johan K. Jensen :johankj]

Attachment: bug1173158_screenshot_imgur_2__johankj.patch

Comment 7

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

Comment on attachment 8632175 [details] [diff] [review]
bug1173158_screenshot_imgur_2__johankj.patch

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

Great, this looks good to me.

Try is closed for the moment, but I'll push it to Try when it reopens.

For future bugs, you should consider applying for Level 1 commit access[1], so you can push to Try yourself.  I'll vouch you if you apply!

[1]: https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 8

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ddf636ebabb

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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