TextAutosizer: unlock multipliers on DOMContentLoaded.

TextAutosizer: unlock multipliers on DOMContentLoaded.

This patch unlocks the text autosizing multipliers when
the document has finished parsing. This is necessary
to avoid weird effects when layout changes significantly
during page load. Resetting multipliers allows for a fresh
autosizing pass to take into account the whole document.

In particular this patch fixes issues with overlines on
http://forum.xda-developers.com and partially addresses
inconsistent font size on www.drudgereport.com
(see referenced bugs for details).

BUG=346786, 131566
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169572

[skobes, 2014-03-11 17:47:57]

https://codereview.chromium.org/189113014/diff/90001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/189113014/diff/90001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4431: // This is to allow for a fresh text
autosizing pass when the page layout
I'm confused here - the autosizer is called by layout().  How is it possible for
layout to "change significantly" without re-running it?

[timvolodine, 2014-03-11 17:57:04]

https://codereview.chromium.org/189113014/diff/90001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/189113014/diff/90001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4431: // This is to allow for a fresh text
autosizing pass when the page layout
On 2014/03/11 17:47:58, skobes wrote:
> I'm confused here - the autosizer is called by layout().  How is it possible
for
> layout to "change significantly" without re-running it?

Yes, what I meant is to allow for a fresh autosizing pass when the layout is
executed after Document::finishedParsing is called..
This is needed for pages which load tables mixed with javascript inside them
e.g. drudgereport and the canonical test I have included.

[skobes, 2014-03-11 19:30:21]

lgtm

https://codereview.chromium.org/189113014/diff/90001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/189113014/diff/90001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4431: // This is to allow for a fresh text
autosizing pass when the page layout
On 2014/03/11 17:57:05, timvolodine wrote:
> On 2014/03/11 17:47:58, skobes wrote:
> > I'm confused here - the autosizer is called by layout().  How is it possible
> for
> > layout to "change significantly" without re-running it?
> 
> Yes, what I meant is to allow for a fresh autosizing pass when the layout is
> executed after Document::finishedParsing is called..
> This is needed for pages which load tables mixed with javascript inside them
> e.g. drudgereport and the canonical test I have included.

I see, this is because you don't allow the multiplier to change after it's been
set.

I wonder if we need to duplicate this in FTA... currently we don't lock
multipliers.

[timvolodine, 2014-03-12 16:25:11]

On 2014/03/11 19:30:21, skobes wrote:
> lgtm
> 
>
https://codereview.chromium.org/189113014/diff/90001/Source/core/dom/Document...
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/189113014/diff/90001/Source/core/dom/Document...
> Source/core/dom/Document.cpp:4431: // This is to allow for a fresh text
> autosizing pass when the page layout
> On 2014/03/11 17:57:05, timvolodine wrote:
> > On 2014/03/11 17:47:58, skobes wrote:
> > > I'm confused here - the autosizer is called by layout().  How is it
possible
> > for
> > > layout to "change significantly" without re-running it?
> > 
> > Yes, what I meant is to allow for a fresh autosizing pass when the layout is
> > executed after Document::finishedParsing is called..
> > This is needed for pages which load tables mixed with javascript inside them
> > e.g. drudgereport and the canonical test I have included.
> 
> I see, this is because you don't allow the multiplier to change after it's
been
> set.
> 
> I wonder if we need to duplicate this in FTA... currently we don't lock
> multipliers.

Thanks Steve.
The reason we locked multipliers is because there were issues with oscillating
font sizes (crbug.com/167977). I have filed a crbug.com/351745 for considering
locking in the new version. In my testing of the FastTextAutosizer I could not
actually reproduce most oscillation bugs but some issues still remain. Anyway
something to keep in mind I think.

[timvolodine, 2014-03-13 11:40:06]

Philip, could you have a look please?
It's a release blocker so I want to fix this sooner rather than later ;)

[timvolodine, 2014-03-17 15:51:14]

On 2014/03/13 11:40:06, timvolodine wrote:
> Philip, could you have a look please?
> It's a release blocker so I want to fix this sooner rather than later ;)

ping?

[pdr., 2014-03-18 21:01:35]

On 2014/03/17 15:51:14, timvolodine wrote:
> On 2014/03/13 11:40:06, timvolodine wrote:
> > Philip, could you have a look please?
> > It's a release blocker so I want to fix this sooner rather than later ;)
> 
> ping?

I think this look fantastic. LGTM :)

[pdr., 2014-03-18 21:04:24]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-18 21:04:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/189113014/90001

[commit-bot: I haz the power, 2014-03-18 21:05:07]

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

[commit-bot: I haz the power, 2014-03-18 21:05:08]

Failed to apply patch for Source/core/rendering/TextAutosizer.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/TextAutosizer.cpp
  Hunk #2 FAILED at 195.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/rendering/TextAutosizer.cpp.rej

Patch:       Source/core/rendering/TextAutosizer.cpp
Index: Source/core/rendering/TextAutosizer.cpp
diff --git a/Source/core/rendering/TextAutosizer.cpp
b/Source/core/rendering/TextAutosizer.cpp
index
c1bb64adf55906929daf5c51977c38ed551cf51d..99fe26cc91ea94fce907c018ffeaf3f1aca591da
100644
--- a/Source/core/rendering/TextAutosizer.cpp
+++ b/Source/core/rendering/TextAutosizer.cpp
@@ -169,8 +169,20 @@ unsigned TextAutosizer::getCachedHash(const RenderObject*
renderer, bool putInCa
     return combinedHashValue;
 }
 
+bool TextAutosizer::isApplicable() const
+{
+    return m_document->settings()
+        && m_document->settings()->textAutosizingEnabled()
+        && m_document->page()
+        && m_document->page()->mainFrame()
+        &&
m_document->page()->mainFrame()->loader().stateMachine()->committedFirstRealDocumentLoad();
+}
+
 void TextAutosizer::recalculateMultipliers()
 {
+    if (!isApplicable())
+        return;
+
     RenderObject* renderer = m_document->renderer();
     while (renderer) {
         if (renderer->style() && renderer->style()->textAutosizingMultiplier()
!= 1)
@@ -183,19 +195,12 @@ bool TextAutosizer::processSubtree(RenderObject*
layoutRoot)
 {
     TRACE_EVENT0("webkit", "TextAutosizer::processSubtree");
 
-    if (!m_document->settings() || layoutRoot->view()->document().printing() ||
!m_document->page())
-        return false;
-
-    bool textAutosizingEnabled =
m_document->settings()->textAutosizingEnabled();
-    if (!textAutosizingEnabled)
+    if (!isApplicable() || layoutRoot->view()->document().printing())
         return false;
 
     LocalFrame* mainFrame = m_document->page()->mainFrame();
     TextAutosizingWindowInfo windowInfo;
 
-    if (!mainFrame->loader().stateMachine()->committedFirstRealDocumentLoad())
-        return false;
-
     InspectorInstrumentation::willAutosizeText(layoutRoot);
 
     // Window area, in logical (density-independent) pixels.

[timvolodine, 2014-03-19 17:53:23]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-03-19 17:53:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/189113014/150001

[commit-bot: I haz the power, 2014-03-19 18:50:30]

Change committed as 169572