[webkit-reviews] review requested: [Bug 45381] [Gtk] Adjust atk_text_get_text_at_offset to account for bullets/numbers in list items : [Attachment 68509] Patch proposal + unit tes updated

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 45381: [Gtk] Adjust atk_text_get_text_at_offset to account for
bullets/numbers in list items
https://bugs.webkit.org/show_bug.cgi?id=45381

Attachment 68509: Patch proposal + unit tes updated
https://bugs.webkit.org/attachment.cgi?id=68509&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 67808 [details] [details])
> > View in context:
https://bugs.webkit.org/attachment.cgi?id=67808&action=review
> > 
> > i don't know how other people feel about adding a variable to
RenderListMarker. sometimes they are quite picky about that. 
> 
> Well, that's the best way I found to allow returning the marker's suffix
without having to recalculate it each time. But if there's a better way to do
it, I'm more than happy to change it.

I finally found a simple way to expose the marker's suffix through a public
function without having to add that variable to RenderListMarker, and it was
just delegating on the internal static function listMarkerSuffix(), and then
doing some small changes in the new funcion
RenderListItem::markerTextWithSuffix(). And to be honest, I recognize I like it
more this way, which hast the plus of not relying in the painting process to
get the right value of the marker suffix.

> > need a layout test for this however
> 
> Wouldn't it be enough with an unit test to check the marker is what it's
supposed to be?

I updated the unit test for ATK to ensure that the exposition of the marker is
the right one. Guess this should be enough, but you (Chris) will better tell
 
> > > WebCore/ChangeLog:39
> > > +
> > 
> > indent comments when they are underneath the a filename. it makes it easier
to read
> 
> Ok

Done.
 
> > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1218
> > > +    RenderObject* renderer = static_cast<const
AccessibilityRenderObject*>(object)->renderer();
> > 
> > check if renderer is nil as well
> 
> Ok.

Done.

> > > WebCore/rendering/RenderListItem.cpp:325
> > > +    const String& markerTxt = m_marker->text();
> > 
> > var name should be markerText
> 
> Ok.

Done.