[webkit-reviews] review granted: [Bug 37079] Links around blocks (e.g. divs) results in too many VoiceOver call outs : [Attachment 54047] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Maciej Stachowiak <mjs at apple.com>'s request for
review:
Bug 37079: Links around blocks (e.g. divs) results in too many VoiceOver call
outs
https://bugs.webkit.org/show_bug.cgi?id=37079

Attachment 54047: Patch
https://bugs.webkit.org/attachment.cgi?id=54047&action=review

------- Additional Comments from mitz at webkit.org
> +static inline RenderObject* lastChildConsideringContinuation(RenderObject*
renderer)
> +{
> +    RenderObject* lastChild = renderer->lastChild();
> +    RenderObject* prev = renderer;

prev is unused (you just assign to it).

> +    RenderObject* cur = renderer;
> +
> +    while (cur) {
> +	   prev = cur;
> +
> +	   if (RenderObject* lc = cur->lastChild())
> +	       lastChild = lc;
> +
> +	   if (cur->isRenderInline()) {
> +	       cur = toRenderInline(cur)->continuation();
> +	       if (cur && cur->isRenderBlock()) 
> +		   cur = toRenderBlock(cur)->inlineContinuation();
> +	   } else
> +	       cur = toRenderBlock(cur)->inlineContinuation();

I can’t think of a case where you couldn’t just do cur =
cur->inlineContinuation() (assuming cur is a block or an inline). In other
words, the first branch seems needlessly complicated. What am I missing?

>  AccessibilityObject* AccessibilityRenderObject::firstChild() const
>  {
>      if (!m_renderer)
>	   return 0;
>      
>      RenderObject* firstChild = m_renderer->firstChild();
> +
> +    if (!firstChild && isInlineWithContinuation(m_renderer))
> +	   firstChild = firstChildConsideringContinuation(m_renderer);

You can just use firstChildConsideringContinuation(), because it includes the
above (getting firstChild() and checking if it’s 0 etc.).

> +static inline RenderObject* endOfContinuations(RenderObject* renderer)
> +{
> +    RenderObject* prev = renderer;
> +    RenderObject* cur = renderer;
> +
> +    while (cur) {
> +	   prev = cur;
> +	   if (cur->isRenderInline()) {
> +	       cur = toRenderInline(cur)->continuation();
> +	       if (cur && cur->isRenderBlock()) 
> +		   cur = toRenderBlock(cur)->inlineContinuation();
> +	   } else
> +	       cur = toRenderBlock(cur)->inlineContinuation();

Like in lastChildConsideringContinuations, I don’t understand why the inline
case can’t just use inlineContinuation().

r=beth and me