Closed Bug 1385995 Opened 3 years ago Closed 2 years ago

Right click -> copy should copy the actual object

Categories

(DevTools :: Console, defect, P1)

55 Branch
defect

Tracking

(firefox57 wontfix, firefox58 verified)

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: gregtatum, Assigned: abhinav.koppula)

References

(Depends on 1 open bug)

Details

(Whiteboard: [reserve-console-html])

Attachments

(1 file)

Currently we don't copy the object itself when you right click and copy a console.log() Object.

STR
 * console.log({ ... some object with many keys ... })
 * Right click the "Object" word in the console
 * Choose "Copy" from the menu

Actual results
 * The clipboard contains the text from the DOM elements, e.g.

> !!! broken thread Object { name: "GeckoMain", processType: "tab", processStartupTime: 2096764.8125, 14 more… } profile-tree.js:303:4
 * Notice how this entry contains "14 more…"

Expected Results:

 * I would expect a full representation of the object to be in my clipbard, e.g. the same results as if:
  - Right click
  - Choose Store as global variable
  - Run `copy(temp0)`
This would be really helpful.
What is nice is that we have in each reps a reference to the actor of the object (`data-actor-id` , or something alike), even for nested objects. We only need a server function that accept an actor id and returns a stringified  version of the object ;with the same logic we already have for the `copy` command so we can deal with multiple type of objects.
We could then rename the "copy" entry to be more explicit ("copy the message"), and add a "copy object" one.
It would be even nicer if we could say "copy array|object|string|number".

There is kind-of similar issue in devtools-core to do it directly from the ObjectInspector https://github.com/devtools-html/devtools-core/issues/507 .
Flags: qe-verify+
Priority: -- → P3
Whiteboard: [console-html]
Whiteboard: [console-html] → [console-html] [triage]
QA Contact: iulia.cristescu
Whiteboard: [console-html] [triage] → [reserve-console-html]
Priority: P3 → P4
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Priority: P4 → P1
Hi Nicolas,
This was an adventurous one for me :)
I have created a mozreview-request for the above issue. I haven't added a test yet since I wanted to get a first round of review done to see if I was proceeding in the right direction.
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review189620

Hello abhinav, thank you so much for working on this, so many people wants this (myself included :) )
So I think your approach is valid, but we can do even more simple by re-using what's already in the code (i.e. `evaluateJSAsync`) so we don't have as much work to do.
So feel free to ignore all the comments on main.js and on the server since we might not need to add those.

The next step would be to create a test with a variety of objects to make sure that 1. we enable the context menu entry only when possible 2. we get the expected string in the clipboard.

::: devtools/client/locales/en-US/webconsole.properties:245
(Diff revision 1)
>  webconsole.menu.copy.accesskey=C
>  
> +# LOCALIZATION NOTE (webconsole.menu.copy.object.label)
> +# Label used for a context-menu item displayed for object/variable log. Clicking on it
> +# will copy the object/variable.
> +webconsole.menu.copy.object.label=Copy array|object|string|number

this might be a bit verbose. I was thinking about having a dynamic label given the variable you try to copy. I don't know if this is easily doable.

This would require to have multiple localization items here (for all the "supported" type), and display the correct one given the variable.

I just checked in the context menu code, and it seems we can't have dynamic labels. So I'd suggest for now to only name this entry `Copy object` since it suits everything, and maybe have a follow up bug for dynamic label (which is only a nice to have I think).

::: devtools/client/webconsole/jsterm.js:555
(Diff revision 1)
>  
>    /**
> +   * Copy the object/variable by invoking the server
> +   * which invokes the `copy(variable)` command and makes it
> +   * available in the clipboard
> +   * @param evalString - string which has the evaluation string `copy(varible)`

nit: s/varible/variable

::: devtools/client/webconsole/jsterm.js:562
(Diff revision 1)
> +    let deferred = defer();
> +
> +    let evalOptions = {
> +      bindObjectActor: options.bindObjectActor,
> +      selectedNodeActor: options.selectedNodeActor,
> +      selectedObjectActor: options.selectedObjectActor,
> +    };
> +
> +    this.webConsoleClient.copyObject(evalString, evalOptions).then((res) => {
> +      if (!res.error) {
> +        deferred.resolve(res);
> +      } else {
> +        deferred.reject(res);
> +      }
> +    });
> +    return deferred.promise;

We try to not use `defer` now. This was a util used when promise weren't supported natively. Now we can use plain promises or async/await.
For example here this code could be like:
```
  copyObject: function (evalString, options = {}) {
    let evalOptions = {
      bindObjectActor: options.bindObjectActor,
      selectedNodeActor: options.selectedNodeActor,
      selectedObjectActor: options.selectedObjectActor,
    };
    return this.webConsoleClient.copyObject(evalString, evalOptions);
  }
```

You'll notice that i don't deal with the rejection: the caller will handle it (and `webConsoleClient.copyObject` will reject if it needs to).

::: devtools/client/webconsole/jsterm.js:564
(Diff revision 1)
> +    let evalOptions = {
> +      bindObjectActor: options.bindObjectActor,
> +      selectedNodeActor: options.selectedNodeActor,
> +      selectedObjectActor: options.selectedObjectActor,
> +    };

maybe we could directly rename the `options` argument to `evalOptions` and not create this, since it already look like what we'll pass in copyObject.

::: devtools/client/webconsole/new-console-output/utils/context-menu.js:139
(Diff revision 1)
> +  // Copy message object.
> +  menu.append(new MenuItem({
> +    id: "console-menu-copy-object",
> +    label: l10n.getStr("webconsole.menu.copy.object.label"),
> +    accesskey: l10n.getStr("webconsole.menu.copy.object.accesskey"),
> +    // Disabled if there is no actor assocaited.

nit: s/assocaited/associated

::: devtools/client/webconsole/new-console-output/utils/context-menu.js:142
(Diff revision 1)
> +    label: l10n.getStr("webconsole.menu.copy.object.label"),
> +    accesskey: l10n.getStr("webconsole.menu.copy.object.accesskey"),
> +    // Disabled if there is no actor assocaited.
> +    disabled: !actor,
> +    click: () => {
> +      let evalString = `{

I think it would be better to only send `_self` to `jsterm.copyObject`, and do 
```
this.webConsoleClient.copyObject(`copy(${evalString}`)`, evalOptions)
```
in it.

::: devtools/client/webconsole/new-console-output/utils/context-menu.js:143
(Diff revision 1)
> +    accesskey: l10n.getStr("webconsole.menu.copy.object.accesskey"),
> +    // Disabled if there is no actor assocaited.
> +    disabled: !actor,
> +    click: () => {
> +      let evalString = `{
> +        copy(_self);

where does `_self` comes from ?

::: devtools/server/actors/webconsole.js:1029
(Diff revision 1)
>      };
>    },
>  
>    /**
> +   * Handler for the copyObject function called from client
> +   * @param object request - request sent by client

we should explcitely declare everything which can be in the request object

::: devtools/server/actors/webconsole.js:1032
(Diff revision 1)
> +    let resultID = Date.now();
> +    this.conn.send({
> +      from: this.actorID,
> +      resultID: resultID
> +    });

why do we send this ?

::: devtools/server/actors/webconsole.js:1046
(Diff revision 1)
> +    let evalInfo = this.evalWithDebugger(request.text, evalOptions);
> +    let evalResult = evalInfo.result;
> +    let helperResult = evalInfo.helperResult;

we could turn this into a shorter : 
```
let {
  evalResult, 
  helperResult
} = this.evalWithDebugger(request.text, evalOptions);
```

::: devtools/server/actors/webconsole.js:1059
(Diff revision 1)
> +    // Finally, send an unsolicited copyObjectResult packet with
> +    // the normal return value
> +    this.conn.sendActorEvent(this.actorID, "copyObjectResult", response);

can't we return `response` directly ?

::: devtools/shared/webconsole/client.js:355
(Diff revision 1)
> +  copyObject: function (command, options = {}) {
> +    let packet = {
> +      to: this._actor,
> +      type: "copyObject",
> +      text: command,
> +      bindObjectActor: options.bindObjectActor,
> +      frameActor: options.frameActor,
> +      url: options.url,
> +      selectedNodeActor: options.selectedNodeActor,
> +      selectedObjectActor: options.selectedObjectActor,
> +    };
> +    return new Promise((resolve, reject) => {
> +      this._client.request(packet, response => {
> +        // Null check this in case the client has been detached while waiting
> +        // for a response.
> +        if (this.pendingEvaluationResults) {
> +          this.pendingEvaluationResults.set(response.resultID, resp => {
> +            if (resp.error) {
> +              reject(resp);
> +            } else {
> +              resolve(resp);
> +            }
> +          });
> +        }
> +      });
> +    });
> +  },
> +
> +  /**
> +   * Response handler which responds to the event sent by server
> +   * once the copyObject request has been performed
> +   */
> +  onCopyObjectResult: function (notification, packet) {
> +    let onResponse = this.pendingEvaluationResults.get(packet.resultID);
> +    if (onResponse) {
> +      onResponse(packet);
> +      this.pendingEvaluationResults.delete(packet.resultID);
> +    } else {
> +      DevToolsUtils.reportException("onCopyObjectResult",
> +        "No response handler for an evaluateJSAsync result (resultID: " +
> +                                    packet.resultID + ")");
> +    }
> +  },

I think if on the server side we return the result object instead of sending a "copyObjectResult" result, we could simplify this a bit.

We could remove the onCopyObjectResult function, and return directly :
```
this._client.request(packet, response);
```
which i think will handle everything.

Or even simpler, in jsterm, call `webconsoleClient.evaluateJSAsync`, so we don't have to write a dedicated server function (and deal with server/client compatibility - aka traits), and will benefit from future fixes and improvements on it for free.
Attachment #8912838 - Flags: review?(nchevobbe) → review-
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review189620

Hi Nicolas,
Thanks for the review. Yes, the review-request could have been a lot simpler.
I have removed the unnecessary server methods and shared/client methods as well and have added a test for the functionality.
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review190066

A handful of comments on that, but after addressing those, it should be good to land !
Thanks again abhinav

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js:13
(Diff revision 2)
> +
> +const TEST_URI = `data:text/html;charset=utf-8,<script>
> +  window.bar = { baz: 1 };
> +  console.log("foo");
> +  console.log("foo", window.bar);
> +  console.log(["foo", window.bar, 2]);

i think we should also test other types like : 
console.log(1)
console.log(true)
console.log(false)
console.log(undefined)
console.log(null)

Also, but this could be a follow up, we should decide what to return for non-obviously serializable object like :

(() => {})
Promise.resolve("foo")
Promise.reject("foo")
new Promise(res => {})
Symbol("foo")
new Uint8Array(1e3)

but I guess this could be done in the server. Maybe we could even pass the text content of the dom object representing the variable, so when the server does not know how top serialize a variable, it can use that as a fallback.
Let's file a bug after this one to handle those cases.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js:71
(Diff revision 2)
> +  yield waitForClipboardPromise(
> +    () => copyObjectMenuItem.click(),
> +    data => {
> +      clipboardText = data;
> +      // remove all whitespace and new line characters
> +      data = data.replace(NEW_LINE_SPACE_REGEX, "");
> +      return data === expectedMessage;
> +    }
> +  );
> +
> +  info("`Copy object` by using the access-key O");
> +  menuPopup = yield openContextMenu(hud, element);
> +  yield waitForClipboardPromise(
> +    () => synthesizeKeyShortcut("O"),
> +    data => {
> +      clipboardText = data;
> +      // remove all whitespace and new line characters
> +      data = data.replace(NEW_LINE_SPACE_REGEX, "");
> +      return data === expectedMessage;
> +    }
> +  );

maybe we can simplify that by declaring the callback and using it in both case, like 
```js
const cb = data => is(data, expectedMessage, "The clipboard has the expected data")
yield waitForClipboardPromise(() => copyObjectMenuItem.click(), cb);
yield waitForClipboardPromise(() => synthesizeKeyShortcut("O"), cb);

```

here you can see that i removed `clipboardText = data;` because even if you return it, we don't use it in the main function. And also I don't think therer were any assertion on that result I think.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js:75
(Diff revision 2)
> +      // remove all whitespace and new line characters
> +      data = data.replace(NEW_LINE_SPACE_REGEX, "");

do we need to do that ? it would be nice if we could check that the message is indeed pretty printed.

::: devtools/client/webconsole/new-console-output/utils/context-menu.js:142
(Diff revision 2)
> +    label: l10n.getStr("webconsole.menu.copy.object.label"),
> +    accesskey: l10n.getStr("webconsole.menu.copy.object.accesskey"),
> +    // Disabled if there is no actor associated.
> +    disabled: !actor,
> +    click: () => {
> +      jsterm.copyObject(`_self`, { selectedObjectActor: actor }).then((res) => {

can you add a comment explaining what `_self` is ?
Attachment #8912838 - Flags: review?(nchevobbe) → review-
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review190066

> i think we should also test other types like : 
> console.log(1)
> console.log(true)
> console.log(false)
> console.log(undefined)
> console.log(null)
> 
> Also, but this could be a follow up, we should decide what to return for non-obviously serializable object like :
> 
> (() => {})
> Promise.resolve("foo")
> Promise.reject("foo")
> new Promise(res => {})
> Symbol("foo")
> new Uint8Array(1e3)
> 
> but I guess this could be done in the server. Maybe we could even pass the text content of the dom object representing the variable, so when the server does not know how top serialize a variable, it can use that as a fallback.
> Let's file a bug after this one to handle those cases.

I have updated the test and enhanced the functionality to test primitive objects as well.
Can we have a separate followup-bug for non-obviously serializable objects?
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review190066

> maybe we can simplify that by declaring the callback and using it in both case, like 
> ```js
> const cb = data => is(data, expectedMessage, "The clipboard has the expected data")
> yield waitForClipboardPromise(() => copyObjectMenuItem.click(), cb);
> yield waitForClipboardPromise(() => synthesizeKeyShortcut("O"), cb);
> 
> ```
> 
> here you can see that i removed `clipboardText = data;` because even if you return it, we don't use it in the main function. And also I don't think therer were any assertion on that result I think.

I have made the above mentioned changes but I couldn't have this assertion `is(data, expectedMessage, "The clipboard has the expected data")` since it keeps waiting for a certain duration and checks if the clipboard text is equal to the data in that duration. For the first few times, the clipboard text is `waitForClipboard-known-value` and hence it breaks the assertion.
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review190066

> can you add a comment explaining what `_self` is ?

Taken this (https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/jsterm.js#508-509) as reference.
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review190944

I think that this is a good. I have some style comments, but nothing that would require another review, so you can make the changes and push to review again so we'll be able to push to try and then autoland.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js:21
(Diff revision 3)
> +const expectedMessages = [
> +    `foo`,
> +    `{"baz":1}`,
> +    `["foo",{"baz":1},2]`,
> +    `{"baz":1}`,
> +    `532`,
> +    `true`,
> +    `false`,
> +    `undefined`,
> +    `null`
> +];

i'm not sure if it helps to have this array, since in the test we refer to it and have to go up and down to follow the test.
I'd rather having the expected result directly when we call copyObject (e.g. `yield copyObject(hud, text, "foo", false)`)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js:36
(Diff revision 3)
> +  let [msgWithText, msgWithObj, msgNested] =
> +    yield waitFor(() => findMessages(hud, "foo"));
> +  ok(msgWithText && msgWithObj && msgNested, "Three messages should have appeared");
> +
> +  let [numberMsgObj] = yield waitFor(() => findMessages(hud, `532`, ".message-body"));
> +  ok(numberMsgObj, "One message with number 532 should have appeared");
> +
> +  let [trueMsgObj] = yield waitFor(() => findMessages(hud, `true`, ".message-body"));
> +  ok(trueMsgObj, "One message with true value should have appeared");
> +
> +  let [falseMsgObj] = yield waitFor(() => findMessages(hud, `false`, ".message-body"));
> +  ok(falseMsgObj, "One message with false value should have appeared");
> +
> +  let [undefinedMsgObj] = yield waitFor(() => findMessages(hud, `undefined`,
> +    ".message-body"));
> +  ok(undefinedMsgObj, "One message with undefined value should have appeared");
> +
> +  let [nullMsgObj] = yield waitFor(() => findMessages(hud, `null`, ".message-body"));
> +  ok(nullMsgObj, "One message with null value should have appeared");

Maybe we could only wait for the `null` message since it's the last one, and then retrieve the other ones by querying the console output ?
Here we should only get the messages, not assert that they are indeed shown (there are specific tests for that).

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js:103
(Diff revision 3)
> +  info("Check `Copy object` is enabled for undefined and null");
> +  yield copyObject(hud, undefinedMsgObj, expectedMessages[7], false);
> +  yield copyObject(hud, nullMsgObj, expectedMessages[8], false);
> +});
> +
> +function* copyObject(hud, element, expectedMessage, objectInput) {

nit: could this be renamed to `testCopyObject` instead ? it would better convey what it does

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js:110
(Diff revision 3)
> +  const cb = data => {
> +    let prettifiedMessage = prettyPrintMessage(expectedMessage, objectInput);
> +    return data === prettifiedMessage;
> +  };
> +
> +  info("Click on `Copy object`");
> +  yield waitForClipboardPromise(() => copyObjectMenuItem.click(), cb);

okay, so i didn't know `waitForClipboardPromise` was doing the assertion.
Could you rename `cb` to `validatorFn` (or something alike), so we can understand this better ?

::: devtools/client/webconsole/new-console-output/utils/context-menu.js:30
(Diff revision 3)
> + *        - {String} variableText (optional) variable which has been entered
> + *            for evaluation

nit: say that it is the textual frontend representation of the variable

::: devtools/client/webconsole/new-console-output/utils/context-menu.js:146
(Diff revision 3)
> +    accesskey: l10n.getStr("webconsole.menu.copy.object.accesskey"),
> +    // Disabled if there is no actor and no variable text associated.
> +    disabled: (!actor && !variableText),
> +    click: () => {
> +      if (actor) {
> +        // The Debugger.Object of the OA will be bound to |_self| during evaluation,

great
Attachment #8912838 - Flags: review?(nchevobbe) → review+
I tested the patch and there's some edge cases:
- `copy object` is enabled on `console.group("group")` messages
- on `inspect({a: 1})`, if I click on the `a` property in the expanded object, he context menu is disabled
- it's hard to tell on which part of the message the action will take

I think the first one should be addressed in this patch.
The two others might be handled in a separate bug since it would need some UI work to do (like, highlighting the element when hovering the context menu entry)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> I tested the patch and there's some edge cases:
> - `copy object` is enabled on `console.group("group")` messages
Have enhanced to ensure that context menu item is disabled for `console.copyGroup` and `console.collapsedGroup` commands and have added tests for the same.
Everything looks fine on TRY, let's push !
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fae2d636bbcb
Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console. r=nchevobbe
Sorry, this had to be backed out

Push on which the test failed: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=752c2be759b6af79b7cb667013b60b9e2d4fad6a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134907624&repo=autoland

14:31:29     INFO -  527 INFO TEST-PASS | devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js | `Copy object` is enabled for object in complex message -
14:31:29     INFO -  528 INFO Click on `Copy object`
14:31:29     INFO -  529 INFO TEST-PASS | devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js | Clipboard has the given value -
14:31:29     INFO -  530 INFO `Copy object` by using the access-key O
14:31:29     INFO -  531 INFO Synthesizing key shortcut: O
14:31:29     INFO -  532 INFO TEST-PASS | devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js | Clipboard has the given value -
14:31:29     INFO -  533 INFO Check `Copy object` is enabled for booleans
14:31:29     INFO -  534 INFO Check `Copy object` is enabled
14:31:29     INFO -  535 INFO TEST-PASS | devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js | `Copy object` is enabled for object in complex message -
14:31:29     INFO -  536 INFO Click on `Copy object`
14:31:29     INFO -  Buffered messages finished
14:31:29    ERROR -  537 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_object.js | Timed out while polling clipboard for pasted data -
14:31:29     INFO -  Stack trace:
14:31:29     INFO -  chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:wait:1003
14:31:29     INFO -  chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard/wait/<:1022

Abhinav, please fix the issue and provide an updated patch. Thank you.
Flags: needinfo?(abhinav.koppula)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/267dd0f306fa
Backed out changeset fae2d636bbcb for frequently failing new devtools test browser_webconsole_context_menu_copy_object.js on Windows 7 debug without e10s. r=backout
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review191696

::: devtools/client/locales/en-US/webconsole.properties:239
(Diff revision 5)
>  webconsole.menu.storeAsGlobalVar.accesskey=S
>  
>  # LOCALIZATION NOTE (webconsole.menu.copy.label)
>  # Label used for a context-menu item displayed for any log. Clicking on it will copy the
>  # content of the log (or the user selection, if any).
> -webconsole.menu.copy.label=Copy
> +webconsole.menu.copy.label=Copy message

For the record, I would have asked for backout for this.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

::: devtools/client/locales/en-US/webconsole.properties:246
(Diff revision 5)
>  
> +# LOCALIZATION NOTE (webconsole.menu.copy.object.label)
> +# Label used for a context-menu item displayed for object/variable log. Clicking on it
> +# will copy the object/variable.
> +webconsole.menu.copy.object.label=Copy object
> +webconsole.menu.copy.object.accesskey=O

Use 'o', not 'O'
Attachment #8912838 - Flags: review-
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review191932

::: devtools/client/locales/en-US/webconsole.properties:245
(Diff revisions 5 - 6)
> -webconsole.menu.copy.accesskey=C
> +webconsole.menu.copyMessage.accesskey=C
>  
> -# LOCALIZATION NOTE (webconsole.menu.copy.object.label)
> +# LOCALIZATION NOTE (webconsole.menu.copyObject.label)
>  # Label used for a context-menu item displayed for object/variable log. Clicking on it
>  # will copy the object/variable.
> -webconsole.menu.copy.object.label=Copy object
> +webconsole.menu.copyObject.label=Copy object

You don't need to update the string ID for this one, only use lowercase o for the accesskey.
Comment on attachment 8912838 [details]
Bug 1385995 - Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console.

https://reviewboard.mozilla.org/r/184172/#review191932

> You don't need to update the string ID for this one, only use lowercase o for the accesskey.

I thought that it would be consistent with the naming style already followed for the other properties like `webconsole.menu.copyMessage.label`, `webconsole.menu.selectAll.label` and since this property(`webconsole.menu.copy.object.label`) didn't exist earlier, I thought of changing it now before it lands into m-c. Your thoughts?
Flags: needinfo?(abhinav.koppula) → needinfo?(francesco.lodolo)
(In reply to Abhinav Koppula from comment #24)
> Comment on attachment 8912838 [details]
> Bug 1385995 - Adding `Copy object` to the context menu which allows to copy
> the object/variable logged to the console.
> 
> https://reviewboard.mozilla.org/r/184172/#review191932
> 
> > You don't need to update the string ID for this one, only use lowercase o for the accesskey.
> 
> I thought that it would be consistent with the naming style already followed
> for the other properties like `webconsole.menu.copyMessage.label`,
> `webconsole.menu.selectAll.label` and since this
> property(`webconsole.menu.copy.object.label`) didn't exist earlier, I
> thought of changing it now before it lands into m-c. Your thoughts?

Sorry, forget that. I was looking at 
https://reviewboard.mozilla.org/r/184172/diff/5-6/

And didn't realize this string was introduced in this bug. It's good to land as it is.
Flags: needinfo?(francesco.lodolo)
everything seem fine on the latest try push.
let's land it again, and if there are still failures, skip on the related platforms
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a31dd6fe18f
Adding `Copy object` to the context menu which allows to copy the object/variable logged to the console. r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/2a31dd6fe18f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
I have reproduced this issue using Firefox  56.0a1 (2017.07.31) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 58.0b3 on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.6. Running every test cases mentioned in this bug, for all test cases in context menu appear the "Copy object" menu item and was enabled, except for console.group("group") and inspect({a:1}) the "Copy object" menu item was disabled.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1418850
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.