[webkit-reviews] review denied: [Bug 15713] Outside list bullets ignore text-align style : [Attachment 68074] Fix the position of outside list bullets

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Renata Hodovan
<reni at inf.u-szeged.hu>'s request for review:
Bug 15713: Outside list bullets ignore text-align style
https://bugs.webkit.org/show_bug.cgi?id=15713

Attachment 68074: Fix the position of outside list bullets
https://bugs.webkit.org/attachment.cgi?id=68074&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68074&action=review

Thanks for taking this on.

This change needs to come with a new test case. Simply updating the results of
test cases that were intended to test other things is not sufficiently clear in
this case.

It’s considered inelegant to have data hanging off an object that’s only used
by code outside the class, and this is the case with m_textAlignOffset.

I also think we could come up with a better name for this concept than “text
align offset”. What is this value? An offset for aligning text? I’m not sure
that’s the best way to describe it.

I’d like to see code that would help us notice and debug if this value was used
without ever being set since the flow of control is indirect. Perhaps a special
uninitialized value and an assertion in the getter would do the trick.

review- because of insufficient test cases

> WebCore/rendering/InlineFlowBox.cpp:346
> +		   RenderListMarker* currMarker =
toRenderListMarker(curr->renderer());
> +		   currMarker->setTextAlignOffset(xPos);

The local variable is no help here. It would be clearer to have it on one line.


> WebCore/rendering/RenderListMarker.h:47
> +    int getTextAlignOffset() const { return m_textAlignOffset; }

In the WebKit project we don’t use get in the names of getter functions such as
this one. It should be named textAlignOffset.

> WebCore/rendering/RenderListMarker.h:84
> +    int m_textAlignOffset;

The set-up here seems fragile. This is not initialized, so the code will only
work correctly if InlineFlowBox::placeBoxesHorizontally is called before
positionListMarker. Is this guaranteed?