[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


Jakub Wieczorek <jwieczorek at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52512|0                           |1
        is obsolete|                            |
  Attachment #52526|                            |review?
               Flag|                            |




--- Comment #5 from Jakub Wieczorek <jwieczorek at webkit.org>  2010-04-05 05:28:54 PST ---
Created an attachment (id=52526)
 --> (https://bugs.webkit.org/attachment.cgi?id=52526)
patch

(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?

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