[webkit-reviews] review requested: [Bug 37060] List item markers are not always updated after changes in the DOM : [Attachment 52526] 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 52526: patch
https://bugs.webkit.org/attachment.cgi?id=52526&action=review

------- Additional Comments from Jakub Wieczorek <jwieczorek at webkit.org>
(In reply to comment #4)
> (From update of attachment 52512 [details])
> >  static Node* enclosingList(const RenderObject* renderer)
> >  {
> > -	 Node* node = renderer->node();
> > -	 if (node)
> > -	     return enclosingList(node);
> > -
> > -	 renderer = renderer->parent();
> >	 while (renderer && !renderer->node())
> >	     renderer = renderer->parent();
> >  
> > -	 node = renderer->node();
> > +	 if (!renderer)
> > +	     return 0;
> > +
> > +	 Node* node = renderer->node();
> >	 if (isList(node))
> >	     return node;
> 
> This change means the code now walks up the render tree before walking up the

> DOM tree. How did you determine that there was no case where that changed the

> function result?

Actually it was doing this before too, I just wanted to formulate it simpler.
But you're right, there is one possible behavior change: for a list that also
acts as a list item, it would return the list itself. Anyway, I'll just revert
this hunk as it's not even necessary for this patch.

> > +void updateListMarkerNumbers(RenderObject* listItem)
> > +{
> > +	 ASSERT(listItem->isListItem());
> 
> Since this function is designed to only be called on a RenderListItem, its
> argument type should be RenderListItem*, not RenderObject*. The callers
should
> call toRenderListItem, which will do the type cast and assertion.
> 
> In fact, you should make it a member function, since it's in the
> RenderListItem.cpp file and is a basic part of what a RenderListItem does.
> 
> I suggest also changing the argument type of the enclosingList function to
> RenderListItem*.

Done.

> > +	 RenderObject* child = listItem->nextInPreOrder(list);
> 
> Is this the right thing to do when list is 0? It means you'll walk the entire

> document looking for other list items. Is that really what we want to do for
> items outside the document.

Well, I made a mistake to let enclosingList be 0 in the first place. For some
reason I came to that conclusion when I saw fast/lists/anonymous-items.html
crashing with my patch but it turned out to be a different issue which I fixed
even before the first version of the patch.

> > +	 while (child) {
> 
> This should probably be written as a for loop instead of a while loop,
although
> the line will end up being long. But it would make the loop logic easier to
> understand.

I changed it to be a for loop but personally I don't find it easier to read as
we're ending up with something like:

if (child->node() && isList(child->node())) {
    // We've found a nested, independent list: nothing to do here.
    child = child->nextInPreOrderAfterChildren(list)->previousInPreOrder();
    continue;
}

> > +		 // We've found a nested, independent list - nothing to do
here.
> 
> Instead of a hyphen separating these phrases, I'd suggest a colon.

Fixed.
 
> >	     // renumber ordered lists
> >	     if (oldChild->isListItem())
> > -		 updateListMarkerNumbers(oldChild->nextSibling());
> > +		 updateListMarkerNumbers(oldChild);
> 
> > +	     // renumber ordered lists
> > +	     if (newChild->isListItem())
> > +		 updateListMarkerNumbers(newChild);
> 
> > +	     // renumber ordered lists
> > +	     if (child->isListItem())
> > +		 updateListMarkerNumbers(child);
> 
> If we really need this same comment every time we call the
> updateListMarkerNumbers function, then I think we need to rename the function

> instead. Please omit the comment.

Yeah, that original comment wasn't really valuable.

> > +gchar* webkit_web_frame_marker_text_for_list_item(WebKitWebFrame* frame,
JSObjectRef nodeObject)
> > +{
> > +	 JSC::JSObject* object = toJS(nodeObject);
> > +	 if (!object || !object->inherits(&JSHTMLElement::s_info))
> > +	     return 0;
> > +
> > +	 HTMLElement* element = static_cast<JSHTMLElement*>(object)->impl();
> > +	 if (!element || !element->renderer())
> > +	     return 0;
> > +
> > +	 RenderObject* renderer = element->renderer();
> > +	 if (!renderer->isListItem())
> > +	     return 0;
> > +
> > +	 String markerText = toRenderListItem(renderer)->markerText();
> > +	 return g_strdup(markerText.utf8().data());
> > +}
> 
> There's no reason to limit this to HTML elements. Non-HTML elements can be
> styled and end up as list items. You should check for JSElement rather than
> JSHTMLElement.

Fixed.

> This function as written isn't safe. You can't grab an element's renderer
> without first calling Document::updateLayout.

Indeed, that's why I needed to explicitly trigger a layout in the tests. Doing
this in the function makes more sense.

> You should move the part of this function that goes from an element to marker

> text out of Qt-specific and GTK-specific code into RenderTreeAsText.cpp,
> alongside the similar counterValueForElement function and using similar
> implementation techniques.

Done.

> > +static JSValueRef markerTextForListItemCallback(JSContextRef context,
JSObjectRef, JSObjectRef thisObject, size_t argumentCount, const JSValueRef
arguments[], JSValueRef* exception)
> > +{
> > +	 if (argumentCount < 1)
> > +	     return JSValueMakeUndefined(context);
> > +
> > +	 LayoutTestController* controller =
static_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject));
> > +	 JSObjectRef nodeObject = JSValueToObject(context, arguments[0],
exception);
> > +	 ASSERT(!*exception);
> > +
> > +	 return JSValueMakeString(context,
controller->markerTextForListItem(nodeObject).get());
> > +}
> 
> I find the division of responsibilities here a little strange. This function
> handles converting the JSValue to a JSObject, but not checking the type of
> JSObject. I think it would be cleaner to pass the JSValue in to the
controller
> since it already has to deal with JavaScript type checking to check that this

> is a node object.

Done.

> review- because I think at least some of these things should be fixed before
> landing

Thanks, can you take another look?