Avoid unnecessary invalidations on resize.

Avoid unnecessary invalidations on resize.

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

[skobes, 2014-03-22 06:23:43]

Based on http://crrev.com/209353003.

[pdr., 2014-03-25 22:23:45]

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

https://codereview.chromium.org/209413002/diff/50001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:1002: {
Should we bail here if !m_mainFrame?

https://codereview.chromium.org/209413002/diff/50001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:1003: if (FastTextAutosizer*
textAutosizer = m_mainFrame->document()->fastTextAutosizer()) {
I'm a little worried about use-after-frees of m_mainFrame (e.g., see comment in
DOMWindow::find). Can m_mainFrame be a refptr?

[skobes, 2014-03-26 00:36:50]

+abarth for Source/web OWNERS.

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

https://codereview.chromium.org/209413002/diff/50001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:1002: {
On 2014/03/25 22:23:45, pdr wrote:
> Should we bail here if !m_mainFrame?

Good catch, done.

https://codereview.chromium.org/209413002/diff/50001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:1003: if (FastTextAutosizer*
textAutosizer = m_mainFrame->document()->fastTextAutosizer()) {
On 2014/03/25 22:23:45, pdr wrote:
> I'm a little worried about use-after-frees of m_mainFrame (e.g., see comment
in
> DOMWindow::find). Can m_mainFrame be a refptr?

Done.

[pdr., 2014-03-26 01:08:23]

On 2014/03/26 00:36:50, skobes wrote:
> +abarth for Source/web OWNERS.
> 
>
https://codereview.chromium.org/209413002/diff/50001/Source/core/rendering/Fa...
> File Source/core/rendering/FastTextAutosizer.cpp (right):
> 
>
https://codereview.chromium.org/209413002/diff/50001/Source/core/rendering/Fa...
> Source/core/rendering/FastTextAutosizer.cpp:1002: {
> On 2014/03/25 22:23:45, pdr wrote:
> > Should we bail here if !m_mainFrame?
> 
> Good catch, done.
> 
>
https://codereview.chromium.org/209413002/diff/50001/Source/core/rendering/Fa...
> Source/core/rendering/FastTextAutosizer.cpp:1003: if (FastTextAutosizer*
> textAutosizer = m_mainFrame->document()->fastTextAutosizer()) {
> On 2014/03/25 22:23:45, pdr wrote:
> > I'm a little worried about use-after-frees of m_mainFrame (e.g., see comment
> in
> > DOMWindow::find). Can m_mainFrame be a refptr?
> 
> Done.

LGTM for FastTextAutosizer

[pdr., 2014-03-26 03:02:21]

https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:294: ASSERT(enabled() &&
m_pageNeedsAutosizing && !m_updatePageInfoDeferred);
This ends up firing on nearly every page :(

[abarth-chromium, 2014-03-26 05:29:36]

https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:995: return;
Can this actually happen?

https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:998:
textAutosizer->m_updatePageInfoDeferred = true;
Should we assert that this value is false?  If this object is nested inside
itself, you'll incorrectly set this value to false in the destructor.

[skobes, 2014-03-28 01:00:29]

https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:294: ASSERT(enabled() &&
m_pageNeedsAutosizing && !m_updatePageInfoDeferred);
On 2014/03/26 03:02:22, pdr wrote:
> This ends up firing on nearly every page :(

What page does it fire on for you?  I'm not seeing it on my N7.

https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:995: return;
On 2014/03/26 05:29:37, abarth wrote:
> Can this actually happen?

Perhaps not.  Removed.

https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:998:
textAutosizer->m_updatePageInfoDeferred = true;
On 2014/03/26 05:29:37, abarth wrote:
> Should we assert that this value is false?  If this object is nested inside
> itself, you'll incorrectly set this value to false in the destructor.

Done.

[pdr., 2014-03-28 01:45:28]

On 2014/03/28 01:00:29, skobes wrote:
>
https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
> File Source/core/rendering/FastTextAutosizer.cpp (right):
> 
>
https://codereview.chromium.org/209413002/diff/150001/Source/core/rendering/F...
> Source/core/rendering/FastTextAutosizer.cpp:294: ASSERT(enabled() &&
> m_pageNeedsAutosizing && !m_updatePageInfoDeferred);
> On 2014/03/26 03:02:22, pdr wrote:
> > This ends up firing on nearly every page :(
> 
> What page does it fire on for you?  I'm not seeing it on my N7.

I was seeing it on desktop when I applied this patch. If it's not occurring for
you, it was probably a bad merge on my part.

LGTM

[skobes, 2014-04-01 00:57:32]

On 2014/03/28 01:45:28, pdr wrote:
> I was seeing it on desktop when I applied this patch. If it's not occurring
for
> you, it was probably a bad merge on my part.

I verified the assertion does not fire on desktop.

@abarth does this change look ok to you now?

[abarth-chromium, 2014-04-01 04:56:27]

lgtm

https://codereview.chromium.org/209413002/diff/240001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/209413002/diff/240001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:1003:
textAutosizer->m_updatePageInfoDeferred = false;
Should we assert that m_updatePageInfoDeferred is true?

[skobes, 2014-04-01 18:18:29]

https://codereview.chromium.org/209413002/diff/240001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/209413002/diff/240001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:1003:
textAutosizer->m_updatePageInfoDeferred = false;
On 2014/04/01 04:56:28, abarth wrote:
> Should we assert that m_updatePageInfoDeferred is true?

Done.

[skobes, 2014-04-01 18:18:42]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-04-01 18:18:52]

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

[commit-bot: I haz the power, 2014-04-01 19:28:30]

Change committed as 170596