Bug 30709 – Web Inspector: Pretty Print all HTML Collection Types like we do for NodeList

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Joseph Pecoraro]

Created attachment 41718 [details]
[IMAGE] Current Behavior of All Known Collection Types

Earlier we added support for pretty printing NodeLists in the Console [1].  We should extend that to HTMLCollection and other Collections defined in the HTML5 Spec [2]:

> HTMLCollection, HTMLAllCollection, HTMLFormControlsCollection,
> HTMLOptionsCollection, HTMLPropertyCollection, DOMTokenList,
> DOMSettableTokenList, DOMStringMap

Luckily, as the attached test case shows most of these reduce down to HTMLCollection in practice or aren't yet implemented.

[1]: Bug 28061
[2]: http://www.whatwg.org/specs/web-apps/current-work/#collections-0

[Joseph Pecoraro]

Created attachment 41719 [details]
[TEST CASE] Test Display of Collection Types

- Tests all known and implemented HTML5 Collection Types
- Attempt to make test usable for dealing with currently unimplemented types

[Joseph Pecoraro]

It seems we can potentially future-proof against new cases:

  > var options = document.createElement('select').options;
  > options.toString()
  [object HTMLOptionsCollection]

  > options instanceof HTMLCollection
  true

This could be used rather then individually checking class types.  But do you think there will be a case where we wouldn't want to do a basic pretty-print an HTMLCollection derivative? For instance indicating the "extra stuff".

  HTMLPropertyCollection has a "names" attribute
  HTMLOptionsCollection has add/remove functions

And DOMTokenList for the elem.classList has some neat functionality:

  https://developer.mozilla.org/en/DOM/DOMTokenList

My vote is that we should do a basic pretty-printing of not just HTMLCollection but also its derivates. If people really want to know whats up they can do a console.dir(...) on the object, but otherwise they should know what they are dealing with and would welcome the pretty printing.

[Joseph Pecoraro]

Interesting to note, I built recently and now document.all returns HTMLAllCollection which currently is not an instance of HTMLCollection. I'll look into this.

[Joseph Pecoraro]

Created attachment 41821 [details]
[TEST CASE] Test Display of Collection Types

I don't feel this should be a final patch due to the following two points:

- HTMLAllCollection as per HTML5 this morning should extend HTMLCollection:

  Make collections inherit from HTMLCollection:
  Fixing http://www.w3.org/Bugs/Public/show_bug.cgi?id=8035

  Commit Diff:
  http://html5.org/tools/web-apps-tracker?from=4322&to=4323

> -  <pre class="idl">interface <dfn>HTMLAllCollection</dfn> {
> +  <pre class="idl">interface <dfn>HTMLAllCollection</dfn> : <span>HTMLCollection</span> {

If this patch is desired now, how should I indicate in the test that the expected results should be updated once HTMLAllCollection is true for instanceof HTMLCollection.  Will just a comment suffice?

- Likewise I wanted to future proof formatting on some other collections like HTMLPropertyCollection, PropertyNodeList, DOMTokenList, DOMSettableTokenList, and DOMStringMap which are currently unimplemented in WebKit. Should I add checks for these with comments for when they fail that we should implement a pretty-printer, or is that bad style?

[Joseph Pecoraro]

Created attachment 41822 [details]
[PATCH] Example Patch

Uploaded the wrong file as a patch, but it was a better test case (RadioNodeList was incorrect in the first test).

[Joseph Pecoraro]

Created attachment 41902 [details]
[PATCH+TEST] Pretty Print Known HTMLCollections

[Joseph Pecoraro]

(In reply to comment #6)
> Created an attachment (id=41902) [details]
> [PATCH+TEST] Pretty Print Known HTMLCollections

- Handle HTMLAllCollection explicitly for now. Assuming the Spec doesn't change, then if HTMLAllCollection derives from HTMLCollection later on we can remove the extra OR.
- Tests show that HTMLFormsCollection and RadioNodeList might change their toString() eventually.  Should I remove the toString() in the test or keep them in and potentially have someone rebuild the expected file?

Since the HTMLCollection stuff has been hot lately, I'll rebuild + test this tonight.

[Joseph Pecoraro]

Pavel: I ran into some wonky problems with my test. When I run the test individually:

>  shell> run-webkit-tests inspector/console-format-collections.html
It outputs expected results as seen in the attached bug.

>  shell> run-webkit-tests inspector
My test fails, with garbled output and ugly unicode characters which I had to remove => /\u200b/.  But still, garbled.

I simplified the test to see if my unusual console.log's were causing problems, but that was not the problem. I then experimented with the test framework. One thing I found was:
http://trac.webkit.org/browser/trunk/LayoutTests/inspector/evaluate-in-frontend.js#L62

>  output.innerHTML += text + "<BR>";

Changing this to the following produced correctly ordered output and did not change any of the other inspector tests:

>  output.appendChild(document.createTextNode(text));
>  output.appendChild(document.createElement("br"));

However I still got differences when running the test individually or in a batch!

Individual Test Results:
http://pastie.textmate.org/private/2qvjs93dlx7ktcnkvczlfw

Batch Test Results:
http://pastie.textmate.org/private/7fkax5vxt6eskwr3enutq

Any idea why the individual and batch results differ, or pointers to areas I should look? I've checked to make sure both tests are using the correct files. I've even renamed them to ensure there were no caching problems (unlikely in a testing framework).

[Pavel Feldman]

The test seems to be right. I don't see it any different from console-format.html (does that one work for you in both cases btw?).

The fact that you get [] in the test with no contents means that it has not yet been populated through the async call. There is a comment in this test explaining the reason. We may be missing one more async indirection, but we actually should not. Does setTimeout(setTimout(notifyDone)) in the frontend injected script solve the problem? That would give us additional hint...

[Joseph Pecoraro]

Created attachment 41952 [details]
[PATCH+TEST] Pretty Print Known HTMLCollections

Thanks to Pavel's suggestion about the double setTimeout the output is the same in individual and batched.

[Joseph Pecoraro]

Created attachment 41974 [details]
[PATCH+TEST] Pretty Print Known HTMLCollections (with runAfterTimeouts)

Added WebInspector.timeouts counter and WebInspector.runAfterTimeouts.

I'm open to different names:
- asyncOperations, WebInspector.runAfterAsyncOperations
- dispatchedOperations, WebInspector.runAfterDispatchedOperations
- timeoutCount, WebInspector.runWithEmptyTimeoutQueue

[Timothy Hatcher]

Comment on attachment 41974 [details]
[PATCH+TEST] Pretty Print Known HTMLCollections (with runAfterTimeouts)


> +    this.timeouts = 0;

How about this.pendingDispatches?


> +WebInspector.runAfterTimeouts = function(cb)

And runAfterPendingDispatches?


> +{
> +    if (WebInspector.timeouts === 0)
> +        cb();
> +    else
> +        setTimeout(WebInspector.runAfterTimeouts, 0, cb);
> +}

cb should be callback. And this syntax looks better to me, but your way is shorten so I fold.

if (!WebInspector.pendingDispatches) {
    callback();
    return;
}

setTimeout(WebInspector.runAfterPendingDispatches, 0, callback);

[Joseph Pecoraro]

> > +    this.timeouts = 0;
> 
> How about this.pendingDispatches?

Done. 

> > +WebInspector.runAfterTimeouts = function(cb)
> 
> And runAfterPendingDispatches?

Done.

> > +{
> > +    if (WebInspector.timeouts === 0)
> > +        cb();
> > +    else
> > +        setTimeout(WebInspector.runAfterTimeouts, 0, cb);
> > +}
> 
> cb should be callback. And this syntax looks better to me, but your way is
> shorten so I fold.
> 
> if (!WebInspector.pendingDispatches) {
>     callback();
>     return;
> }
> 
> setTimeout(WebInspector.runAfterPendingDispatches, 0, callback);

I originally had your syntax, so I went with it.


Landed in http://trac.webkit.org/changeset/50168
r50168 = 883729f76eae97ef88de8a84bb9a81f80ec852e7

[Joseph Pecoraro]

Closing the bug.
Note that there do exist some other HTMLCollection types that may appear in the future. They should be handled by this patch, but they should get added to the test case. Also things like DOMStringMap and Properties might look nice pretty-printed.