[Webkit-unassigned] [Bug 37060] List item markers are not always updated after changes in the DOM

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37060


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52512|review?                     |review-
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2010-04-04 21:46:15 PST ---
(From update of attachment 52512)
>  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?

> +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*.

> +    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.

> +    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.

> +            // We've found a nested, independent list - nothing to do here.

Instead of a hyphen separating these phrases, I'd suggest a colon.

>          // 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.

> +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.

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

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.

> +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.

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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.