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
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
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.
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.
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?
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).
Created attachment 41902 [details] [PATCH+TEST] Pretty Print Known HTMLCollections
(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.
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).
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...
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.
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
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);
> > + 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
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.