676722 - The output of console.log(object) isn't expandable/inspectable in the Web Console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Jason Orendorff [:jorendorff]]

<robcee> "firebug does it"

If I enter "document" in the Web Console, I get back
  [object HTMLDocument]
and it's linkified; if I click on it, I get the Inspector.

If I enter "console.log(document)", I get this:
  [object XrayWrapper [object HTMLDocument]]
and it is not linkified or inspectable.

Comment 1

[Andreas Gal :gal]

Attachment: testcase (open console to see whats happening)

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

In order to make sure it doesn't get overlooked, I'm making an explicit note that bug 685815 requested making object substitution patterns (%o) for console.log and friends expandable, too.

Comment 4

[Paul Rouget [:paul]]

I think this is a pretty important feature. -> P2

Comment 5

[Joe Walker [:jwalker]]

This is partly fixed in bug 693269, and partly depends on bug 660197. Either way, it is tracked.

Comment 6

[Mihai Sucan [:msucan]]

Reopening and taking the bug because GCLI is no longer in the Web Console and we still need to have console.log(object) inspectable in the Web Console. Fixing bug 660197 is a different issue which requires UI work on the JS object inspector, and that will happen at a later point in time.

Will submit a patch soonish.

Comment 7

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch with included test. Now any console.*() call will underline object toString() representations, allowing a click to open the object inspector.

Please let me know if further changes are needed. Thanks!

As agreed with Rob, this patch does not go into fancy UI/UX. We still use the object inspector. Inline inspection and other UX improvements will come later.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch to include a fix for when there are multiple objects: console.log(object1, object2).

Comment 9

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 647996 [details] [diff] [review]
rebased patch

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

This looks pretty good and after playing with it, I really want this functionality. Just a couple of questions in line, but this is great.

::: browser/devtools/webconsole/webconsole.js
@@ +1078,5 @@
> +   * window.console API.
> +   *
> +   * @private
> +   * @param nsIDOMNode aMessage
> +   *        The message element for which we live.

little grammar nit: The message element we... live in?

We don't live in a message element! That's ridiculous! Maybe The message element this handler corresponds to? Or The message element this handler is bound to? Something along those lines might make more sense.

@@ +1080,5 @@
> +   * @private
> +   * @param nsIDOMNode aMessage
> +   *        The message element for which we live.
> +   * @param nsIDOMNode aAnchor
> +   *        The object inspector anchor element. This is the clickbable element

typo: "clickable". teehee.

@@ +1121,5 @@
> +      }
> +    }.bind(this);
> +
> +    propPanel.panel.addEventListener("popuphiding", onPopupHide, false);
> +  },

looks good.

@@ +1897,5 @@
> +        str = aBody.resultString;
> +      }
> +      else if (["log", "info", "warn", "error", "debug"].indexOf(aLevel) > -1 &&
> +               typeof aBody == "object") {
> +        this._makeConsoleLogMessageBody(node, bodyNode, aBody);

you could return early here and do away with the !== undefined check at the bottom.

@@ +2019,5 @@
> +      configurable: false
> +    });
> +
> +    aBody.remoteObjects.forEach(function(aItem, aIndex) {
> +      if (aContainer.lastChild) {

isn't aContainer always going to have a lastChild element? Seems like this check is going to always be true.

@@ +2037,5 @@
> +
> +      this._addMessageLinkCallback(elem,
> +        this._consoleLogClick.bind(this, aMessage, elem, aItem));
> +
> +      aContainer.appendChild(elem);

there's a whole lot of appendChild in this method, which I'm not entirely keen on. Would this make sense to use a documentFragment (or heck, even a string) here to build up your node and then insert the whole thing into aContainer at the end? This feels like it might be a little slow.

::: dom/base/ConsoleAPI.js
@@ +356,5 @@
>      let processed = format.replace(ARGUMENT_PATTERN, function CA_PA_substitute(match, submatch) {
>        switch (submatch) {
>          case "o":
> +          objects.push(args.shift());
> +          return splitter;

cool. :)

Comment 11

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> Comment on attachment 647996 [details] [diff] [review]
> rebased patch
> 
> Review of attachment 647996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good and after playing with it, I really want this
> functionality. Just a couple of questions in line, but this is great.

Thanks!

> ::: browser/devtools/webconsole/webconsole.js
> @@ +1078,5 @@
> > +   * window.console API.
> > +   *
> > +   * @private
> > +   * @param nsIDOMNode aMessage
> > +   *        The message element for which we live.
> 
> little grammar nit: The message element we... live in?
> 
> We don't live in a message element! That's ridiculous! Maybe The message
> element this handler corresponds to? Or The message element this handler is
> bound to? Something along those lines might make more sense.

Thanks for the grammar fix. Will fix.

> @@ +1080,5 @@
> > +   * @private
> > +   * @param nsIDOMNode aMessage
> > +   *        The message element for which we live.
> > +   * @param nsIDOMNode aAnchor
> > +   *        The object inspector anchor element. This is the clickbable element
> 
> typo: "clickable". teehee.

"click-bable!" ;)


> @@ +1897,5 @@
> > +        str = aBody.resultString;
> > +      }
> > +      else if (["log", "info", "warn", "error", "debug"].indexOf(aLevel) > -1 &&
> > +               typeof aBody == "object") {
> > +        this._makeConsoleLogMessageBody(node, bodyNode, aBody);
> 
> you could return early here and do away with the !== undefined check at the
> bottom.

Not really, we still need the rest of the createMessageNode() code to execute. I know, ugly stuff. Can't return early...

> @@ +2019,5 @@
> > +      configurable: false
> > +    });
> > +
> > +    aBody.remoteObjects.forEach(function(aItem, aIndex) {
> > +      if (aContainer.lastChild) {
> 
> isn't aContainer always going to have a lastChild element? Seems like this
> check is going to always be true.

I meant to only add the space when there's already some content. If the container is empty we don't add the space. Will change to check for "firstChild" - should be less confusing.


> @@ +2037,5 @@
> > +
> > +      this._addMessageLinkCallback(elem,
> > +        this._consoleLogClick.bind(this, aMessage, elem, aItem));
> > +
> > +      aContainer.appendChild(elem);
> 
> there's a whole lot of appendChild in this method, which I'm not entirely
> keen on. Would this make sense to use a documentFragment (or heck, even a
> string) here to build up your node and then insert the whole thing into
> aContainer at the end? This feels like it might be a little slow.

Good point, but the message node is not yet appended into the output. So, for all practical purposes, this should not be slow - once everything is done, it's all appended at once into the output.


Thanks for looking into the patch!

Comment 12

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated to address your comments.

Comment 13

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 648427 [details] [diff] [review]
updated patch

ah, you make excellent points. Thanks for the update.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Backed out:
https://hg.mozilla.org/integration/fx-team/rev/98cdb3e2d30a

due to test failures in:
https://tbpl.mozilla.org/?tree=Try&rev=b871b8028de9

Comment 16

[Mozilla RelEng Bot]

Try run for 8b9ec1e414ee is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8b9ec1e414ee
Results (out of 24 total builds):
    success: 21
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8b9ec1e414ee

Comment 17

[Mozilla RelEng Bot]

Try run for 8b9ec1e414ee is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8b9ec1e414ee
Results (out of 48 total builds):
    success: 42
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8b9ec1e414ee

Comment 18

[Mihai Sucan [:msucan]]

Attachment: test fixed for mac os x

After some oranges on the try server, I got the greens.

Landed in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/fe50f2057825

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

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