Set multiplier to 1 on blocks with no direct text children.

Set multiplier to 1 on blocks with no direct text children.

This fixes cases where a node that previously had a >1 multiplier ceases to have direct text children.  This can happen if its text content is removed in script, or if its 'display' style is changed from inline to block.  (There are probably other ways for it to happen also.)

Previously such a node would have a "stale" multiplier, since the autosizer only updated multipliers on text nodes and their immediate parents.  In theory the stale multiplier should not have affected the rendering, but:

- it is observable through the lineHeight property, and
- due to http://crbug.com/380903 the stale multiplier on an ancestor might be used to render text, even when the text node has the correct multiplier

BUG=378959
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175603

[pdr., 2014-06-05 20:13:49]

This is clever! I like this patch.

https://codereview.chromium.org/319013003/diff/20001/LayoutTests/fast/text-au...
File LayoutTests/fast/text-autosizing/display-type-change-lineHeight.html
(right):

https://codereview.chromium.org/319013003/diff/20001/LayoutTests/fast/text-au...
LayoutTests/fast/text-autosizing/display-type-change-lineHeight.html:22: <div
id="ddd">
Remove ddd

https://codereview.chromium.org/319013003/diff/20001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/319013003/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:444: void
FastTextAutosizer::inflate(RenderBlock* block)
I think this would be a little cleaner if this returned the multiplier and took
a float multiplier as an argument that has a default value of 0. This would
remove the need for a second function and the by-ref float.

https://codereview.chromium.org/319013003/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:456: if (parent->isRenderBlock() &&
parent->childrenInline())
The fast firstChild checks only remove a single virtual call so these checks
will actually end up being more expensive. I think we can refactor this as:
if (parent->isRenderBlock() && parent->childrenInline() ||
parent->isRenderInline())
    child = parent->firstChild();

https://codereview.chromium.org/319013003/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:483: applyMultiplier(parent, 1);
Should we only do this when textAutosizingMultiplier != 1? Otherwise we will
create many more unique styles which may be unnecessary.

[skobes, 2014-06-05 21:19:20]

https://codereview.chromium.org/319013003/diff/20001/LayoutTests/fast/text-au...
File LayoutTests/fast/text-autosizing/display-type-change-lineHeight.html
(right):

https://codereview.chromium.org/319013003/diff/20001/LayoutTests/fast/text-au...
LayoutTests/fast/text-autosizing/display-type-change-lineHeight.html:22: <div
id="ddd">
On 2014/06/05 20:13:49, pdr wrote:
> Remove ddd

Done.

https://codereview.chromium.org/319013003/diff/20001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/319013003/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:444: void
FastTextAutosizer::inflate(RenderBlock* block)
On 2014/06/05 20:13:49, pdr wrote:
> I think this would be a little cleaner if this returned the multiplier and
took
> a float multiplier as an argument that has a default value of 0. This would
> remove the need for a second function and the by-ref float.

Done.

https://codereview.chromium.org/319013003/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:456: if (parent->isRenderBlock() &&
parent->childrenInline())
On 2014/06/05 20:13:49, pdr wrote:
> The fast firstChild checks only remove a single virtual call so these checks
> will actually end up being more expensive. I think we can refactor this as:
> if (parent->isRenderBlock() && parent->childrenInline() ||
> parent->isRenderInline())
>     child = parent->firstChild();

We can't directly call parent->firstChild() because firstChild does not exist on
RenderObject.  Or do you mean we should call slowFirstChild()?

https://codereview.chromium.org/319013003/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:483: applyMultiplier(parent, 1);
On 2014/06/05 20:13:49, pdr wrote:
> Should we only do this when textAutosizingMultiplier != 1? Otherwise we will
> create many more unique styles which may be unnecessary.

applyMultiplier does this check.

[pdr., 2014-06-05 21:52:21]

>
https://codereview.chromium.org/319013003/diff/20001/Source/core/rendering/Fa...
> Source/core/rendering/FastTextAutosizer.cpp:456: if (parent->isRenderBlock()
&&
> parent->childrenInline())
> On 2014/06/05 20:13:49, pdr wrote:
> > The fast firstChild checks only remove a single virtual call so these checks
> > will actually end up being more expensive. I think we can refactor this as:
> > if (parent->isRenderBlock() && parent->childrenInline() ||
> > parent->isRenderInline())
> >     child = parent->firstChild();
> 
> We can't directly call parent->firstChild() because firstChild does not exist
on
> RenderObject.  Or do you mean we should call slowFirstChild()?

I did mean slowFirstChild(). I think this is fine as-is after all.

LGTM

[skobes, 2014-06-05 21:52:43]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-06-05 21:53:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skobes@chromium.org/319013003/40001

[pdr., 2014-06-05 21:53:32]

Please add a little more to the change description before landing.

[skobes, 2014-06-05 21:54:58]

The CQ bit was unchecked by skobes@chromium.org

[skobes, 2014-06-05 22:03:28]

On 2014/06/05 21:53:32, pdr wrote:
> Please add a little more to the change description before landing.

Done.

[skobes, 2014-06-05 22:03:32]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-06-05 22:04:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skobes@chromium.org/319013003/40001

[commit-bot: I haz the power, 2014-06-05 23:06:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10857)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-05 23:06:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-05 23:06:45]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[skobes, 2014-06-06 00:17:03]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-06-06 00:17:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skobes@chromium.org/319013003/40001

[commit-bot: I haz the power, 2014-06-06 00:18:55]

Change committed as 175603