Last Comment Bug 676722 - The output of console.log(object) isn't expandable/inspectable in the Web Console
: The output of console.log(object) isn't expandable/inspectable in the Web Con...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 17
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
: 685815 (view as bug list)
Depends on: 688981
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-04 16:35 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-08-05 02:33 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (open console to see whats happening) (96 bytes, text/html)
2011-09-17 23:33 PDT, Andreas Gal :gal
no flags Details
proposed patch (15.58 KB, patch)
2012-07-13 13:35 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
updated patch (16.11 KB, patch)
2012-07-16 09:16 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
rebased patch (16.09 KB, patch)
2012-08-01 09:44 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
updated patch (16.10 KB, patch)
2012-08-02 12:12 PDT, Mihai Sucan [:msucan]
rcampbell: review+
Details | Diff | Splinter Review
test fixed for mac os x (16.22 KB, patch)
2012-08-04 03:34 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-08-04 16:35:21 PDT
<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 2011-09-17 23:33:04 PDT
Created attachment 560757 [details]
testcase (open console to see whats happening)
Comment 2 Mihai Sucan [:msucan] 2011-10-27 09:28:17 PDT
*** Bug 685815 has been marked as a duplicate of this bug. ***
Comment 3 Panos Astithas [:past] (away until 7/21) 2011-10-27 09:38:29 PDT
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] 2012-01-18 17:21:58 PST
I think this is a pretty important feature. -> P2
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-24 03:46:12 PST
This is partly fixed in bug 693269, and partly depends on bug 660197. Either way, it is tracked.

*** This bug has been marked as a duplicate of bug 693269 ***
Comment 6 Mihai Sucan [:msucan] 2012-07-13 09:55:40 PDT
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] 2012-07-13 13:35:25 PDT
Created attachment 642023 [details] [diff] [review]
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] 2012-07-16 09:16:15 PDT
Created attachment 642612 [details] [diff] [review]
updated patch

Updated the patch to include a fix for when there are multiple objects: console.log(object1, object2).
Comment 9 Mihai Sucan [:msucan] 2012-08-01 09:44:38 PDT
Created attachment 647996 [details] [diff] [review]
rebased patch
Comment 10 Rob Campbell [:rc] (:robcee) 2012-08-02 07:34:28 PDT
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] 2012-08-02 12:08:13 PDT
(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] 2012-08-02 12:12:38 PDT
Created attachment 648427 [details] [diff] [review]
updated patch

Updated to address your comments.
Comment 13 Rob Campbell [:rc] (:robcee) 2012-08-02 12:15:28 PDT
Comment on attachment 648427 [details] [diff] [review]
updated patch

ah, you make excellent points. Thanks for the update.
Comment 14 Panos Astithas [:past] (away until 7/21) 2012-08-03 01:55:44 PDT
https://hg.mozilla.org/integration/fx-team/rev/7e5f3a8a7ca7
Comment 15 Panos Astithas [:past] (away until 7/21) 2012-08-03 02:03:17 PDT
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 2012-08-03 20:00:29 PDT
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/mihai.sucan@gmail.com-8b9ec1e414ee
Comment 17 Mozilla RelEng Bot 2012-08-04 02:30:26 PDT
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/mihai.sucan@gmail.com-8b9ec1e414ee
Comment 18 Mihai Sucan [:msucan] 2012-08-04 03:34:01 PDT
Created attachment 648983 [details] [diff] [review]
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] 2012-08-05 02:33:53 PDT
https://hg.mozilla.org/mozilla-central/rev/fe50f2057825

Note You need to log in before you can comment on or make changes to this bug.