[Webkit-unassigned] [Bug 82566] RenderTextControlSingleLine::scrollWidth/H/L/T should not call back to DOM tree

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=82566


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #134486|review?                     |
               Flag|                            |




--- Comment #4 from Julien Chaffraix <jchaffraix at webkit.org>  2012-04-06 09:49:36 PST ---
(From update of attachment 134486)
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.

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