Closed Bug 1173158 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set
normal

Tracking

(firefox41 affected, firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: aryx, Assigned: jj)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
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
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.
Attachment #8632053 - Flags: review+
Comment on attachment 8632053 [details] [diff] [review]
bug1173158_screenshot_imgur__johankj.patch

Flagging for review
Attachment #8632053 - Flags: review+ → review?(jryans)
Assignee: nobody → jj
Status: NEW → ASSIGNED
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
Attachment #8632053 - Flags: review?(jryans) → feedback+
Attachment #8632053 - Attachment is obsolete: true
Attachment #8632175 - Flags: review?(jryans)
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/
Attachment #8632175 - Flags: review?(jryans) → review+
https://hg.mozilla.org/mozilla-central/rev/a3e70f46393d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.