[Webkit-unassigned] [Bug 82566] RenderTextControlSingleLine::scrollWidth/Height/Left/Top should not call back to DOM tree

[bugzilla-daemon at webkit.org]

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





--- Comment #22 from Julien Chaffraix <jchaffraix at webkit.org>  2012-06-07 11:32:24 PST ---
(From update of attachment 137437)
View in context: https://bugs.webkit.org/attachment.cgi?id=137437&action=review

>>> Source/WebCore/ChangeLog:9
>>> +        unexpected layout update.
>> 
>> How is that possible? When we're calling scrollWidth on RenderTextControlSingleLine via Element::scrollWidth(),
>> we should have already called updateLayoutIgnorePendingStylesheets().
>> The call to updateLayoutIgnorePendingStyleSheets() here should be no-op. Who is the caller?
>> We need to fix that caller to call updateLayoutIgnorePendingStyleSheets first. r- because this is a wrong fix.
>> 
>> Do we have a crash report / stack trace for this?
> 
> scrollWidth() is a virtual function. Anything that calls renderBox->scrollWidth() can potentially go to RenderTextControlSingleLine::scrollWidth().
> 
> Why is it a wrong fix when there is no layout test fails? Why query innerTextElement()->scrollWidth() from the DOM tree when there is the cached value in the render tree?

I stand on my ground and agree with Tien-Ren logic here: the overflow logic, that runs during layout, will query scrollWidth. This can end up in us triggering a new layout (which is slow but normally harmless). I don't deny that I don't know the condition for that to happen, just that it is an undeniable possibility.

If we put the overflow logic aside, it's a layering violation that rendering/ calls back to DOM in this case.

Fixing callers is unfeasible for several reasons: RenderTextControlSingleLine have a very different way of working than the average RenderObject (ie it wraps a shadow DOM) but mostly because you *don't* want rendering to start calling Document::updateLayoutIgnorePendingStyleSheets

>>> Source/WebCore/ChangeLog:11
>>> +        Reviewed by NOBODY (OOPS!).
>> 
>> This line should appear before the long description.
> 
> Okay, we'll find a way to test this, but before that let's determine what is the correct fix.

There is no mention of where the "Reviewed by ..." line should be in the coding style or any tool enforcing what you said, Ryosuke. I don't think a contributor should be penalized for that nor that a reviewer should impose his view on that.

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