[webkit-reviews] review canceled: [Bug 82566] RenderTextControlSingleLine::scrollWidth/H/L/T should not call back to DOM tree : [Attachment 134486] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Tien-Ren Chen
<trchen at chromium.org>'s request for review:
Bug 82566: RenderTextControlSingleLine::scrollWidth/H/L/T should not call back
to DOM tree
https://bugs.webkit.org/show_bug.cgi?id=82566

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134486&action=review


> I'm not really certain if it is the right thing to do, since it changes the
layout behavior,

The tests are passing on cr-linux EWS so how come it changes the layout? What
kind of changes do you see? It sounds like you should add a test that shows
this difference? A test would also help us see the issue and advise a better
way to fix it (I think your approach is sane but would love to see the
incriminating test case).

> Source/WebCore/ChangeLog:3
> +	   RenderTextControlSingleLine::scrollWidth/H/L/T should not call back
to DOM tree

Please don't abbreviate, it makes your change less readable.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:694
> +    if (innerTextElement() && innerTextElement()->renderBox())

I wonder if we need any of the NULL-checks. Does it make sense to have a
RenderTextControlSingleLine without an inner element? (the code is not
consistent about those unfortunately)

I would have to look at this logic but tkent@ may know from the top of his
head.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:695
> +	   return innerTextElement()->renderBox()->scrollWidth();

As you pointed out, there is a difference with respect to zoom factor between
Element::scrollWidth and RenderBox::scrollWidth. To be truly accurate here, we
should take the innerTextElement()'s zoom factor into account in the same way
Element::scrollWidth() does. I do fear code duplication here but I don't see a
good way around that as we don't want to expose the inner gut of
Element::scrollWidth to everyone.