Power users of the GCLI screenshot command will not be satisfied with a screenshot button. As per Eric Meyer here https://bugzilla.mozilla.org/show_bug.cgi?id=1429421#c27 They want to do things like `screenshot --fullpage --dpr 0.5 cnn-no-css` to grab a low-resolution copy of a very long page. `screenshot --selector '#d0_u1_m' --dpr 3 modal-dialog` to capture a high-resolution example of a piece of a web page. And often, I do just `screenshot --fullpage` in a 1080p-size responsive-layout window to capture a rendering for a slide deck. and frequently use up arrow etc so there's a need for repeatable easy access to the last command. We can implement the command in the console panel.

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1429421#c29 - I expect this would be done with `WebConsoleCommands._registerOriginal` similar to `copy`: https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole/utils.js#576.

I have to admit a little bit of side-eye at the idea of JSifiying a utility command a la `screenshot({ fullpage: true, dpr: 0.5, name: 'cnn-no-css' });` but I can live with it. Does this mean we could create CORS-compliant pages that fired off screenshots when loaded? (I’m half-joking on that.)

I also prefer the CLI syntax, and in the past we've discussed exposing that from the toolbox somehow: 1) Console input commands prefixed with `:`. We do have flexibility on what strings get turned into commands, so a line like `:screenshot --fullpage --dpr 0.5 cnn-no-css` could be made to work. We'd at least have to redo / port the argument parsing and error handling. 2) A separate command palette in the toolbox accessible via keyboard shortcut, similar to what some text editors have. This could actually be really nice to have anyway. We'd probably end up reimplementing a fair amount of what GCLI does to do either of those well. I think that JSifying the command is the easiest way to get the feature back, but would be interested to hear if someone has time to work on something more.

(In reply to Eric A. Meyer from comment #2) > Does this mean we could create CORS-compliant pages > that fired off screenshots when loaded? (I’m half-joking on that.) Heh, no, at least not how we current implement web console commands :). They get passed in as the `bindings` arg to the debugger API executeInGlobalWithBindings call, which means they never actually get exposed to content: https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Object.

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review255122 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/actors/webconsole/utils.js:618 (Diff revision 1) > + * @return void > + */ > +WebConsoleCommands._registerOriginal("screenshotOptions", function(owner, value) { > + return getScreenshotCommandParams; > +}); > + Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]

Hey folks, I pushed an early version of this to get feedback in case I am missing something. I am having a little trouble getting the localization to work -> since moving the translations into screenshot.properties, the console is now blank. I am not sure what I am doing wrong, but if the translations are left in gclicommands.properties everything works as expected. I will find out what is happening soon, I might not be building correctly for example. I introduced two new commands -> screenshot and screenshotOptions. Screenshot works exactly as you would expect it, using the object syntax mentioned by bgrins, for example: `screenshot({ fullpage: true, dpr: 0.5, name: 'cnn-no-css' });` -- this is just for now to make sure everything is working. In the next round I will move towards the `:screenshot --fullpage --dpr 0.5 --name cnn-no-css` What still needs to be done before the command line style syntax are the tests, and I will work on those tomorrow. I will also take care of that lint message, surprised I didn't notice it earlier! Let me know if you find anything else amiss!

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review255156 ::: devtools/server/actors/webconsole/screenshot.js:83 (Diff revision 1) > + }, > + { > + name: "selector", > + type: "node", > + defaultValue: null, > + description: L10N.getStr("inspectNodeDesc"), You are having an issue with L10N because screenshot.properties doesn't define this key. ::: devtools/server/actors/webconsole/utils.js:9 (Diff revision 1) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > const {Ci, Cu} = require("chrome"); > +const {screenshot, getScreenshotCommandParams} = require("devtools/server/actors/webconsole/screenshot.js"); You do not need ".js" suffix for importing modules. Also, here you could use relative paths: require("./webconsole/screenshot"); ::: devtools/shared/gcli/commands/screenshot.js:1 (Diff revision 1) > /* This Source Code Form is subject to the terms of the Mozilla Public I imagine it is just for testing something that you modified this, right? Because you should not end up modifying it and instead only fork it like you did in server folder. (same comment for gclicommands.properties)

Sorry to be thick, but is this actually working in Nightly as yet? If so, how/where do we test it?

Not yet! still needs the tests in (and I noticed that the file functionality from the original code is broken in some cases) but it should be done soon :)

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review256268 ::: devtools/server/actors/webconsole/screenshot.js:1 (Diff revision 2) > +"use strict"; If I'm not wrong, this file is copied from devtools/shared/gcli/commands/screenshot.js right ? Could we use `hg copy` so we preserve the commit history ? Also, it will make it easier to see what was changed to make it work in the console. ::: devtools/server/actors/webconsole/utils.js:599 (Diff revision 2) > > /** > + * Take a screenshot of a page. > + * > + * @param any value > + * A value you want to copy as a string. I feel like this comment is erroneous ? ::: devtools/server/actors/webconsole/utils.js:611 (Diff revision 2) > + > +/** > + * List options for screenshot > + * > + * @param any value > + * A value you want to copy as a string. I feel like this comment is erroneous ? ::: devtools/server/actors/webconsole/utils.js:614 (Diff revision 2) > + * > + * @param any value > + * A value you want to copy as a string. > + * @return void > + */ > +WebConsoleCommands._registerOriginal("screenshotOptions", function(owner, value) { nit: could this be named listScreenshotOptions, or getScreenshotOptions so it's more obvious what it does ? ::: devtools/server/actors/webconsole/utils.js:614 (Diff revision 2) > + * > + * @param any value > + * A value you want to copy as a string. > + * @return void > + */ > +WebConsoleCommands._registerOriginal("screenshotOptions", function(owner, value) { owner and value don't seem to be used

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review256278 I'm concern of the case where screenshot is a content defined function but :screenshot should work. Let's see if we can handle this here or do it in a follow up ::: devtools/server/actors/webconsole.js:54 (Diff revision 2) > function isObject(value) { > return Object(value) === value; > } > > +function isCommand(string) { > + return /^:/.test(string); maybe we should check that there is at least one character after the colon ? ::: devtools/server/actors/webconsole.js:1305 (Diff revision 2) > + if (isCommand(string)) { > + string = formatCommand(string); > + } so here, I think we could have a problem if the content already defines a `screenshot` function. If this is the case, I expect `screenshot()` to call the content function, but `:screenshot` should run the command code. I'm not sure how we should handle this, but maybe we could actually call the screenshot code on the actor without relying on the js `screenshot()` helper ? ::: devtools/server/actors/webconsole/commands.js:21 (Diff revision 2) > + * > + * @param String string > + * A string to format that begins with `:`. > + */ > +function formatCommand(string) { > + if (!/^:/.test(string)) { could we re-use `isCommand` ? ::: devtools/server/actors/webconsole/commands.js:39 (Diff revision 2) > + * Output: a string formatted as ` { key: value, ... } ` or an empty string > + * > + * @param Object tree > + * A tree object produced by parseCommand > + */ > +function formatArgs(tree) { nit: instead of relying on the sahpe of `tree`, could formatArgs take `args` and `command` parameters ? ::: devtools/server/actors/webconsole/commands.js:40 (Diff revision 2) > + const args = Object.keys(tree.args).map(name => { > + const values = tree.args[name]; Since we are retrieving the values, could we use Object.entries so we get both the name and the value ? ::: devtools/server/actors/webconsole/commands.js:40 (Diff revision 2) > + const args = Object.keys(tree.args).map(name => { > + const values = tree.args[name]; > + // default value > + let value = "true"; > + if (values.length > 1) { > + value = `[${values.join(", ")}]`; > + } > + if (values.length === 1) { > + value = `${values[0]}`; > + } > + name = tree.command === "screenshot" && name === "default" ? "filename" : name; > + return `${name}: ${value}`; > + }); > + > + return args.length > + ? `{ ${args.join(", ")} }` > + : ""; instead of manipulating strings here, do you think we could build a plain js object and return JSON.stringify of it ? ::: devtools/server/actors/webconsole/commands.js:70 (Diff revision 2) > + * > + * @param String string > + * A string to use as the basis for the token > + */ > +function createToken(string) { > + if (/^:/.test(string)) { could we reuse `isCommand` here ? ::: devtools/server/actors/webconsole/commands.js:71 (Diff revision 2) > + * @param String string > + * A string to use as the basis for the token > + */ > +function createToken(string) { > + if (/^:/.test(string)) { > + const value = string.substr(1); nit: could we put `/^:/` in a variable, and use it here for string.replace ? I feel like it would be easier to quickly parse than substring a number of char ::: devtools/server/actors/webconsole/commands.js:73 (Diff revision 2) > + */ > +function createToken(string) { > + if (/^:/.test(string)) { > + const value = string.substr(1); > + if (!value || !validCommands.includes(value)) { > + throw Error("invalid command"); nit: Could we throw something like `"${value}" is not a valid command` ? ::: devtools/server/actors/webconsole/commands.js:78 (Diff revision 2) > + throw Error("invalid command"); > + } > + return { type: "command", value }; > + } > + if (/^--/.test(string)) { > + const value = string.substr(2); nit: same here, could we have a doubleDashRegexp variable and use it to test and also replace ? ::: devtools/server/actors/webconsole/commands.js:98 (Diff revision 2) > +function parseCommand(tokens) { > + const output = { command: null, args: {} }; > + > + for (let i = 0; i < tokens.length; i++) { > + const token = tokens[i]; > + if (token.type === "command") { nit: I realize that we use some strings as keys, could we put them in const ? ::: devtools/server/actors/webconsole/commands.js:99 (Diff revision 2) > + const output = { command: null, args: {} }; > + > + for (let i = 0; i < tokens.length; i++) { > + const token = tokens[i]; > + if (token.type === "command") { > + if (output.command) { could we add a comment explaining why we throw here ? (I guess because we already set the command in a previous loop iteration, but it would be more clear explicitely saying it) ::: devtools/server/actors/webconsole/commands.js:120 (Diff revision 2) > + i++; > + } > + output.args[token.value] = values; > + } > + > + // for now, ignore multiple unflagged args do we plan to add this support in another bug ? if so, that would be good to reference it here ::: devtools/server/tests/unit/test_format_command.js:9 (Diff revision 2) > +const testcases = [ > + { input: ":help", expectedOutput: "help()" }, > + { > + input: ":screenshot --fullscreen", > + expectedOutput: "screenshot({ fullscreen: true })" > + }, > + { input: ":screenshot ", expectedOutput: "screenshot()" }, > + { > + input: ":screenshot --dpr 0.5 --fullpage --chrome", > + expectedOutput: "screenshot({ dpr: 0.5, fullpage: true, chrome: true })" > + }, > + { > + input: ":screenshot 'filename'", > + expectedOutput: "screenshot({ filename: 'filename' })" > + }, > + { > + input: ":screenshot 'filename1' 'filename2' 'filename3'", > + expectedOutput: "screenshot({ filename: 'filename1' })" > + }, > + // unhandled case: > + // { > + // input: ":screenshot \"file name with spaces\"", > + // expectedOutput: "screenshot({ filename: "file name with spaces"" > + // } > +]; i think the functon handle such case as `screenshot --opt --opt` ? If so we should make sure it produces the right output ::: devtools/server/tests/unit/test_format_command.js:28 (Diff revision 2) > + }, > + { > + input: ":screenshot 'filename1' 'filename2' 'filename3'", > + expectedOutput: "screenshot({ filename: 'filename1' })" > + }, > + // unhandled case: do we have a follow up bug for this that we could reference here ?

We should make a decision about Bug 1466504 about removing the 'upload to imgur' feature before landing this as-is to avoid extra work for localizers having to translate the upload strings if we are going to remove them shortly anyway. I'd be OK to remove it personally - but needinfo Sole who's looked into this to make a final decision. On that note, we should also check with l10n to see if there's a way to 'copy over' the strings in screenshot.properties from their old location at https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/devtools/shared/locales/en-US/gclicommands.properties#37 to avoid retranslating.

(In reply to Brian Grinstead [:bgrins] from comment #16) > On that note, we should also check with l10n to see if there's a way to > 'copy over' the strings in screenshot.properties from their old location at > https://searchfox.org/mozilla-central/rev/ > c621276fbdd9591f52009042d959b9e19b66d49f/devtools/shared/locales/en-US/ > gclicommands.properties#37 to avoid retranslating. We don't have safe ways to copy strings between .properties files. If they use Pontoon to localize, they will see suggestions from existing Translation Memory.

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review256268 > nit: could this be named listScreenshotOptions, or getScreenshotOptions so it's more obvious what it does ? probably it should be binded to :screenshot --help / screenshot({help: true})

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review256278 > so here, I think we could have a problem if the content already defines a `screenshot` function. > > If this is the case, I expect `screenshot()` to call the content function, but `:screenshot` should run the command code. I'm not sure how we should handle this, but maybe we could actually call the screenshot code on the actor without relying on the js `screenshot()` helper ? i dont think we should create another way that commands could be written, however we can use the existing solution of allowing overrides of the `$` / `$$` coming from user space. I have done a minimal intervention to make this possible, but i also have a rewrite of evalWithDebugger. Let me know what you think of the minimal version and if you think that the rewrite makes this big block of code easier to deal with > instead of manipulating strings here, do you think we could build a plain js object and return JSON.stringify of it ? we would then miss certain cases such as getting nodes for inspect for instance. it would be transformed into a string and not evaluated. for example passing a selector with document.querySelector('.class') would need special cases to work. > do we have a follow up bug for this that we could reference here ? this was pushed as a wip, it should be fixed now

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review256836 ::: devtools/server/actors/webconsole.js:1384 (Diff revision 3) > } > > // Check if the Debugger.Frame or Debugger.Object for the global include > - // $ or $$. We will not overwrite these functions with the Web Console > + // $ or $$ or screenshot. We will not overwrite these functions with the Web Console > // commands. > - let found$ = false, found$$ = false; > + let found$ = false, found$$ = false, foundScreenshot = false; i dont think this is the best solution, but it is a minimal intervention for now.

The tests are still a work in progress but i think we are getting close

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review256840 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/actors/webconsole/utils.js:9 (Diff revision 3) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > const {Ci, Cu} = require("chrome"); > +const {screenshot, getScreenshotCommandParams} = require("devtools/server/actors/webconsole/screenshot"); Error: 'getscreenshotcommandparams' is assigned a value but never used. [eslint: no-unused-vars] ::: devtools/server/actors/webconsole/utils.js:607 (Diff revision 3) > +WebConsoleCommands._registerOriginal("screenshot", function(owner, args) { > + const reply = screenshot(owner, args); > + owner.helperResult = { > + type: "screenshotOutput", > + value: reply > + } Error: Missing semicolon. [eslint: semi]

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review256842 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/actors/webconsole/utils.js:9 (Diff revision 3) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > const {Ci, Cu} = require("chrome"); > +const {screenshot, getScreenshotCommandParams} = require("devtools/server/actors/webconsole/screenshot"); Error: 'getscreenshotcommandparams' is assigned a value but never used. [eslint: no-unused-vars] ::: devtools/server/actors/webconsole/utils.js:607 (Diff revision 3) > +WebConsoleCommands._registerOriginal("screenshot", function(owner, args) { > + const reply = screenshot(owner, args); > + owner.helperResult = { > + type: "screenshotOutput", > + value: reply > + } Error: Missing semicolon. [eslint: semi]

Comment on attachment 8984659 [details] Bug 1464461 - experiemental rewrite of evalWithDebugger; https://reviewboard.mozilla.org/r/250526/#review256844 Code analysis found 4 defects in this patch: - 4 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/actors/webconsole.js:30 (Diff revision 1) > loader.lazyRequireGetter(this, "StackTraceCollector", "devtools/shared/webconsole/network-monitor", true); > loader.lazyRequireGetter(this, "JSPropertyProvider", "devtools/shared/webconsole/js-property-provider", true); > loader.lazyRequireGetter(this, "Parser", "resource://devtools/shared/Parser.jsm", true); > loader.lazyRequireGetter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm", true); > loader.lazyRequireGetter(this, "addWebConsoleCommands", "devtools/server/actors/webconsole/utils", true); > loader.lazyRequireGetter(this, "formatCommand", "devtools/server/actors/webconsole/commands", true); Error: 'formatcommand' is defined but never used. [eslint: no-unused-vars] ::: devtools/server/actors/webconsole.js:31 (Diff revision 1) > loader.lazyRequireGetter(this, "JSPropertyProvider", "devtools/shared/webconsole/js-property-provider", true); > loader.lazyRequireGetter(this, "Parser", "resource://devtools/shared/Parser.jsm", true); > loader.lazyRequireGetter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm", true); > loader.lazyRequireGetter(this, "addWebConsoleCommands", "devtools/server/actors/webconsole/utils", true); > loader.lazyRequireGetter(this, "formatCommand", "devtools/server/actors/webconsole/commands", true); > loader.lazyRequireGetter(this, "isCommand", "devtools/server/actors/webconsole/commands", true); Error: 'iscommand' is defined but never used. [eslint: no-unused-vars]

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review257580 I have a few minor comment, but this looks good for me ::: devtools/client/webconsole/components/JSTerm.js:317 (Diff revision 3) > break; > case "copyValueToClipboard": > clipboardHelper.copyString(helperResult.value); > break; > + case "screenshotOutput": > + console.log(helperResult); not sure what we are doing here ? ::: devtools/shared/locales/en-US/screenshot.properties:33 (Diff revision 3) > +screenshotFilenameDesc=Destination filename > + > +# LOCALIZATION NOTE (screenshotFilenameManual) A fuller description of the > +# 'filename' parameter to the 'screenshot' command, displayed when the user > +# asks for help on what it does. > +screenshotFilenameManual=The name of the file (should have a ‘.png’ extension) to which we write the screenshot. not sure if we should move or not the (should have a png extension) part at the end of the sentence ::: devtools/shared/locales/en-US/screenshot.properties:97 (Diff revision 3) > +# in HH.MM.SS format. Please don't add the extension here. > +screenshotGeneratedFilename=Screen Shot %1$S at %2$S > + > +# LOCALIZATION NOTE (screenshotErrorSavingToFile) Text displayed to user upon > +# encountering error while saving the screenshot to the file specified. > +screenshotErrorSavingToFile=Error saving to shouldn't the path to the file be a part of this l10n entry ? ::: devtools/shared/locales/en-US/screenshot.properties:101 (Diff revision 3) > +# encountering error while saving the screenshot to the file specified. > +screenshotErrorSavingToFile=Error saving to > + > +# LOCALIZATION NOTE (screenshotSavedToFile) Text displayed to user when the > +# screenshot is successfully saved to the file specified. > +screenshotSavedToFile=Saved to same question here ::: devtools/shared/locales/en-US/screenshot.properties:105 (Diff revision 3) > +# screenshot is successfully saved to the file specified. > +screenshotSavedToFile=Saved to > + > +# LOCALIZATION NOTE (screenshotErrorCopying) Text displayed to user upon > +# encountering error while copying the screenshot to clipboard. > +screenshotErrorCopying=Error occurred while copying to clipboard. maybe we should say that this was about the screenshot (Error occured while copying screenshot to clipboard) ? ::: devtools/shared/locales/en-US/screenshot.properties:109 (Diff revision 3) > +# encountering error while copying the screenshot to clipboard. > +screenshotErrorCopying=Error occurred while copying to clipboard. > + > +# LOCALIZATION NOTE (screenshotCopied) Text displayed to user when the > +# screenshot is successfully copied to the clipboard. > +screenshotCopied=Copied to clipboard. Same thing here, maybe say "Screenshot copied to clipboard" ?

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review257588 Looks good to me overall, even if I whish the argument parsing would be a more simpler to follow. ::: devtools/server/actors/webconsole.js:1384 (Diff revision 3) > } > > // Check if the Debugger.Frame or Debugger.Object for the global include > - // $ or $$. We will not overwrite these functions with the Web Console > + // $ or $$ or screenshot. We will not overwrite these functions with the Web Console > // commands. > - let found$ = false, found$$ = false; > + let found$ = false, found$$ = false, foundScreenshot = false; Yes, I guess we could have a follow-up to make this more generic and handle other commands we have (copy, help, keys, values, ...) ::: devtools/server/actors/webconsole/commands.js:123 (Diff revision 3) > + > + if (token.type === KEY) { > + const nextTokenIndex = i + 1; > + const nextToken = tokens[nextTokenIndex]; > + let values = args[token.value] || DEFAULT_VALUE; > + if (nextToken && nextToken.type === "arg") { looks like we could use ARG here instead of "arg" ::: devtools/server/actors/webconsole/commands.js:160 (Diff revision 3) > + return { command, args }; > +} > + > +function collectString(token, tokens, index) { > + const stringChars = ["\"", "'", "`"]; > + function checkFirstChar(testChar) { nit: given the name of the function, i would except it to accept a string and testing the first char of it. Maybe we could do that, or change the name of the functio nto something that better conveys its meaning ? ::: devtools/server/actors/webconsole/commands.js:173 (Diff revision 3) > + > + const firstChar = token.value[0]; > + const isString = checkFirstChar(firstChar); > + > + // the test value is not a string, or it is a string but a complete one > + // ie) `"test"`, as opposed to `"foo`. In either case, this we can return early nit: s/ie)/ie. ? Or maybe I don't know about the ie) notation ? ::: devtools/server/tests/unit/test_format_command.js:21 (Diff revision 3) > + { > + input: ":screenshot --dpr 0.5 --fullpage --chrome", > + expectedOutput: "screenshot({ dpr: 0.5, fullpage: true, chrome: true })" > + }, > + { > + input: ":screenshot 'filename'", I wonder if we could get rid of the need for those strings for the filename ? In the gcli, you wouldn't need them, and I wouldn't expect to need them when using a unix command as well (i.e. cp, mv, ...) Maybe that can be a follow up

Comment on attachment 8984658 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/250524/#review257592 The test fails locally for me: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js FAIL Uncaught exception - at chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/head.js:111 - TypeError: hud.ui is undefined Stack trace: waitForMessages/<@chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/head.js:111:5 waitForMessages@chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/head.js:109:10 waitForMessage@chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/head.js:174:26 async*testFile@chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:44:9 async*@chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:29:9 Which is probably because how we could waitForMessage in the test (without passing the hud as the first argument) ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:4 (Diff revision 1) > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +/* global helpers, btoa, whenDelayedStartupFinished, OpenBrowserWindow */ I thought those were automatically handled ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:19 (Diff revision 1) > + const opened = waitForBrowserConsole(); > + > + let hud = HUDService.getBrowserConsole(); > + ok(!hud, "browser console is not open"); > + info("wait for the browser console to open with ctrl-shift-j"); > + EventUtils.synthesizeKey("j", { accelKey: true, shiftKey: true }, window); > + > + hud = await opened; I think we could use `let hud = await HUDService.toggleBrowserConsole();` instead ? The whole keyshortcut workflow is (or should) be handled by another dedicated test. ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:21 (Diff revision 1) > +add_task(async function() { > + await addTab(TEST_URI); > + > + const opened = waitForBrowserConsole(); > + > + let hud = HUDService.getBrowserConsole(); is there a reason we use the browser console and not the webconsole ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:22 (Diff revision 1) > + await addTab(TEST_URI); > + > + const opened = waitForBrowserConsole(); > + > + let hud = HUDService.getBrowserConsole(); > + ok(!hud, "browser console is not open"); Maybe we could say that the hud does not exist instead ? Because here, the browser console is always not opened (we open it with the keyboard shortcut 2 lines after this) ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:41 (Diff revision 1) > + await testFullpageClipboardScrollbar(hud, scrollbarSize); > +}); > + > +async function testFile(hud) { > + // Test capture to file > + const file = FileUtils.getFile("TmpD", [ "TestScreenshotFile.png" ]); could we retrieve the file after the command was executed ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:44 (Diff revision 1) > +async function testFile(hud) { > + // Test capture to file > + const file = FileUtils.getFile("TmpD", [ "TestScreenshotFile.png" ]); > + const command = `:screenshot ${file.path}`; > + hud.jsterm.execute(command); > + await waitForMessage("Saved to TmpD/TestScreenshotFile.png"); waitForMessage takes the hud as a first argument https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/devtools/client/webconsole/test/mochitest/head.js#108 And you need to declare it before executing the command, since it relies on an event being fired, so you don't risk a race: ```js const onSavedMessage = waitForMessage(hud, path); hud.jsterm.execute(command); await onSavedMessage; ``` ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:56 (Diff revision 1) > +} > + > +async function testClipboard(hud) { > + const command = `:screenshot --clipboard`; > + hud.jsterm.execute(command); > + await waitForMessage("Copied to clipboard."); same as the previous function, we should: - pass the hud to waitForMessage - declare waitForMessage before executing the command and then await on the resulting promise so we don't have races ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:58 (Diff revision 1) > +async function testClipboard(hud) { > + const command = `:screenshot --clipboard`; > + hud.jsterm.execute(command); > + await waitForMessage("Copied to clipboard."); > + const imgSize1 = await getImageSizeFromClipboard(); > + await ContentTask.spawn(browser, imgSize1, function* (imgSize) { nit: the callback can be a plain function and not a generator ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:68 (Diff revision 1) > + hud.jsterm.execute(command); > + await waitForMessage("Copied to clipboard."); same as the previous function, we should: - pass the hud to waitForMessage - declare waitForMessage before executing the command and then await on the resulting promise so we don't have races ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:71 (Diff revision 1) > + await ContentTask.spawn(browser, imgSize1, function* (imgSize) { > + Assert.equal(imgSize.width, > + content.innerWidth + content.scrollMaxX - content.scrollMinX, > + "Image width matches page size"); > + Assert.equal(imgSize.height, > + content.innerHeight + content.scrollMaxY - content.scrollMinY, > + "Image height matches page size"); > + }); do you think we could extract this part in a checkImageSize(expectedSize) function ? The same snippet seems to be used in multiple parts of the test. ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:83 (Diff revision 1) > + hud.jsterm.execute(command); > + await waitForMessage("Copied to clipboard."); same as the previous function, we should: - pass the hud to waitForMessage - declare waitForMessage before executing the command and then await on the resulting promise so we don't have races ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:101 (Diff revision 1) > + // Trigger scrollbars by forcing document to overflow > + // This only affects results on OSes with scrollbars that reduce document size > + // (non-floating scrollbars). With default OS settings, this means Windows > + // and Linux are affected, but Mac is not. For Mac to exhibit this behavior, > + // change System Preferences -> General -> Show scroll bars to Always. > + await ContentTask.spawn(browser, {}, function* () { the function can be a simple one and not a generator ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:105 (Diff revision 1) > + // change System Preferences -> General -> Show scroll bars to Always. > + await ContentTask.spawn(browser, {}, function* () { > + content.document.body.classList.add("overflow"); > + }); > + > + const scrollbarSize = await ContentTask.spawn(browser, {}, function* () { the function can be a plain one and not a generator ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:123 (Diff revision 1) > + hud.jsterm.execute(command); > + await waitForMessage("Copied to clipboard."); same as the other functions, we should: - pass the hud to waitForMessage - declare waitForMessage before executing the command and then await on the resulting promise so we don't have races ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:138 (Diff revision 1) > + hud.jsterm.execute(command); > + await waitForMessage("Copied to clipboard."); same as the other functions, we should: - pass the hud to waitForMessage - declare waitForMessage before executing the command and then await on the resulting promise so we don't have races ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:165 (Diff revision 1) > + const data = new Object(); > + const dataLength = new Object(); i'm curious why are we creating them as new Object and not literals ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js:204 (Diff revision 1) > + const loaded = new Promise(resolve => { > + img.addEventListener("load", function() { > + resolve(); > + }, {once: true}); > + }); maybe we could use the `once` helper function here: https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/devtools/client/shared/test/shared-head.js#336

So, with the patches apply I can't have a screenshot file to appear on my machine. The experience is different from what I get from the inspector -> screenshot node: - there's no shutter sound - the "downloading arrow" doesn't appear/gets animated - I don't get a file on my machine Maybe I missed it and it should only be about supporting the command but not actually screenshoting ?

Comment on attachment 8984659 [details] Bug 1464461 - experiemental rewrite of evalWithDebugger; https://reviewboard.mozilla.org/r/250526/#review257604 I'll look back at this the week after i'm back for PTO (I can only look at nits right now ^^) ::: devtools/server/actors/webconsole/eval-with-debugger.js:10 (Diff revision 1) > +const DevToolsUtils = require("devtools/shared/DevToolsUtils"); > +loader.lazyRequireGetter(this, "formatCommand", "devtools/server/actors/webconsole/commands", true); > +loader.lazyRequireGetter(this, "isCommand", "devtools/server/actors/webconsole/commands", true); > +loader.lazyRequireGetter(this, "Parser", "resource://devtools/shared/Parser.jsm", true); > + > +/** > + * Evaluates a string using the debugger API. > + * > + * To allow the variables view to update properties from the Web Console we this looks like a big, important and legacy function and it would be niceto keep the history on it. Even if it makes little sense, could we copy the webconsole actor file to create this file so we keep the function history ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:105 (Diff revision 1) > + bindings, > + frame, > + dbgWindow > + ); > + > + const helperResult = helpers.helperResult; nit: could be ```js cont {helperResult} = helpers; ``` ::: devtools/server/actors/webconsole/eval-with-debugger.js:110 (Diff revision 1) > + return { > + result: result, > + helperResult: helperResult, > + dbg: dbg, > + frame: frame, > + window: dbgWindow, nit: could be turned into: ```js return { result, helperResult, dbg, frame, window: dbgWindow }; ``` ::: devtools/server/actors/webconsole/eval-with-debugger.js:235 (Diff revision 1) > + > +function getFrameDbg(options, webConsole) { > + if (!options.frameActor) { > + return { frame: null, dbg: webConsole.dbg }; > + } > +// Find the Debugger.Frame of the given FrameActor. nit: comment is a bit off for some reason

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review257580 > not sure what we are doing here ? Right, thanks for bringing this up -- this is a question I had for you -- I am having trouble getting the reply value to be logged in the console because the screenshot is async, and any output from it is somehow "protected". Is there already a path to get things logged? > not sure if we should move or not the (should have a png extension) part at the end of the sentence screenshot.properties was moved from the gcli.properties without changes, this is probably why you are seeing some stuff that looks out of date > shouldn't the path to the file be a part of this l10n entry ? screenshot.properties was moved from the gcli.properties without changes, this is probably why you are seeing some stuff that looks out of date

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review256278 > we would then miss certain cases such as getting nodes for inspect for instance. it would be transformed into a string and not evaluated. for example passing a selector with document.querySelector('.class') would need special cases to work. now that i think about it we could change the server code to retrieve the node from a string, it looks like that is what the expected behavior was on the client, but the server had a different idea, and i chose the server approach. happy to discuss on this!

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review256278

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review257588 > Yes, I guess we could have a follow-up to make this more generic and handle other commands we have (copy, help, keys, values, ...) there is already a follow up in the rewrite of evalWithDebugger, should be much cleaner there!

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review257588 > nit: s/ie)/ie. ? > Or maybe I don't know about the ie) notation ? turns out it would be most correct to do i.e. :)

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review258496 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/actors/webconsole.js:1387 (Diff revision 4) > - // $ or $$. We will not overwrite these functions with the Web Console > + // $ or $$ or screenshot. We will not overwrite these functions with the Web Console > // commands. > - let found$ = false, found$$ = false; > + let found$ = false, found$$ = false, foundScreenshot = false; > + if (!isCmd) { > - if (frame) { > + if (frame) { > - const env = frame.environment; > + let env = frame.environment; Error: 'env' is never reassigned. use 'const' instead. [eslint: prefer-const]

Comment on attachment 8986772 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252064/#review258512 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/actors/webconsole/utils.js:603 (Diff revision 1) > * @param object args > * The arguments to be passed to the screenshot > * @return void > */ > WebConsoleCommands._registerOriginal("screenshot", function(owner, args) { > const reply = screenshot(owner, args); Error: 'reply' is assigned a value but never used. [eslint: no-unused-vars]

It looks like you may be able to wait for your command result asynchronously. First by making helperResult be a Promise: WebConsoleCommands._registerOriginal("screenshot", function(owner, args) { owner.helperResult = new Promise(async (resolve) => { const reply = await screenshot(owner, args); resolve({ type: "screenshotOutput", value: reply }); }); }); And handle this promise correctly from the console actor: WebConsoleActor.evaluateJSAsync: async function(request) { ... const response = this.evaluateJS(request); response.resultID = resultID; // Wait for asynchronous command completion before sending back the response // // There must be a better way of identifying if helperResult is a promise... // We could also remove the if condition and always try to resolve it, // as `await primitive-value` returns the primitive value. // But I'm afraid it may cause races by always spinning the event loop here... if (response.helperResult && typeof(response.helperResult.then) == "function") { response.helperResult = await response.helperResult; } // Finally, send an unsolicited evaluationResult packet with // the normal return value this.conn.sendActorEvent(this.actorID, "evaluationResult", response); },

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review258692 Saving to a file seems to be broken, at least on linux. I get the following error: Unix error 13 during operation makeDir on file /home/alex/Downloads (Permission denied) When doing: :screenshot :screenshot /tmp/foo.png :screenshot --file /tmp/foo.png ::: devtools/server/actors/webconsole/utils.js:9 (Diff revision 5) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > const {Ci, Cu} = require("chrome"); > +const {screenshot} = require("devtools/server/actors/webconsole/screenshot"); Please load this module lazily via loader.lazyRequireGetter. ::: devtools/server/actors/webconsole/screenshot.js:28 (Diff revision 5) > - * Both commands have almost the same set of standard optional parameters, except for the > - * type of the --selector option, which can be a node only on the server. > - */ > -const getScreenshotCommandParams = function(isClient) { > - return { > group: l10n.lookup("screenshotGroupOptions"), I'm not sure we are going to use this attribute anymore? ::: devtools/server/actors/webconsole/screenshot.js:68 (Diff revision 5) > - // trigger an unsafe CPOW. > - type: isClient ? "string" : "node", > - defaultValue: null, > + defaultValue: null, > - description: l10n.lookup("inspectNodeDesc"), > + description: l10n.lookup("inspectNodeDesc"), > - manual: l10n.lookup("inspectNodeManual") > + manual: l10n.lookup("inspectNodeManual") > - }, > + }, Did you tested selector argument? It fails for me with following error: Warning: getUnicodeUrl failed to get a Unicode URL fromdebugger eval code, reason: TypeError: debugger eval code is not a valid URL.error:TypeError: node.ownerDocument is undefined error:TypeError: node.ownerDocument is undefined :screenshot --clipboard --selector "body" If you can't fix that easily, remove this and fix it in a followup. ::: devtools/server/actors/webconsole/screenshot.js:83 (Diff revision 5) > - file.reveal(); > - } > - }); > - } > + } > + ] > +}; All these params objects are only used by `:screenshot --help` which you don't support yet. The output was being used by gcli frontend to print a nice help message. It would be weird to print these JSON objects as-is. So I would rather strip that until proper implementation of help message/argument. (and remove the l10n string until we use them) ::: devtools/server/actors/webconsole/screenshot.js:100 (Diff revision 5) > - simulateCameraEffect(context.environment.chromeDocument, "shutter"); > - return capture.then(saveScreenshot.bind(null, args, context)); > - }, > - }, > - { > - item: "command", > +exports.screenshot = async function takeScreenshot(owner, args = {}) { > + if (args.help) { > + return getScreenshotCommandParams; > + } > + const context = createContext(owner); > + simulateCameraEffect(context.chromeDocument, "shutter"); I'm seeing the following error message: Security Error: Content at https://www.google.com/ may not load or link to resource://devtools/client/themes/audio/shutter.wav. You may want to remove the audio feature to address that in a followup.

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review258706 I imagine I should have looked deeper at the patch sooner, sorry for not having done that! I see two points that requires some discussion: * Should we expose screenshot as a function? * Should we allow arbitrary JS evaluation for screenshot arguments? May be you already talked about that with Sole, Nicolas or Brian? It would be great to post a conclusion here. ::: devtools/server/actors/webconsole.js:1410 (Diff revision 5) > if (found$$) { > $$ = bindings.$$; > delete bindings.$$; > } > + if (foundScreenshot) { > + screenshot = bindings.screenshot; I'm wondering if we should expose screenshot as a function at all. Isn't the goal to use commands as :screenshot? Or do we want to expose both ways: functions and commands? My issues with functions is that it brings more questions around possible security issues. $ and $$ doesn't bring any particular privileges, while screenshot certainly does as it is able to write files and take privileged screenshots that cross the domain boundaries. I imagine you did that while thinking about the other already existing commands: https://searchfox.org/mozilla-central/search?q=registerOriginal(&case=false&regexp=false&path= Where it makes sense for them to be functions used inside a piece of arbitrary JS being written in jsterm *and* it makes also sense for them to accept arbitrary JS as arguments. May be we should be more conservative for a first shot, start with only :screenshot and let it be exposed as a command only without any JS evaluation of its arguments? In such scenario, we would rather call a `executeCommand` function rather than a `formatCommand` one. ::: devtools/server/actors/webconsole/commands.js:67 (Diff revision 5) > + }); > + > + if (argsString.length) { > + return `{ ${argsString.join(", ")} }`; > + } > + return ""; I think Nicolas already commented on this. It would be safer to map `args` to a js object with another shape and then call JSON.stringify, rather than manipulating strings like that! Regarding: > > we would then miss certain cases such as getting nodes for inspect for instance. it would be transformed into a string and not evaluated. for example passing a selector with document.querySelector('.class') would need special cases to work. > now that i think about it we could change the server code to retrieve the node from a string, it looks like that is what the expected behavior was on the client, but the server had a different idea, and i chose the server approach. Yes, the expected behavior is to pass a selector string and let the server evaluate it. I don't know if we should let arbitrary JS evaluation be possible in command arguments for screenshot. It sounds nice, bet also subject to security issues! But as said in a next comment, this feature makes sense for the existing commands like inspect(), values(), keys(), ... ::: devtools/server/tests/unit/test_format_command.js:33 (Diff revision 5) > + { > + input: ":screenshot --name 'filename' --name `filename` --name \"filename\"", > + expectedOutput: "screenshot({ name: ['filename',`filename`,\"filename\"] })" > + }, > + { > + input: ":screenshot 'filename1' 'filename2' 'filename3'", Could you add tests covering: :screenshot 'filename1' --name 'filename2' :screenshot --name 'filename1' 'filename2' ::: devtools/server/tests/unit/test_format_command.js:47 (Diff revision 5) > + expectedOutput: "screenshot({ filename: \"file name with spaces\" })" > + } > +]; > + > +const edgecases = [ > + { input: ":", expectedOutput: "invalid command" }, I think it would be less confusing to name "expectedOutput" "expectedError" or "expectedException".

Comment on attachment 8986772 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252064/#review258712 You may be able to complete this test with bug 1464461 comment 46.

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review258692 saving to file is broken on any page that is not an FF page. This doesn't seem to be related to the screenshot code, rather what is happening is in https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadIntegration.jsm#271 -- this fails if we are not on an FF page, and the catch never resolves. after thinking about this a bit, i suspect this needs some priviledges that are not available? Probably this is why it might have worked on gcli and no longer does > I'm not sure we are going to use this attribute anymore? it is just a string "options". i added some basic formatting so that if someone uses the help keyword it is shown in a sensible way > Did you tested selector argument? > It fails for me with following error: > Warning: getUnicodeUrl failed to get a Unicode URL fromdebugger eval code, reason: TypeError: debugger eval code is not a valid URL.error:TypeError: node.ownerDocument is undefined > error:TypeError: node.ownerDocument is undefined > > :screenshot --clipboard --selector "body" > > If you can't fix that easily, remove this and fix it in a followup. This was using the server syntax rather than the client one --> I will change it to the client syntax. > All these params objects are only used by `:screenshot --help` which you don't support yet. The output was being used by gcli frontend to print a nice help message. It would be weird to print these JSON objects as-is. > So I would rather strip that until proper implementation of help message/argument. (and remove the l10n string until we use them) im a bit wary of removing things that were being used in the old implementation. I did a simple formatter so that these are displayed in a sensible way > I'm seeing the following error message: > Security Error: Content at https://www.google.com/ may not load or link to resource://devtools/client/themes/audio/shutter.wav. > > You may want to remove the audio feature to address that in a followup. this was related to the file issue and is now fixed

Comment on attachment 8983463 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/249324/#review258706 > I'm wondering if we should expose screenshot as a function at all. > Isn't the goal to use commands as :screenshot? > Or do we want to expose both ways: functions and commands? > > My issues with functions is that it brings more questions around possible security issues. > $ and $$ doesn't bring any particular privileges, while screenshot certainly does as it is able to write files and take privileged screenshots that cross the domain boundaries. > > I imagine you did that while thinking about the other already existing commands: > https://searchfox.org/mozilla-central/search?q=registerOriginal(&case=false&regexp=false&path= > Where it makes sense for them to be functions used inside a piece of arbitrary JS being written in jsterm *and* it makes also sense for them to accept arbitrary JS as arguments. > > May be we should be more conservative for a first shot, start with only :screenshot and let it be exposed as a command only without any JS evaluation of its arguments? > In such scenario, we would rather call a `executeCommand` function rather than a `formatCommand` one. This was discussed in the issue that is associated with this patch (see https://bugzilla.mozilla.org/show_bug.cgi?id=1464461#c2). The idea was to have `screenshot()` available first as an mvp, and then at a later point add `:screenshot`. I went ahead and added `:screenshot` since it wasn't much more work. I think you make a good point about arbitrary js execution, so I will mask the `screenshot()` syntax using the `isCmd` boolean. I am not sure about renaming `formatCommand`, since that is what it does (it doesn't handle any execution, that is done as part of evalInDebugger, and is handled by c++ code) -- I would rather refactor all of this code so that commands do not share the same path as regular js evaluation and have a dedicated path. but that is out of scope for this issue.

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252766/#review259230 Code analysis found 3 defects in this patch: - 3 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/main.js:1656 (Diff revision 1) > }).then(response => { > if (!this.transport) { > throw new Error(`Connection closed, pending response from ${from}, ` + > `type ${type} failed`); > } > + dump(`%%%%%%%%%%%%%%%%%%%\n`) Error: Missing semicolon. [eslint: semi] ::: devtools/server/main.js:1657 (Diff revision 1) > if (!this.transport) { > throw new Error(`Connection closed, pending response from ${from}, ` + > `type ${type} failed`); > } > + dump(`%%%%%%%%%%%%%%%%%%%\n`) > + dump(`${type}\n`) Error: Missing semicolon. [eslint: semi] ::: devtools/server/main.js:1658 (Diff revision 1) > throw new Error(`Connection closed, pending response from ${from}, ` + > `type ${type} failed`); > } > + dump(`%%%%%%%%%%%%%%%%%%%\n`) > + dump(`${type}\n`) > + dump(`%%%%%%%%%%%%%%%%%%%\n`) Error: Missing semicolon. [eslint: semi]

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review259220 Thanks Yulia, it looks good to me. I mostly have aesthetic comments now. ::: devtools/client/webconsole/components/JSTerm.js:345 (Diff revision 7) > + result.destinations.forEach(destination => { > + this.hud.consoleOutput.dispatchMessageAdd({ > + result: destination > + }, true); > + }); > + } This polymorphism is hard to follow. `!result.destinations` is for when you are using `--help`, but this logic is hidden deep into processScreenshot/screenshot-helper.js. I would suggest processScreenshot to always return an array of messages to log: ``` const messages = await processScreenshot(helperResult, this.hud); for (let message in messages) { this.hud.consoleOutput.dispatchMessageAdd({ result: message }, true); // <= please also comment why you pass `true` here. } ``` ::: devtools/server/actors/webconsole/utils.js:607 (Diff revision 7) > + */ > +WebConsoleCommands._registerOriginal("screenshot", function(owner, args) { > + const promise = screenshot(owner, args); > + owner.helperResult = { > + type: "screenshotOutput", > + async: true, Are you using this attribute somewhere? ::: devtools/server/actors/webconsole/screenshot.js:15 (Diff revision 7) > -const FILENAME_DEFAULT_VALUE = " "; > const CONTAINER_FLASHING_DURATION = 500; > +const STRINGS_URI = "devtools/shared/locales/screenshot.properties"; > +const L10N = new LocalizationHelper(STRINGS_URI); > > -/* > +exports.screenshot = function takeAsyncScreenshot(owner, args = {}) { Instead of passing `owner` here, we could immediatly pass the `window` or `document` object. ::: devtools/server/actors/webconsole/screenshot.js:41 (Diff revision 7) > if (args.delay > 0) { > return new Promise((resolve, reject) => { > document.defaultView.setTimeout(() => { > createScreenshotData(document, args).then(resolve, reject); > }, args.delay * 1000); > }); nit: you may improve the readabiliy of this function by using async/await and waitForTime https://searchfox.org/mozilla-central/source/devtools/shared/DevToolsUtils.js#90 ::: devtools/server/actors/webconsole/screenshot.js:69 (Diff revision 7) > window.scrollTo(0, 0); > width = window.innerWidth + window.scrollMaxX - window.scrollMinX; > height = window.innerHeight + window.scrollMaxY - window.scrollMinY; > filename = filename.replace(".png", "-fullpage.png"); > } else if (args.selector) { > - ({ top, left, width, height } = getRect(window, args.selector, window)); > + const selector = window.document.querySelector(args.selector); The variable name should be `node` instead of `selector`. ::: devtools/shared/locales/en-US/screenshot.properties:19 (Diff revision 7) > + > +# LOCALIZATION NOTE (screenshotDesc) A very short description of the > +# 'screenshot' command. See screenshotManual for a fuller description of what > +# it does. This string is designed to be shown in a menu alongside the > +# command name, which is why it should be as short as possible. > +screenshotDesc=Save an image of the page Please review each l10n pair and remove the one that aren't used. I see the following as potentialy not used: screenshotDesc screenshotManual screenshotGroupOptions screenshotTooltipPage ::: devtools/shared/webconsole/screenshot-helper.js:9 (Diff revision 7) > + > +"use strict"; > + > +const { Cc, Ci, Cr } = require("chrome"); > +const ChromeUtils = require("ChromeUtils"); > +const { LocalizationHelper } = require("devtools/shared/L10N"); File names for modules are always using lowercases. We only use capitals for JSMs. (same comment for actors/webconsole/screenshot.js) ::: devtools/shared/webconsole/screenshot-helper.js:95 (Diff revision 7) > + const name = `${padding}--${value}`; > + return name; > + } > + if (key === "type") { > + return `${padding}${padding}${key}: ${formatType(param.type)}`; > + } I'm not sure `type` and `defaultValue` serializations are ready for production. * For `type`, when it is primitives, the output is great. When it is objects, it is not and I can't understand what it means: -dpr type: {\"name\":\"number\",\"min\":0,\"allowFloat\":true} * Similar comment applies to `defaultValue` --filename type: string defaultValue: \"\"` And at the end, you never checks for types, not defaults to the values mentioned here... So it is better to drop all of that (serialization and definitions in `screenshotCommandParams`) and come back with a coherent implementation of that in a followup. ::: devtools/shared/webconsole/screenshot-helper.js:108 (Diff revision 7) > + .join("\n\n"); > + > + return `${screenshotGroupOptions}\n\n${formattedParams}`; > +} > + > +module.exports = function processScreenshot({ value, args = {} }, context) { 1) I don't think this function should receive `helperResult` but instead, only what it needs. 2) helperResult.value is set to `null` from devtools/server/actors/webconsole/utils.js's `WebConsoleCommands._registerOriginal("screenshot",`, so I don't quite follow why we are using it here? Oh wait, that's because `helperResult.value` is replaced in `evaluateJSAsync` by `helperResult.promise`'s resolution. I think my approach was less confusing? Or is it just because I wrote it originaly? Was that confusing for you? ::: devtools/shared/webconsole/screenshot-helper.js:144 (Diff revision 7) > + > + return Promise.all([ > + args.clipboard ? saveToClipboard(context, reply) : SKIP, > + args.imgur ? uploadToImgur(reply) : SKIP, > + fileNeeded ? saveToFile(context, reply) : SKIP, > + ]).then(() => reply); All that has been written before async/await andcould be much clearer now: ``` async function saveScreenshot(args, context, reply) { const fileNeeded = ...; if (args.clipboard) { await saveToClipboard(context, reply); } if (args.imgur) { await uploadToImgur(reply); } if (fileNeeded) { await saveToFile(context, reply); } return reply; } ``` Also, I'm wondering if it would help to pass to saveScreenshot only what it needs, instead of the whole context object: ``` async function saveScreenshot(args, window, reply) { const fileNeeded = ...; if (args.clipboard) { await saveToClipboard(window); } if (args.imgur) { await uploadToImgur(reply); } if (fileNeeded) { await saveToFile(window); } return reply; } ``` ::: devtools/shared/webconsole/screenshot-helper.js:234 (Diff revision 7) > + xhr.responseType = "json"; > + > + xhr.onreadystatechange = function() { > + if (xhr.readyState == 4) { > + if (xhr.status == 200) { > + reply.href = xhr.response.data.link; We are no longer using this `href` attribute, but only `destinations`. ::: devtools/shared/webconsole/screenshot-helper.js:319 (Diff revision 7) > + const document = context.window.document; > + const window = context.window; > + > + // Check there is a .png extension to filename > + if (!reply.filename.match(/.png$/i)) { > + reply.filename += ".png"; Same comment than `href`, we are no longer reading another other attribute except `destinations`, so it would be clearer to make a local variable of `reply` and only modify it.

Comment on attachment 8987489 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/252742/#review259260 The two last changesets are hard to review as you mixed things. You adressed in the last one things introduced in the first or in the second patch. Could you clean up things? It may be hard for you to keep the second one now, with only things related to unix style syntax. One option could be to have just two commits: one for the new test, and another with the rest.

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252766/#review259266

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review259220 > I'm not sure `type` and `defaultValue` serializations are ready for production. > * For `type`, when it is primitives, the output is great. When it is objects, it is not and I can't understand what it means: > -dpr > type: {\"name\":\"number\",\"min\":0,\"allowFloat\":true} > * Similar comment applies to `defaultValue` > --filename > type: string > defaultValue: \"\"` > > And at the end, you never checks for types, not defaults to the values mentioned here... > So it is better to drop all of that (serialization and definitions in `screenshotCommandParams`) and come back with a coherent implementation of that in a followup. I changed the type to be simpler to understand and removed the default value field > 1) I don't think this function should receive `helperResult` but instead, only what it needs. > 2) helperResult.value is set to `null` from devtools/server/actors/webconsole/utils.js's `WebConsoleCommands._registerOriginal("screenshot",`, so I don't quite follow why we are using it here? > Oh wait, that's because `helperResult.value` is replaced in `evaluateJSAsync` by `helperResult.promise`'s resolution. > I think my approach was less confusing? Or is it just because I wrote it originaly? Was that confusing for you? The approach you originally suggested doesn't work, because the helper is resolved in c++ code and this code is not available at this point in the execution, as a result I needed to come up with a way of resolving the promise under these constraints.. I am open to changing this if there isa better idea.. > Same comment than `href`, we are no longer reading another other attribute except `destinations`, so it would be clearer to make a local variable of `reply` and only modify it. I am starting to wonder if the imgur functionality is worth keeping. I left it here primarily to make this refactor as close to the original code as I could, but we are changing quite a bit of it now. There is a bug open here: https://bugzilla.mozilla.org/show_bug.cgi?id=1466504

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review259716 ::: devtools/client/webconsole/components/JSTerm.js:315 (Diff revision 8) > case "copyValueToClipboard": > clipboardHelper.copyString(helperResult.value); > break; > + case "screenshotOutput": > + const res = await processScreenshot(helperResult, this.hud.window); > + this.screenshotNotify(res); maybe we could directly use dispatchMessagesAdd(res) https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/devtools/client/webconsole/webconsole-output-wrapper.js#257 ? ::: devtools/server/actors/webconsole/utils.js:604 (Diff revision 8) > + owner.helperResult = new Promise(async (resolve) => { > + const reply = await screenshot(owner, args); > + resolve({ > + type: "screenshotOutput", > + value: reply, > + args > + }); > + }); nit: could we return the result of the async function without wrapping it in a promise ? ```js owner.helperResult = (async () => { const value = await screenshot(owner, args); return { type: "screenshotOutput", value, args }; )(); ``` I feel like this should work since the async function call will return a promise, and is a bit more straightforward ? not really sure about this with the iife, so feel free to keep this as is or change it :) ::: devtools/server/actors/webconsole/screenshot.js:17 (Diff revision 8) > +const STRINGS_URI = "devtools/shared/locales/screenshot.properties"; > +const L10N = new LocalizationHelper(STRINGS_URI); > > -/* > - * There are 2 commands and 1 converter here. The 2 commands are nearly > - * identical except that one runs on the client and one in the server. > +exports.screenshot = function takeAsyncScreenshot(owner, args = {}) { > + if (args.help) { > + return null; Can we add a comment here explaining why we bail in this case ? ::: devtools/server/actors/webconsole/screenshot.js:69 (Diff revision 8) > window.scrollTo(0, 0); > width = window.innerWidth + window.scrollMaxX - window.scrollMinX; > height = window.innerHeight + window.scrollMaxY - window.scrollMinY; > filename = filename.replace(".png", "-fullpage.png"); > } else if (args.selector) { > - ({ top, left, width, height } = getRect(window, args.selector, window)); > + const selector = window.document.querySelector(args.selector); nit: here `selector` refers to an element (or null), so we could either call the variable element, node or result to better convey what it holds. ::: devtools/shared/webconsole/screenshot-helper.js:32 (Diff revision 8) > + { > + name: "imgur", > + type: "boolean", > + description: L10N.getStr("screenshotImgurDesc"), > + manual: L10N.getStr("screenshotImgurManual") > + }, is it removed in a following patch ? ::: devtools/shared/webconsole/screenshot-helper.js:76 (Diff revision 8) > + description: L10N.getStr("screenshotFilenameDesc"), > + manual: L10N.getStr("screenshotFilenameManual") > + } > +]; > + > +function formatHelpField(param) { could we have jsdoc on this function ? ::: devtools/shared/webconsole/screenshot-helper.js:84 (Diff revision 8) > + return type; > + } > + return JSON.stringify(type); > + } > + > + const padding = new Array(5).join(" "); nit: we could use `" ".repeat(4)` or `" "` to be more explicit. ::: devtools/shared/webconsole/screenshot-helper.js:90 (Diff revision 8) > + if (key === "type") { > + return `${padding}${padding}${key}: ${formatType(param.type)}`; > + } > + return `${padding}${padding}${key}: ${value}`; nit: we could simplify this a bit maybe ```js if (key === "type") { value = formatType(value); } return `${padding.repeat(2)}${key}: ${value}`; ``` ::: devtools/shared/webconsole/screenshot-helper.js:97 (Diff revision 8) > + } > + return `${padding}${padding}${key}: ${value}`; > + }).join("\n"); > +} > + > +function getFormattedHelpData() { could we add jsdoc to this function ? ::: devtools/shared/webconsole/screenshot-helper.js:105 (Diff revision 8) > +module.exports = function processScreenshot({ value, args = {} }, window) { > + if (args.help) { > + const message = getFormattedHelpData(); > + // Wrap meesage in an array so that the return value is consistant with saveScreenshot > + return [message]; > + } > + simulateCameraShutter(window.document); > + return saveScreenshot(args, window, value); > +}; I don't know if it's true everywhere, but in most files we export at the very end of the file. SO maybe here we could move this function to the bottom, or just declare processScreenshot and export it at the bottom ? ::: devtools/shared/webconsole/screenshot-helper.js:127 (Diff revision 8) > + audioCamera.play(); > + } > +} > + > +/** > + * Save the captured screenshot to one of several destinations. it would be nice to add the expected params and what the function returns ::: devtools/shared/webconsole/screenshot-helper.js:131 (Diff revision 8) > +/** > + * Save the captured screenshot to one of several destinations. > + */ > +async function saveScreenshot(args, window, image) { > + const fileNeeded = args.filename || > + (!args.imgur && !args.clipboard) || args.file; is the imgur case is removed in a following patch ? ::: devtools/shared/webconsole/screenshot-helper.js:134 (Diff revision 8) > + if (args.clipboard) { > + const result = await saveToClipboard(window, image.data); > + results.push(result); > + } > + > + if (fileNeeded) { > + const result = await saveToFile(window, image); > + results.push(result); > + } > + return results; can we have a case where we save both to disk and clipboard ? if not, maybe we could only return the result and not an array. If we can have both, maybe we could not await each time and push the promises to the array, and return Promise.all(results) ? This way the 2 functions can run "in parallel" . ::: devtools/shared/webconsole/screenshot-helper.js:150 (Diff revision 8) > + > +/** > + * Save the image data to the clipboard. This returns a promise, so it can > + * be treated exactly like imgur / file processing, but it's really sync > + * for now. > + */ could we add what the expected params are and what we return ? ::: devtools/shared/webconsole/screenshot-helper.js:161 (Diff revision 8) > + const loadContext = window > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIWebNavigation) > + .QueryInterface(Ci.nsILoadContext); nit: the indent feels a bit weird ? But maybe it's just me, feel free to keep it ::: devtools/shared/webconsole/screenshot-helper.js:170 (Diff revision 8) > + > + const callback = { > + onImageReady(container, status) { > + if (!container) { > + console.error("imgTools.decodeImageAsync failed"); > + resolve(L10N.getStr("screenshotErrorCopying")); why don't we reject when this occurs ? ::: devtools/shared/webconsole/screenshot-helper.js:190 (Diff revision 8) > + Services.clipboard.setData(trans, null, Ci.nsIClipboard.kGlobalClipboard); > + > + resolve(L10N.getStr("screenshotCopied")); > + } catch (ex) { > + console.error(ex); > + resolve(L10N.getStr("screenshotErrorCopying")); why don't we reject when this occurs ? ::: devtools/shared/webconsole/screenshot-helper.js:202 (Diff revision 8) > + .getService(Ci.imgITools); > + imgTools.decodeImageAsync(input, channel.contentType, callback, > + threadManager.currentThread); > + } catch (ex) { > + console.error(ex); > + resolve(L10N.getStr("screenshotErrorCopying")); why don't we reject when this occurs ? ::: devtools/shared/webconsole/screenshot-helper.js:219 (Diff revision 8) > + * visual feedback from the downloads toolbar button when the save is done. > + * > + * It also allows the browser window to show auth prompts if needed (should not > + * be needed for saving screenshots). > + * > + * This code is borrowed directly from contentAreaUtils.js. could we add expected params and returns ? ::: devtools/shared/webconsole/screenshot-helper.js:234 (Diff revision 8) > + this._completedDeferred = defer(); > + this.completed = this._completedDeferred.promise; we are trying to get rid of defer in other places, it would be nice if we can not add it here ::: devtools/shared/webconsole/screenshot-helper.js:243 (Diff revision 8) > + getInterface: function(iid) { > + if (iid.equals(Ci.nsIAuthPrompt) || > + iid.equals(Ci.nsIAuthPrompt2)) { > + const ww = Cc["@mozilla.org/embedcomp/window-watcher;1"] > + .getService(Ci.nsIPromptFactory); > + return ww.getPrompt(this.window, iid); > + } > + > + throw Cr.NS_ERROR_NO_INTERFACE; > + }, this function isn't called directly in this file. Is this called in the "persister" ? ::: devtools/shared/webconsole/screenshot-helper.js:271 (Diff revision 8) > + } > +}; > + > +/** > + * Save the screenshot data to disk, returning a promise which is resolved on > + * completion. could we add expected params and return value ? ::: devtools/shared/webconsole/screenshot-helper.js:296 (Diff revision 8) > + const targetFile = new FileUtils.File(filename); > + const targetFileURI = Services.io.newFileURI(targetFile); > + > + // Create download and track its progress. > + // This is adapted from saveURL in contentAreaUtils.js, but simplified greatly > + // and modified to allow saving to arbitrary paths on disk. Using these nit: can we have a simple space after the "." ?

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review259946 Something I just noticed: if you open the console and start to type "screens", there is the autocomplete for "screenshot", and when you hit tab and then enter, it says that screenshot is not defined. I think screenshot shouldn't appear in the autocompletion suggestions

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review259948

Comment on attachment 8987489 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/252742/#review259954 If I navigate to `data:text/html,<meta charset=utf8><script>function screenshot() {console.log("screen")}</script>` and evaluate `:screenshot` in the console, I get the "screen" log printed in the output, and not a screenshot as I would expect.

Comment on attachment 8987489 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/252742/#review259956 The mochitests fails on my machine: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command.js FAIL Clipboard: Image width matches window size - 1367 == 1382 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:4 resource://testing-common/content-task.js:null:60 FAIL Clipboard: Image height matches window size - 538 == 553 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:6 resource://testing-common/content-task.js:null:60 FAIL Fullpage Clipboard: Image width matches page size - 2742 == 2757 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:4 resource://testing-common/content-task.js:null:60 FAIL Fullpage Clipboard:Image height matches page size - 1092 == 1107 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:7 resource://testing-common/content-task.js:null:60 FAIL Scroll Clipboard: Image width matches window size minus scrollbar size - 1367 == 1382 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:4 resource://testing-common/content-task.js:null:60 FAIL Scroll Clipboard: Image height matches window size minus scrollbar size - 538 == 25 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:6 resource://testing-common/content-task.js:null:60 FAIL Scroll Fullpage Clipboard: Image width matches page size minus scrollbar size - 2742 == 5492 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:4 resource://testing-common/content-task.js:null:60 FAIL Scroll Fullpage Clipboard: Image height matches page size minus scrollbar size - 1092 == 123 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:8 resource://testing-common/content-task.js:null:60 Also, it would be nice to have all the commits related to the mochitests in only one changeset so it's easier to review

Comment on attachment 8987857 [details] Bug 1464461 - experiemental rewrite of evalWithDebugger p1; https://reviewboard.mozilla.org/r/253132/#review259958 It feels a lot cleaner, thanks yulia. I mostly have cosmetic comments, so it's good to go with a green try. ::: devtools/server/actors/webconsole/eval-with-debugger.js:54 (Diff revision 1) > * @param string string > * String to evaluate. > * @param object [options] now there's also the webconsole param ::: devtools/server/actors/webconsole/eval-with-debugger.js:84 (Diff revision 1) > - helpers.selectedNode = actor.rawNode; > - } > - } > > - // Check if the Debugger.Frame or Debugger.Object for the global include > - // $ or $$. We will not overwrite these functions with the Web Console > +exports.evalWithDebugger = function(string, options = {}, webConsole) { > + dump(`\n\nHI\n\n`); :) ::: devtools/server/actors/webconsole/eval-with-debugger.js:125 (Diff revision 1) > + frame, > + window: dbgWindow, > + }; > +}; > + > +function getEvalResult(string, evalOptions, bindings, frame, dbgWindow) { could you add some jsdoc ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:139 (Diff revision 1) > + parseErrorOutput(dbgWindow, string); > + } > + return result; > +} > + > +function parseErrorOutput(dbgWindow, string) { could you add some jsdoc ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:147 (Diff revision 1) > - // since it's already being handled elsewhere and we are only interested > + // since it's already being handled elsewhere and we are only interested > - // in initializing bindings. > + // in initializing bindings. > - try { > + try { > - ast = Parser.reflectionAPI.parse(string); > + ast = Parser.reflectionAPI.parse(string); > - } catch (ex) { > + } catch (ex) { > - ast = {"body": []}; > + return; what does this change do ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:200 (Diff revision 1) > - const helperResult = helpers.helperResult; > - delete helpers.evalInput; > - delete helpers.helperResult; > - delete helpers.selectedNode; > > - if ($) { > +function updateConsoleInputEvaluation(dbg, dbgWindow, webConsole) { jsdoc would be nice here :) ::: devtools/server/actors/webconsole/eval-with-debugger.js:213 (Diff revision 1) > - } > + } > - if (screenshot) { > - bindings.screenshot = screenshot; > - } > +} > > - if (bindings._self) { > +function getEvalInput(string) { could you add some jsdoc ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:237 (Diff revision 1) > - s = s.parent; > - } > + } > + return string; > - } > +} > - let lineText = pageError.sourceLine; > - if (lineText && lineText.length > DebuggerServer.LONG_STRING_INITIAL_LENGTH) { > + > +function getFrameDbg(options, webConsole) { nit: could we put options as the second arg ? I think it is more common in the codebase, plus it allows terser function call if we don't need to pass any options Also, can we add some jsdoc to this function ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:238 (Diff revision 1) > - } > + } > + return string; > - } > +} > - let lineText = pageError.sourceLine; > - if (lineText && lineText.length > DebuggerServer.LONG_STRING_INITIAL_LENGTH) { > - lineText = lineText.substr(0, DebuggerServer.LONG_STRING_INITIAL_LENGTH); > + > +function getFrameDbg(options, webConsole) { > + if (!options.frameActor) { Could we guard with options so we don't throw if options is not an object ::: devtools/server/actors/webconsole/eval-with-debugger.js:256 (Diff revision 1) > - } > + } > + return DevToolsUtils.reportException("evalWithDebugger", > + Error("The frame actor was not found: " + options.frameActor)); > - } > +} > > - return { > +function getDbgWindow(options, dbg, webConsole) { could you add some jsdoc ? also, could we have options be the last argument as it's a more common pattern through the codebase ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:279 (Diff revision 1) > - for (const {name, value} of headers) { > - channel.setRequestHeader(name, value, false); > + if (!isObject(jsVal)) { > + return { bindSelf: jsVal, dbgWindow }; > - } > + } > > - if (body) { > - channel.QueryInterface(Ci.nsIUploadChannel2); > + // If we use the makeDebuggeeValue method of jsVal's own global, then > + // we'll get a D.O that sees jsVal as viewed from its own compartment - nit: I don't know what's a D.O. , if you happen to know, could you replace this with the full name ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:286 (Diff revision 1) > + const _dbgWindow = dbg.makeGlobalObjectReference(global); > + return { bindSelf, dbgWindow: _dbgWindow }; nit: could we rename _dbgWindow to dbgWindow ? This would make the return object a bit terser ::: devtools/server/actors/webconsole/eval-with-debugger.js:289 (Diff revision 1) > + const global = Cu.getGlobalForObject(jsVal); > + try { > + const _dbgWindow = dbg.makeGlobalObjectReference(global); > + return { bindSelf, dbgWindow: _dbgWindow }; > + } catch (err) { > + // The above will throw if `global` is invisible to debugger. shouldn't we log something to the browser console here ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:295 (Diff revision 1) > + } > + } > + return { bindSelf, dbgWindow }; > - } > +} > > - NetUtil.asyncFetch(channel, () => {}); > +function getHelpers(dbgWindow, options, webConsole) { could we add some jsdoc for this function and put options as the last parameter ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:298 (Diff revision 1) > - } > +} > > - NetUtil.asyncFetch(channel, () => {}); > - > - const actor = this.getNetworkEventActor(channel.channelId); > - > +function getHelpers(dbgWindow, options, webConsole) { > + // Get the Web Console commands for the given debugger window. > + const helpers = webConsole._getWebConsoleCommands(dbgWindow); > + if (options.selectedNodeActor) { should we guard options ? ::: devtools/server/actors/webconsole/eval-with-debugger.js:305 (Diff revision 1) > - // End of event handlers for various listeners. > + function cleanupHelpers() { > + delete helpers.evalInput; > + delete helpers.helperResult; > + delete helpers.selectedNode; > + } maybe we could not declare this function everytime ? Also, I seem to recall delete being slow, so maybe we could recreate a new object instead of modifying helpers (so we have a more functionnal style) ? Something like: ```js function getCleanedUpHelpers(helpers) { const {evalInput, helperResult, selectedNode, ...props} = helpers; return props; } ``` ::: devtools/server/actors/webconsole/eval-with-debugger.js:322 (Diff revision 1) > + bindings._self = bindSelf; > + } > + // Check if the Debugger.Frame or Debugger.Object for the global include > + // $ or $$. We will not overwrite these functions with the Web Console > + // commands. > + const overrides = ["$", "$$", "screenshot"]; I wonder if screenshot is still needed here since we don't expose screenshot() but only :screenshot ::: devtools/server/actors/webconsole/eval-with-debugger.js:339 (Diff revision 1) > - result.styles = Array.map(message.styles || [], (string) => { > - return this.createValueGrip(string); > + function cleanupBindings() { > + Object.entries(backups).forEach(([name, value]) => { > + bindings[name] = value; > }); > - > - result.category = message.category || "webdev"; > + if (bindings._self) { > + delete bindings._self; > - > - return result; > - }, > - > - /** > - * Find the XUL window that owns the content window. > - * > - * @return Window > - * The XUL window that owns the content window. > - */ > - chromeWindow: function() { > - let window = null; > - try { > - window = this.window.QueryInterface(Ci.nsIInterfaceRequestor) > - .getInterface(Ci.nsIWebNavigation).QueryInterface(Ci.nsIDocShell) > - .chromeEventHandler.ownerGlobal; > - } catch (ex) { > - // The above can fail because chromeEventHandler is not available for all > - // kinds of |this.window|. > } could we mode cleanupBindings out of this function and take bindings and backups as arguments ? And maybe we could make it return a new object instead of modifying what is passed.

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review259716 > can we have a case where we save both to disk and clipboard ? if not, maybe we could only return the result and not an array. > > If we can have both, maybe we could not await each time and push the promises to the array, and return Promise.all(results) ? This way the 2 functions can run "in parallel" . we can have this case. I originally had the Promise.all, however alex recommended doing it this way for legibility, since this case is rare and there is not much of a performance loss > why don't we reject when this occurs ? we would handle the catch the exact same way as the resolve (pass a message to the console), and it would take more effort without adding any clarity. At least i think this was the original intention of the code. > why don't we reject when this occurs ? we would handle the catch the exact same way as the resolve (pass a message to the console), and it would take more effort without adding any clarity. At least i think this was the original intention of the code.

Comment on attachment 8987489 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/252742/#review259956 this patch shouldn't have those there. sorry, must have been a problem during rebasing. this set of patches has had a lot of revisions

Comment on attachment 8987489 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/252742/#review259954 I can't reproduce this. I am not really sure how it is possible with line 1393 in webconsole.js, which guards against this case. But, there have been a lot of revisions to this code, are you using the most recent patch? I did notice that there was an issue with the "foundScreenshot" code, that should be fixed now

Comment on attachment 8983096 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/248944/#review260358 ::: devtools/client/webconsole/components/JSTerm.js:317 (Diff revision 9) > break; > + case "screenshotOutput": > + const { args, value } = helperResult; > + const results = await processScreenshot(this.hud.window, args, value); > + this.screenshotNotify(results); > + // early return as screenshot notify has dispatched all necessary messages nit: s/screenshot notify/screenshotNotify ::: devtools/server/actors/webconsole.js:905 (Diff revision 9) > > + this._waitForHelperResultAndSend(response); > + }, > + > + /** > + * In order to have asynchornous commands such as screenshot, we have to be s/asynchornous/asynchronous

Comment on attachment 8988452 [details] Bug 1464461 - make dispatchMessagesAdd use batching; https://reviewboard.mozilla.org/r/253728/#review260374 This looks good to me, thanks !

Comment on attachment 8987489 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/252742/#review260376 I only have a question about foundScreenshot and a typo, thanks :) ::: devtools/server/actors/webconsole.js:1430 (Diff revision 3) > - } > + } > - } else { > + } else { > - found$ = !!dbgWindow.getOwnPropertyDescriptor("$"); > + found$ = !!dbgWindow.getOwnPropertyDescriptor("$"); > - found$$ = !!dbgWindow.getOwnPropertyDescriptor("$$"); > + found$$ = !!dbgWindow.getOwnPropertyDescriptor("$$"); > - } > + } > + foundScreenshot = true; it doesn't look like we check if screenshot is defined right ? ::: devtools/server/actors/webconsole/commands.js:121 (Diff revision 3) > + const { value, offset } = collectString(nextToken, tokens, nextTokenIndex); > + // in order for JSON.stringify to correctly output values, they must be correctly > + // typed > + // As per the GCLI documentation, we can only have one value associated with a > + // flag but multiple flags with the same name can exist and should be combined > + // into and array. Here we are associating only the value on the right hand s/into and/into an

> Something I just noticed: if you open the console and start to type "screens", there is the autocomplete for "screenshot", and when you hit tab and then enter, it says that screenshot is not defined. Fixed :) > If I navigate to `data:text/html,<meta charset=utf8><script>function screenshot() {console.log("screen")}</script>` and evaluate `:screenshot` in the console, I get the "screen" log printed in the output, and not a screenshot as I would expect. Also fixed (it would be nice defining such function in the test support file to make sure we don't regress this)

Comment on attachment 8988539 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/253784/#review260442 Thanks Yulia, it looks good to me. I don't know if you tried, but it even works in the browser console and browser toolbox :) ::: devtools/server/actors/webconsole.js:901 (Diff revision 1) > > // Then, execute the script that may pause. > const response = this.evaluateJS(request); > response.resultID = resultID; > > + this._waitForHelperResultAndSend(response); As discussed on Slack, you should use .catch here to ensure printing exception/rejections happening in this function. ::: devtools/server/actors/webconsole/utils.js:609 (Diff revision 1) > + owner.helperResult = (async () => { > + const value = await screenshot(owner, args); > + return { > + type: "screenshotOutput", > + value, > + args You may add a comment here explaining why you pipe args back to the client as it is a bit surprising. ::: devtools/shared/webconsole/screenshot-helper.js:115 (Diff revision 1) > - name: "screenshot_server", > - hidden: true, > - returnType: "imageSummary", > - params: [ > - filenameParam, > - serverScreenshotParams, > + * was called. > + * @param object value > + * an object with a image value and file name > + * > + * @param object window > + * The Debugger Client window. The arguments order should match documentation order. This isn't the Debugger Client window but the Webconsole window. Please fix that here and in other places where you document the window argument. ::: devtools/shared/webconsole/screenshot-helper.js:181 (Diff revision 1) > /** > * Save the image data to the clipboard. This returns a promise, so it can > - * be treated exactly like imgur / file processing, but it's really sync > - * for now. > + * be treated exactly like file processing. > + * > + * @param object window > + * The Debugger Client window. Same comment. ::: devtools/shared/locales/en-US/screenshot.properties:18 (Diff revision 1) > +# documentation on web development on the web. > + > +# LOCALIZATION NOTE (screenshotDesc) A very short description of the > +# 'screenshot' command. See screenshotManual for a fuller description of what > +# it does. This string is designed to be shown in a menu alongside the > +# command name, which is why it should be as short as possible. I imagine we can rewrite l10n keys description to better match what happens in the console

Comment on attachment 8988541 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/253788/#review260448 ::: devtools/server/actors/webconsole.js:1430 (Diff revision 1) > - } > + } > - } else { > + } else { > - found$ = !!dbgWindow.getOwnPropertyDescriptor("$"); > + found$ = !!dbgWindow.getOwnPropertyDescriptor("$"); > - found$$ = !!dbgWindow.getOwnPropertyDescriptor("$$"); > + found$$ = !!dbgWindow.getOwnPropertyDescriptor("$$"); > - } > + } > + foundScreenshot = true; A comment for this line would help, I think another one before `if (!isCmd)` would also help following what is going on here. ::: devtools/server/actors/webconsole/commands.js:72 (Diff revision 1) > + */ > +function createToken(string) { > + if (isCommand(string)) { > + const value = string.replace(COMMAND_PREFIX, ""); > + if (!value || !validCommands.includes(value)) { > + throw Error(`'${value}' is not a valid command`); You are not testing that in your unit test ::: devtools/server/actors/webconsole/commands.js:125 (Diff revision 1) > + // flag but multiple flags with the same name can exist and should be combined > + // into and array. Here we are associating only the value on the right hand > + // side if it is of type `arg` as a single value; the second case initializes > + // an array, and the final case pushes a value to an existing array > + const typedValue = getTypedValue(value); > + if (!values || values === DEFAULT_VALUE) { Can `values` ever be falsy here? ::: devtools/server/actors/webconsole/commands.js:131 (Diff revision 1) > + values = typedValue; > + } else if (!Array.isArray(values)) { > + values = [values, typedValue]; > + } else { > + values.push(typedValue); > + } Would it be simplier to read if we do that on line 114? let values = args[token.value] || [DEFAULT_VALUE]; and here: if (values[0] == DEFAULT_VALUE) { values = [typedValue]; } else { values.push(typedValue); } But I realize it would force args to always refer to arrays. ::: devtools/server/actors/webconsole/commands.js:184 (Diff revision 1) > + if (nextToken.type !== ARG) { > + throw Error(`String does not terminate before flag ${nextToken.value}`); > + } > + > + value = `${value} ${nextToken.value}`; > + if (checkLastChar(nextToken.value, firstChar)) { Shouldn't we also check that there is no unescaped `firstChar` in value? That, to look for misplaced quotes in middle of args. Like: :screenshot "fo"o bar ::: devtools/server/actors/webconsole/commands.js:205 (Diff revision 1) > +function getTypedValue(value) { > + if (!isNaN(value)) { > + return Number(value); > + } > + if (value === "true" || value === "false") { > + return Boolean(value); I'm not sure your are testing this codepath in your test? ::: devtools/server/tests/unit/test_format_command.js:65 (Diff revision 1) > + { input: ":screenshot \"file name", expectedError: "String does not terminate" }, > + { > + input: ":screenshot \"file name --clipboard", > + expectedOutput: "String does not terminate before flag \"clipboard\"" > + }, > + { input: "::screenshot", expectedOutput: "invalid command" } The two last should have expectedError instead of expectedOutput.

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252766/#review260472 Looks good to me, assuming you easily end up fixing the clipboard issue. ::: devtools/client/webconsole/test/mochitest/browser.ini:233 (Diff revision 4) > [browser_jsterm_popup_close_on_tab_switch.js] > [browser_jsterm_popup.js] > +[browser_jsterm_screenshot_command_copy.js] > +subsuite = clipboard > +[browser_jsterm_screenshot_command_file.js] > +skip-if = true # Bug 849168: screenshot command tests fail in try but not locally Do you have a try run where this is failing? (Just checking if this is also related to "L10N" require path that doesn't work on linux)

Comment on attachment 8988541 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/253788/#review260448 > Would it be simplier to read if we do that on line 114? > let values = args[token.value] || [DEFAULT_VALUE]; > and here: > if (values[0] == DEFAULT_VALUE) { > values = [typedValue]; > } else { > values.push(typedValue); > } > But I realize it would force args to always refer to arrays. I would rather not have everything treated as an array for now, as this group of patches has gone through a lot of iterations. Maybe we can do this in a second pass

Comment on attachment 8988541 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/253788/#review260676

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=371bf7de7cd0fecb4b6c15de3c4a05d8fe20c644

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252766/#review260680

There seems to be an issue with the windows run of the file save test, however since this test was disabled for quite some time, I do not think this is a blocker. I have updated the browser.ini to not run the screenshot file test on windows.

Comment on attachment 8988540 [details] Bug 1464461 - make dispatchMessagesAdd use batching; https://reviewboard.mozilla.org/r/253786/#review260950

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252766/#review260388 I get command_copy to fail when lauching it with --verify: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js FAIL Scroll Clipboard: Image height matches window size minus scrollbar size - 538 == 10 - JS frame :: chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js :: testClipboardScrollbar :: line 41 Stack trace: chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:testClipboardScrollbar:41 chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:null:22 chrome://mochikit/content/browser-test.js:Tester_execTest/<:1098 chrome://mochikit/content/browser-test.js:Tester_execTest:1089 chrome://mochikit/content/browser-test.js:nextTest/<:986 chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795 ::: devtools/client/webconsole/test/mochitest/browser.ini:230 (Diff revision 3) > [browser_jsterm_no_input_and_tab_key_pressed.js] > [browser_jsterm_no_input_change_and_tab_key_pressed.js] > [browser_jsterm_null_undefined.js] > [browser_jsterm_popup_close_on_tab_switch.js] > [browser_jsterm_popup.js] > +[browser_jsterm_screenshot_command_copy.js] nit: could we append _clipboard at the end of the file ? ::: devtools/client/webconsole/test/mochitest/browser.ini:233 (Diff revision 6) > [browser_jsterm_popup_close_on_tab_switch.js] > [browser_jsterm_popup.js] > +[browser_jsterm_screenshot_command_copy.js] > +subsuite = clipboard > +[browser_jsterm_screenshot_command_file.js] > +skip-if = (os == 'win') # Bug 1464461, disabled on Win due to timeouts could we remove this until we do find there's timeouts on Win ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:4 (Diff revision 6) > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +/* global helpers, btoa, whenDelayedStartupFinished, OpenBrowserWindow */ I think this can be removed ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:16 (Diff revision 6) > + "test/mochitest/test_jsterm_screenshot_command.html"; > + > +// on some machines, such as macOS, dpr is set to 2. This is expected behavior, however > +// to keep tests consistant across OSs we are setting the dpr to 1 > +const dpr = "--dpr 1"; > +// const isOSX = Services.appinfo.OS === "Darwin"; nit: Should we remove this ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:33 (Diff revision 6) > + > +async function testClipboardScrollbar(hud) { > + await createScrollbarOverflow(); > + const command = `:screenshot --clipboard ${dpr}`; > + hud.jsterm.execute(command); > + await waitForMessage(hud, "Screenshot copied to clipboard."); we should assign this to a variable before executing the command and then wait (like we do in testFullPOageClipboardScrollbar) ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:38 (Diff revision 6) > + imgSize.scrollbarWidth = scrollbarSize.width; > + imgSize.scrollbarHeight = scrollbarSize.height; do we need to put scrollbarWith/Height on imgSize ? Or can we have separeted variables for that ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:41 (Diff revision 6) > + const scrollbarSize = await getScrollbarSize(); > + > + imgSize.scrollbarWidth = scrollbarSize.width; > + imgSize.scrollbarHeight = scrollbarSize.height; > + > + Assert.equal(imgSize.width, contentSize.innerWidth - imgSize.scrollbarWidth, one question here: why don't we use `is` and `ok` like in other tests ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:57 (Diff revision 6) > + imgSize.scrollbarWidth = scrollbarSize.width; > + imgSize.scrollbarHeight = scrollbarSize.height; same question as before, can we declare scrollbarWidth/Height as independent variables instead of putting them in imgSize ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:70 (Diff revision 6) > +async function createScrollbarOverflow() { > + // Trigger scrollbars by forcing document to overflow > + // This only affects results on OSes with scrollbars that reduce document size > + // (non-floating scrollbars). With default OS settings, this means Windows > + // and Linux are affected, but Mac is not. For Mac to exhibit this behavior, > + // change System Preferences -> General -> Show scroll bars to Always. > + await ContentTask.spawn(gBrowser.selectedBrowser, {}, function* () { > + content.document.body.classList.add("overflow"); > + }); > +} I may be wrong but it looks like calling this function is one of the first thing we do (in testClipboardScrollbar), and we don't ever remove this css class. So if this is the state we have for every assertion we make in this test, maybe we should call this function only once, before executing the test* functions (l.24) ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:76 (Diff revision 6) > + // Trigger scrollbars by forcing document to overflow > + // This only affects results on OSes with scrollbars that reduce document size > + // (non-floating scrollbars). With default OS settings, this means Windows > + // and Linux are affected, but Mac is not. For Mac to exhibit this behavior, > + // change System Preferences -> General -> Show scroll bars to Always. > + await ContentTask.spawn(gBrowser.selectedBrowser, {}, function* () { nit: The function does not need to be a generator ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:136 (Diff revision 6) > + innerHeight: content.innerHeight > + }; > + } > + ); > + > + info(`Scrollbar size: ${contentSize.innerWidth}x${contentSize.innerHeight}`); nit: s/Scrollbar size/Content size (I guess) ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:150 (Diff revision 6) > + const data = new Object(); > + const dataLength = new Object(); why new Object instead of literals ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:187 (Diff revision 6) > + dataURI = dataURI + encodedData; > + } else { > + throw new Error("Unable to read image data"); > + } > + > + const img = document.createElementNS("http://www.w3.org/1999/xhtml", "img"); would it work to use document.createElement ? ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:189 (Diff revision 6) > + const loaded = new Promise(resolve => { > + img.addEventListener("load", function() { > + resolve(); > + }, {once: true}); > + }); I think there's a `once` helper [1] we can use: ```js const loaded = once(img, "load"); ``` [1] https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/devtools/client/shared/test/shared-head.js#357 ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_file.js:4 (Diff revision 6) > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +/* global helpers, btoa, whenDelayedStartupFinished, OpenBrowserWindow */ this can be removed I think ::: devtools/server/actors/webconsole/screenshot.js:9 (Diff revision 6) > > "use strict"; > > const { Ci, Cu } = require("chrome"); > const { getRect } = require("devtools/shared/layout/utils"); > -const { LocalizationHelper } = require("devtools/shared/L10N"); > +const { LocalizationHelper } = require("devtools/shared/l10n"); not a big deal, but this would be perfect if this change was added in the patch that introduces the screenshot command

Comment on attachment 8988539 [details] Bug 1464461 - implement screenshot command in console panel; https://reviewboard.mozilla.org/r/253784/#review260964

Comment on attachment 8988541 [details] Bug 1464461 - implement unix style syntax for console commands; https://reviewboard.mozilla.org/r/253788/#review260966 ::: devtools/server/actors/webconsole.js:1129 (Diff revision 2) > - .filter(n => n.startsWith(result.matchProp))); > + .filter(n => > + // filter out `screenshot` command as it is inaccessible without > + // the `:` prefix > + n !== "screenshot" && n.startsWith(result.matchProp) great. It would be nice to have a test making sure we don't regress this

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252766/#review260992 Let's handle changes in follow-ups

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252766/#review260388 this is fixed > would it work to use document.createElement ? I checked, and no -- not sure why though.

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7dbd719046016090be052702c9d133780e52375

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; https://reviewboard.mozilla.org/r/252766/#review261028 ::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_copy.js:187 (Diff revision 6) > + dataURI = dataURI + encodedData; > + } else { > + throw new Error("Unable to read image data"); > + } > + > + const img = document.createElementNS("http://www.w3.org/1999/xhtml", "img"); That's because mochitests are evaluated in the scope of browser.xul document. i.e. the top level document of Firefox. So that, document is having XUL namespace by default. I don't know if XUL <image> supports load event, but it might work with `document.createElement("image")`. Using createElementNS is fine here. You may use createElement("img") if you were using webconsole document object (that I imagine you could retrieve from `hud` and that is an HTML document) instead of document global.

new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10b7c4ebf7c3152b4e313154b50d89941d9efcca had some new test failures after the fix, everything is working again

Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6c9488ed5ebd implement screenshot command in console panel; r=nchevobbe,ochameau https://hg.mozilla.org/integration/autoland/rev/50f7f633bb7d make dispatchMessagesAdd use batching; r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/014fa1322e94 implement unix style syntax for console commands; r=nchevobbe,ochameau https://hg.mozilla.org/integration/autoland/rev/001370fe21e3 add back mochitests for screenshot command; r=nchevobbe,ochameau

Backed out for Talos damp failures on webconsole/objectexpand.js Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=186173107&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=damp&fromchange=001370fe21e34de588e40da2e140f2482acf6364&tochange=cf26f1c33da69097dc87a93d76d8d2a01b2c0193 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186173107&repo=autoland&lineNumber=915 Backout link: https://hg.mozilla.org/integration/autoland/rev/cf26f1c33da69097dc87a93d76d8d2a01b2c0193 05:12:28 INFO - PID 1721 | webconsole/streamlog.js took 3536ms. 05:12:28 INFO - PID 1721 | Loading test 'webconsole/objectexpand.js' 05:12:28 INFO - PID 1721 | Executing test 'webconsole/objectexpand.js' 05:12:29 ERROR - PID 1721 | TEST-UNEXPECTED-FAIL | damp | webconsole/objectexpand.js: TypeError: tree is null 05:12:29 INFO - PID 1721 | module.exports@chrome://damp/content/tests/webconsole/objectexpand.js:50:1 05:12:29 INFO - PID 1721 | async*_runNextTest@chrome://damp/content/damp.js:249:19 05:12:29 INFO - PID 1721 | testTeardown@chrome://damp/content/damp.js:207:5 05:12:29 INFO - PID 1721 | async*exports.testTeardown@chrome://damp/content/tests/head.js:46:10 05:12:29 INFO - PID 1721 | module.exports@chrome://damp/content/tests/webconsole/streamlog.js:54:9 05:12:29 INFO - PID 1721 | async*_runNextTest@chrome://damp/content/damp.js:249:19 05:12:29 INFO - PID 1721 | testTeardown@chrome://damp/content/damp.js:207:5 05:12:29 INFO - PID 1721 | async*exports.testTeardown@chrome://damp/content/tests/head.js:46:10 05:12:29 INFO - PID 1721 | module.exports@chrome://damp/content/tests/webconsole/bulklog.js:51:9 05:12:29 INFO - PID 1721 | async*_runNextTest@chrome://damp/content/damp.js:249:19 05:12:29 INFO - PID 1721 | testTeardown@chrome://damp/content/damp.js:207:5 05:12:29 INFO - PID 1721 | async*exports.testTeardown@chrome://damp/content/tests/head.js:46:10 05:12:29 INFO - PID 1721 | module.exports@chrome://damp/content/tests/debugger/custom.js:35:9 05:12:29 INFO - PID 1721 | async*_runNextTest@chrome://damp/content/damp.js:249:19 05:12:29 INFO - PID 1721 | testTeardown@chrome://damp/content/damp.js:207:5 05:12:29 INFO - PID 1721 | async*exports.testTeardown@chrome://damp/content/tests/head.js:46:10 05:12:29 INFO - PID 1721 | module.exports@chrome://damp/content/tests/inspector/custom.js:23:9 05:12:29 INFO - Terminating psutil.Process(pid=1721, name='firefox', started='05:10:15') 05:12:29 INFO - PID 1721 | async*_runNextTest@chrome://damp/content/damp.js:249:19 05:12:29 INFO - PID 1721 | testTeardown@chrome://damp/content/damp.js:207:5 05:12:29 INFO - PID 1721 | async*exports.testTeardown@chrome://damp/content/tests/head.js:46:10 05:12:29 INFO - PID 1721 | module.exports@chrome://damp/content/tests/webconsole/custom.js:23:9 05:12:29 INFO - PID 1721 | 05:12:29 INFO - PID 1721 | [GFX1-]: Receive IPC close with reason=AbnormalShutdown 05:12:29 INFO - PID 1721 | [GFX1-]: Receive IPC close with reason=AbnormalShutdown 05:12:29 INFO - PID 1721 | [Child 1726, Chrome_ChildThread] WARNING: pipe error: Broken pipe: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 726 05:12:29 INFO - PID 1721 | ** Unknown exception behavior: -2147483647 05:12:29 INFO - PID 1721 | ** Unknown exception behavior: -2147483647 05:12:29 INFO - PID 1721 | ** Unknown exception behavior: -2147483647

Comment on attachment 8989413 [details] Bug 1464461 - fix talos failure; https://reviewboard.mozilla.org/r/254492/#review261284 ::: commit-message-de0f6:1 (Diff revision 1) > +Bug 1464461 - fix talos failure; r=nchevobbe nit: Maybe add a sentence to indicate that simple.message is also logging a message and that a change in how we dispatch message introduced a race in the test.

Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1269a7d4b143 implement screenshot command in console panel; r=nchevobbe,ochameau https://hg.mozilla.org/integration/autoland/rev/f1f577e0d6f4 make dispatchMessagesAdd use batching; r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/e090de5269dc implement unix style syntax for console commands; r=nchevobbe,ochameau https://hg.mozilla.org/integration/autoland/rev/ea3556925d99 add back mochitests for screenshot command; r=nchevobbe,ochameau https://hg.mozilla.org/integration/autoland/rev/dc54472ca3bf fix talos failure; r=nchevobbe

There was a race in the talos test that was not noticed until we had batched groups of messages. this is now fixed thanks to nchevobbe's help!

https://hg.mozilla.org/mozilla-central/rev/1269a7d4b143 https://hg.mozilla.org/mozilla-central/rev/f1f577e0d6f4 https://hg.mozilla.org/mozilla-central/rev/e090de5269dc https://hg.mozilla.org/mozilla-central/rev/ea3556925d99 https://hg.mozilla.org/mozilla-central/rev/dc54472ca3bf

Needinfo not relevant anymore.

Have we considered uplifting this, considering that GCLI was removed in 62?

See comment 126.

Thanks for the ping, Alex! I guess if we uplift to the version where there's no GCLI at all, that's better than nothing, and it sounds good to me. If we uplift to a version with GCLI it might make things worse. But I'm not sure if the patches are going to apply cleanly. Alex, what do you think? If you deem it safe, we could request the uplift (I guess there's always a place for backing out the uplift if things break though).

Hey Harald, can we get your PM expertise here: is the screenshot feature worth an uplift to beta, so that the functionality that was removed along with GCLI is brought back? Do we have numbers to justify this? Thanks!

I'd like to hear the risk assessment from Alex. Feature-wise it makes sense to get it back in, but not critical enough to take major risks.

This one is special to assess: * while it adds l10n strings, they all are duplicated from gcli * while it adds a bunch of code to implement this feature, 90% of it comes from gcli. * this feature doesn't introduce a new complete "command" feature, it relies a lot on an existing one where we support "copy" for example. But instead of using inlined function, it introduced a parser that parse any input string that starts with ":" in the console input. I think Yulia and Nicolas would be better judges for this. * this code baked for a bit on Nightly. We got to disable a test on linux (bug 1473120), so at least that would have to be uplifted. Most of the bugs/feedback that we received were about existing limitation from gcli I think. * the part I'm more concerned about is the tweak made to dispatchMessagesAdd which introduced a regression that has been fixed but I can't find the related bug. Overall, I think that's a possible candidate for uplift. If something is broken, it was most likely already broken in the same way in gcli. The code to introduce this features should not regress the console as most of it is implemented/imported lazily, only when this feature is used (i.e. a input starts with ':'). My only concern is: did we really figured out completely the issues around dispatchMessagesAdd regression? That particular modification to this function is in a hot codepath of the console.

The regression that was introduced was fixed in this bug with this patch: https://bugzilla.mozilla.org/attachment.cgi?id=8989413. With regards to the dispatchMessagesAdd, I did some testing and it is not responsible for bug 1473120. We couldn't find any related code that was causing that failure so far. I also have a hard time reproducing it -- it occurs very infrequently when I am looking for it so I am not too sure how to narrow it down further. I would say that we are not taking a major risk in uplifting this

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; Approval Request Comment [Feature/Bug causing the regression]: 1461970 [User impact if declined]: Users cannot take screenshots in DevTools using a rich set of options, they're very limited and can only take viewport screenshots [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: All the patches associated to this bug [Is the change risky?]: No [Why is the change risky/not risky?]: We have essentially moved code around, to bring back a highly popular feature that was behind an insecure UI (see the explanation in more detail here https://bugzilla.mozilla.org/show_bug.cgi?id=1464461#c131 ). The code has been in Nightly for a while, all the feedback we've got is from people that want MORE features, but no bugs associated to this change. [String changes made/needed]: No, we copied the strings from the previous files

Comment on attachment 8988541 [details] Bug 1464461 - implement unix style syntax for console commands; Approval Request Comment [Feature/Bug causing the regression]: 1461970 [User impact if declined]: Users cannot take screenshots in DevTools using a rich set of options, they're very limited and can only take viewport screenshots [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: All the patches associated to this bug [Is the change risky?]: No [Why is the change risky/not risky?]: We have essentially moved code around, to bring back a highly popular feature that was behind an insecure UI (see the explanation in more detail here https://bugzilla.mozilla.org/show_bug.cgi?id=1464461#c131 ). The code has been in Nightly for a while, all the feedback we've got is from people that want MORE features, but no bugs associated to this change. [String changes made/needed]: No, we copied the strings from the previous files

Comment on attachment 8988539 [details] Bug 1464461 - implement screenshot command in console panel; Approval Request Comment [Feature/Bug causing the regression]: [User impact if declined]: [Is this code covered by automated tests?]: [Has the fix been verified in Nightly?]: [Needs manual test from QE? If yes, steps to reproduce]: [List of other uplifts needed for the feature/fix]: [Is the change risky?]: [Why is the change risky/not risky?]: [String changes made/needed]: Approval Request Comment [Feature/Bug causing the regression]: 1461970 [User impact if declined]: Users cannot take screenshots in DevTools using a rich set of options, they're very limited and can only take viewport screenshots [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: All the patches associated to this bug [Is the change risky?]: No [Why is the change risky/not risky?]: We have essentially moved code around, to bring back a highly popular feature that was behind an insecure UI (see the explanation in more detail here https://bugzilla.mozilla.org/show_bug.cgi?id=1464461#c131 ). The code has been in Nightly for a while, all the feedback we've got is from people that want MORE features, but no bugs associated to this change. [String changes made/needed]: No, we copied the strings from the previous files

Comment on attachment 8988540 [details] Bug 1464461 - make dispatchMessagesAdd use batching; Approval Request Comment [Feature/Bug causing the regression]: 1461970 [User impact if declined]: Users cannot take screenshots in DevTools using a rich set of options, they're very limited and can only take viewport screenshots [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: All the patches associated to this bug [Is the change risky?]: No [Why is the change risky/not risky?]: We have essentially moved code around, to bring back a highly popular feature that was behind an insecure UI (see the explanation in more detail here https://bugzilla.mozilla.org/show_bug.cgi?id=1464461#c131 ). The code has been in Nightly for a while, all the feedback we've got is from people that want MORE features, but no bugs associated to this change. [String changes made/needed]: No, we copied the strings from the previous files

Comment on attachment 8989413 [details] Bug 1464461 - fix talos failure; Approval Request Comment [Feature/Bug causing the regression]: 1461970 [User impact if declined]: Users cannot take screenshots in DevTools using a rich set of options, they're very limited and can only take viewport screenshots [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: All the patches associated to this bug [Is the change risky?]: No [Why is the change risky/not risky?]: We have essentially moved code around, to bring back a highly popular feature that was behind an insecure UI (see the explanation in more detail here https://bugzilla.mozilla.org/show_bug.cgi?id=1464461#c131 ). The code has been in Nightly for a while, all the feedback we've got is from people that want MORE features, but no bugs associated to this change. [String changes made/needed]: No, we copied the strings from the previous files

Liz, do we need to do anything else to request the uplift? I wonder if I missed something. Thank you!

Comment on attachment 8987511 [details] Bug 1464461 - add back mochitests for screenshot command; After reviewing the risk assessment in comment 131 and given that we have a month of Beta cycle remaining, I am ok to uplift this to Beta62.

Hi Andrei, this is a mini-feature DevTools feature we are uplifting in Beta62. Please flag any regressions/blockers promptly. If things don't look good from a focused test run, we'll consider backing it out. Thanks!

(In reply to Ritu Kothari (:ritu) from comment #140) > Hi Andrei, this is a mini-feature DevTools feature we are uplifting in > Beta62. Please flag any regressions/blockers promptly. If things don't look > good from a focused test run, we'll consider backing it out. Thanks! Understood. Forwarding ni? to Cornel and Bogdan, to make sure this is added to Release QA's priorities for this week.

https://hg.mozilla.org/releases/mozilla-beta/rev/ba2081206f9f https://hg.mozilla.org/releases/mozilla-beta/rev/f7804a512504 https://hg.mozilla.org/releases/mozilla-beta/rev/3f296e84fdc6 https://hg.mozilla.org/releases/mozilla-beta/rev/433e33451a43 https://hg.mozilla.org/releases/mozilla-beta/rev/42473b21092f

After an exploratory test session over the screenshots area for 62.0b14 build validation. Managed to check the screenshot command on Firefox 62.0b14 - win8.1, macos 10.12, Ubuntu 16.04LTS and 63.0a1 (2018-08-03) on win10. The screenshots are saved properly, however a few things are worth mentioning: - on 62 the path is written as: C:\\Users\\[username]\\Downloads\\Screen - for both versions(62,63), if you're fast enough you can save multiple screenshots under the same name; - in some cases, spamming the command does not trigger the save at all just posts it on the console; - trying the ":screenshot --delay type: 5" command flashes the screen, but doesn't save or post any errors in the console. Keep in mind that we didn't have time to fully test it, else we might have found a few more issues.

After an extended exploratory session over 3 main platforms (win10x64, Ubuntu 16.04 LTS, macOS 10.13) we’ve uncovered 24 extra issues: We've wrote them down in the following document: https://docs.google.com/document/d/1CLI129N8CeTC7aDnkcmNuCHlGejWxz4BmbT9d-0Zf5A/edit?usp=sharing - the most serious one (nr.5) on the following command: “:screenshot --dpr 10 --fullpage” crashes/freezes the browser; - some cases where the screenshot is not saved at all, even with the flash animation being played. - combination of options seemed to cause some issues; - we’ve also wrote down a couple of enhancement suggestions, which we believe would help the mini-feature get some extra love from users. Yulia, your feedback would be more than welcome for the potential issues we found. Also, feel free to contact us for any additional info or clarification.

thanks :cfogel for the fantastic QA work. With regards to the most serious issue, this is a carry over from the original GCLI behavior. So in terms of risk, we are not regressing the previously existing behavior. --- Questions - the item I am most concerned about is item 15, where we are getting an unopenable file format. I am not sure why this would happen, other than that the file is corrupted because it is too large but managed to save. Do you have any more information about this one? - I also checked item 19 and wasn't able to reproduce it -- it might be related to the large size issue (nr. 5) and the page i tested on wasn't large enough, do we have more information about this item? - item 21 isn't clear to me -- is the behavior that, while scrolled half way down on netflix, using the screenshot command takes you back to the top? --- Bugs I think we can open the following polish bugs: - screenshots with high DPRs / large scale images cause the browser to crash (covers items 2-5, 19?, also 6) this might be solved why limiting the max dpr, it would be very unusual for someone to want to take a dpr at 10 for example, as the default for most machines is 1, and for mac it is 2. - Screenshot command line issues (covers items 7, 8, 11, 18 & 22) We need more documentation, and some checking on the parameters that are being sent, and if the incorrect parameters are given, we should send back an error. The filename is expected, we only save pngs -- so this needs to be added to the docs - Screenshot selector issues (covers items 13,15,16,17) This is a little bit more mysterious, but it is likely that we can track it down with some STRs - Screenshot autocomplete issues (covers items 9, 10) - Screenshot fullpage (covers items 2) this is related to this issue, we might want to expand that one: https://bugzilla.mozilla.org/show_bug.cgi?id=1473895 - Screenshot delay issues (covers items 1, 23) - screenshot cross process (might be issues 17 & 20) this one is interesting, but i am not sure if it should impact videos and iframes. if we are going cross process to load an iframe then we might. we have an open bug for that here https://bugzilla.mozilla.org/show_bug.cgi?id=1474006

Hey there, it was a team effort in the end. Shoutout to :vlucaci and :asoncuntean. --- Regarding the questions: - n.15 - at this moment it's the only information we got about it. From what I've noticed selecting classes, only saves unique ones. That one is used in multiple places. Would that be relevant? - n.19 - for this one, only the --fullpage option was used(without the dpr or another one). As pages for that one we used Youtube, Reddit, Cnn, Pinterest, Facebook, Netflix . - n.21 - it doesn't matter where you are in page before the command. It doesn't reproduce on ffx63 on macOS and Ubuntu. Also it's not the simple command, it needs the extra options to be reproducible.

:cfogel thank you so much for opening the bugs. I have moved them to a dedicated meta bug #1483143. Let's continue any discussion regarding screenshots there, as this bug has become too large to easily navigate.

Documentation updated for #1464461, added to 62 release notes, and review requested. Added the command here: https://developer.mozilla.org/en-US/docs/Tools/Tips under general tips and added to the Web Console section of the same page. Added to Web Console Helpers as well. https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Helpers Finally, I added a description of the command to the release notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/62