[webkit-reviews] review requested: [Bug 37060] List item markers are not always updated after changes in the DOM : [Attachment 53896] patch

[bugzilla-daemon at webkit.org]

Jakub Wieczorek <jwieczorek at webkit.org> has asked  for review:
Bug 37060: List item markers are not always updated after changes in the DOM
https://bugs.webkit.org/show_bug.cgi?id=37060

Attachment 53896: patch
https://bugs.webkit.org/attachment.cgi?id=53896&action=review

------- Additional Comments from Jakub Wieczorek <jwieczorek at webkit.org>
Getting back to the first patch. It does no longer apply against ToT (partially
due to a recent Qt DRT refactoring), let me post a slightly rebased version
(moved the Qt private API from QWebFramePrivate to the new
DumpRenderTreeSupportQt).

(In reply to comment #29)
> (From update of attachment 52569 [details])
> The downside of waiting for all the EWS compiles is that I forgot the review
I
> did earlier, and had to reread the patch.
> 
> > +	 var child = element.firstChild;
> > +	 while (child) {
> > +	     if (child.nodeType == Node.ELEMENT_NODE) {
> > +		 var marker =
layoutTestController.markerTextForListItem(child);
> > +		 if (marker)
> > +		     ret += indent(depth) + ' ' + marker + ' ' +
child.innerText.trim() + '<br />';
> > +		 ret += dumpListHelper(child, depth + 1);
> > +	     }
> > +
> > +	     child = child.nextSibling;
> > +	 }
> 
> This could be a for loop. Loop could use firstElementChild and
> nextElementSibling and so avoid the nodeType check.
> 
> No need for that "/" in "<br />". This is not XHTML; the slash is just
ignored.
> 
> > +	 return ret;
> 
> I much prefer words for variable names over fragments of words.
> 
> > +		     // Don't show the actual list as it is useless in the
text-only mode.
> > +		     document.getElementById("list").style.display = "none";
> 
> Instead of styling it, you can just remove it from the DOM tree after
grabbing
> a dump of it.

Fixed all of them.

I'm afraid the LayoutTestController patch will need to be changed to pass the
JS context to the private functions in each port, just like the new
computedStyleIncludingVisitedInfo API does. I had no idea that there was an
assertion about the context not being 0 in the JSC C API, even though it is not
actually used.

I'm not a big fan of this but that's what computedStyleIncludingVisitedInfo
does. Following this idea seems like the best choice.

Darin, these are the only changes I've made compared to the old version,
hopefully it doesn't involve much effort from you. Thanks in advance.