Avoid overflow recalc on tables if sections need rebuild

Avoid overflow recalc on tables if sections need rebuild

A second go at https://codereview.chromium.org/1901203002. What I really should
have done in that CL is return early if the table sections needed rebuilding.
There are legitimate cases where a table needs layout but may not need to layout
its sections.

BUG=609018
Committed: https://crrev.com/806f45bd9dd5e39cdc20009f2b03bfa9d24566e0
Cr-Commit-Position: refs/heads/master@{#394871}

[rhogan, 2016-05-04 20:23:41]

Description was changed from

==========
Avoid overflow recalc on tables if sections need rebuild

BUG=609018
==========

to

==========
Avoid overflow recalc on tables if sections need rebuild

A second go at https://codereview.chromium.org/1901203002. What I really should
have done in that CL is return early if the table sections needed rebuilding.
There are legitimate cases where a table needs layout but may not need to layout
its sections.

BUG=609018
==========

[rhogan, 2016-05-05 11:47:34]

robhogan@gmail.com changed reviewers:
+ wangxianzhu@chromium.org

[rhogan, 2016-05-05 11:47:35]

The test isn't quite right I think. Could you suggest an improvement?

[Xianzhu, 2016-05-05 16:01:33]

https://codereview.chromium.org/1946413002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/repaint/checkbox-selection-rect-in-cell-expected.txt
(right):

https://codereview.chromium.org/1946413002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/repaint/checkbox-selection-rect-in-cell-expected.txt:2:
"bounds": [800, 600],
What is the expectation without this CL?

https://codereview.chromium.org/1946413002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/repaint/checkbox-selection-rect-in-cell.html
(right):

https://codereview.chromium.org/1946413002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/repaint/checkbox-selection-rect-in-cell.html:6:
document.getElementById("a").focus();
What will these affect? Why two focus calls?

[rhogan, 2016-05-05 18:30:11]

https://codereview.chromium.org/1946413002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/repaint/checkbox-selection-rect-in-cell.html
(right):

https://codereview.chromium.org/1946413002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/repaint/checkbox-selection-rect-in-cell.html:6:
document.getElementById("a").focus();
On 2016/05/05 at 16:01:33, Xianzhu wrote:
> What will these affect? Why two focus calls?

I want to mimic the effect of #b gaining then losing focus - so I can show that
the repaints of the focus ring are effective.

[Xianzhu, 2016-05-05 18:35:49]

https://codereview.chromium.org/1946413002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/repaint/checkbox-selection-rect-in-cell.html
(right):

https://codereview.chromium.org/1946413002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/repaint/checkbox-selection-rect-in-cell.html:6:
document.getElementById("a").focus();
On 2016/05/05 18:30:10, rhogan wrote:
> On 2016/05/05 at 16:01:33, Xianzhu wrote:
> > What will these affect? Why two focus calls?
> 
> I want to mimic the effect of #b gaining then losing focus - so I can show
that
> the repaints of the focus ring are effective.

Does it really work to focus and unfocus in the same cycle?

What will this test produce without the patch?

[rhogan, 2016-05-05 18:44:04]

On 2016/05/05 at 18:35:49, wangxianzhu wrote:
> What will this test produce without the patch?

---
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-expected.txt
+++
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-actual.txt
@@ -18,7 +18,7 @@
         },
         {
           "object": "LayoutBlockFlow INPUT id='b'",
-          "rect": [44, 58, 14, 13],
+          "rect": [43, 57, 16, 15],
           "reason": "full"
         },
         {
Works fine on Linux. Maybe I just need Mac/Win specific expectations?

[rhogan, 2016-05-05 18:55:49]

On 2016/05/05 at 18:44:04, rhogan wrote:
> On 2016/05/05 at 18:35:49, wangxianzhu wrote:
> > What will this test produce without the patch?
> 
> ---
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-expected.txt
> +++
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-actual.txt
> @@ -18,7 +18,7 @@
>          },
>          {
>            "object": "LayoutBlockFlow INPUT id='b'",
> -          "rect": [44, 58, 14, 13],
> +          "rect": [43, 57, 16, 15],
>            "reason": "full"
>          },
>          {
> Works fine on Linux. Maybe I just need Mac/Win specific expectations?

No, I need to fix this test. Using the repaint framework stops it from failing
properly without the CL.

[rhogan, 2016-05-05 19:14:17]

On 2016/05/05 at 18:55:49, rhogan wrote:
> On 2016/05/05 at 18:44:04, rhogan wrote:
> > On 2016/05/05 at 18:35:49, wangxianzhu wrote:
> > > What will this test produce without the patch?
> > 
> > ---
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-expected.txt
> > +++
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-actual.txt
> > @@ -18,7 +18,7 @@
> >          },
> >          {
> >            "object": "LayoutBlockFlow INPUT id='b'",
> > -          "rect": [44, 58, 14, 13],
> > +          "rect": [43, 57, 16, 15],
> >            "reason": "full"
> >          },
> >          {
> > Works fine on Linux. Maybe I just need Mac/Win specific expectations?
> 
> No, I need to fix this test. Using the repaint framework stops it from failing
properly without the CL.

OK, I can recreate the bug manually (without my CL) in chromium but not
content_shell. Tabbing between the two elements shows it:

<!DOCTYPE html>
<table>
    <tr>
        <td>
            <a href="#" id="a">ENABLE</a>
            <input id="b" type="checkbox">
        </td>
    </tr>
</table>

I now see I can only recreate the bug in chromium. What do you suggest?

[Xianzhu, 2016-05-05 19:43:21]

On 2016/05/05 19:14:17, rhogan wrote:
> On 2016/05/05 at 18:55:49, rhogan wrote:
> > On 2016/05/05 at 18:44:04, rhogan wrote:
> > > On 2016/05/05 at 18:35:49, wangxianzhu wrote:
> > > > What will this test produce without the patch?
> > > 
> > > ---
>
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-expected.txt
> > > +++
>
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-actual.txt
> > > @@ -18,7 +18,7 @@
> > >          },
> > >          {
> > >            "object": "LayoutBlockFlow INPUT id='b'",
> > > -          "rect": [44, 58, 14, 13],
> > > +          "rect": [43, 57, 16, 15],
> > >            "reason": "full"
> > >          },
> > >          {
> > > Works fine on Linux. Maybe I just need Mac/Win specific expectations?
> > 
> > No, I need to fix this test. Using the repaint framework stops it from
failing
> properly without the CL.
> 
> OK, I can recreate the bug manually (without my CL) in chromium but not
> content_shell. Tabbing between the two elements shows it:
> 
> <!DOCTYPE html>
> <table>
>     <tr>
>         <td>
>             <a href="#" id="a">ENABLE</a>
>             <input id="b" type="checkbox">
>         </td>
>     </tr>
> </table>
> 
> I now see I can only recreate the bug in chromium. What do you suggest?

This is why I asked you the question "does it really work to focus and unfocus
in the same frame". I think the test will work if you separate focusing and
unfocusing into different cycles (using runAfterLayoutAndPaint() in
resources/run-after-layout-and-paint.js), like this:
<script src="../../resources/text-based-repaint.js"></script>
<script>
function repaintTest() {
  document.getElementById('b').focus();
}
runAfterLayoutAndPaint(function() {
  document.getElementById('a').focus();
  runRepaintTest();
});
</script>

However, I think a unit test would be better to test directly the result of
overflow recalc. You can use LayoutTableCellTest.cpp as a reference to create
LayoutTableTest.cpp to contain the new unit test.

[rhogan, 2016-05-05 20:21:39]

On 2016/05/05 at 19:43:21, wangxianzhu wrote:
> On 2016/05/05 19:14:17, rhogan wrote:
> > On 2016/05/05 at 18:55:49, rhogan wrote:
> > > On 2016/05/05 at 18:44:04, rhogan wrote:
> > > > On 2016/05/05 at 18:35:49, wangxianzhu wrote:
> > > > > What will this test produce without the patch?
> > > > 
> > > > ---
> >
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-expected.txt
> > > > +++
> >
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-actual.txt
> > > > @@ -18,7 +18,7 @@
> > > >          },
> > > >          {
> > > >            "object": "LayoutBlockFlow INPUT id='b'",
> > > > -          "rect": [44, 58, 14, 13],
> > > > +          "rect": [43, 57, 16, 15],
> > > >            "reason": "full"
> > > >          },
> > > >          {
> > > > Works fine on Linux. Maybe I just need Mac/Win specific expectations?
> > > 
> > > No, I need to fix this test. Using the repaint framework stops it from
failing
> > properly without the CL.
> > 
> > OK, I can recreate the bug manually (without my CL) in chromium but not
> > content_shell. Tabbing between the two elements shows it:
> > 
> > <!DOCTYPE html>
> > <table>
> >     <tr>
> >         <td>
> >             <a href="#" id="a">ENABLE</a>
> >             <input id="b" type="checkbox">
> >         </td>
> >     </tr>
> > </table>
> > 
> > I now see I can only recreate the bug in chromium. What do you suggest?
> 
> This is why I asked you the question "does it really work to focus and unfocus
in the same frame". I think the test will work if you separate focusing and
unfocusing into different cycles (using runAfterLayoutAndPaint() in
resources/run-after-layout-and-paint.js), like this:
> <script src="../../resources/text-based-repaint.js"></script>
> <script>
> function repaintTest() {
>   document.getElementById('b').focus();
> }
> runAfterLayoutAndPaint(function() {
>   document.getElementById('a').focus();
>   runRepaintTest();
> });
> </script>
> 
> However, I think a unit test would be better to test directly the result of
overflow recalc. You can use LayoutTableCellTest.cpp as a reference to create
LayoutTableTest.cpp to contain the new unit test.

Yikes - do you have a pointer for something similar? I've never managed to get a
unit test working on a rendering issue and this seems like a really challenging
candidate.

[Xianzhu, 2016-05-05 20:35:54]

On 2016/05/05 20:21:39, rhogan wrote:
> On 2016/05/05 at 19:43:21, wangxianzhu wrote:
> > On 2016/05/05 19:14:17, rhogan wrote:
> > > On 2016/05/05 at 18:55:49, rhogan wrote:
> > > > On 2016/05/05 at 18:44:04, rhogan wrote:
> > > > > On 2016/05/05 at 18:35:49, wangxianzhu wrote:
> > > > > > What will this test produce without the patch?
> > > > > 
> > > > > ---
> > >
>
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-expected.txt
> > > > > +++
> > >
>
/home/robert/Dev/blink/src/out/Release/layout-test-results/fast/repaint/checkbox-selection-rect-in-cell-actual.txt
> > > > > @@ -18,7 +18,7 @@
> > > > >          },
> > > > >          {
> > > > >            "object": "LayoutBlockFlow INPUT id='b'",
> > > > > -          "rect": [44, 58, 14, 13],
> > > > > +          "rect": [43, 57, 16, 15],
> > > > >            "reason": "full"
> > > > >          },
> > > > >          {
> > > > > Works fine on Linux. Maybe I just need Mac/Win specific expectations?
> > > > 
> > > > No, I need to fix this test. Using the repaint framework stops it from
> failing
> > > properly without the CL.
> > > 
> > > OK, I can recreate the bug manually (without my CL) in chromium but not
> > > content_shell. Tabbing between the two elements shows it:
> > > 
> > > <!DOCTYPE html>
> > > <table>
> > >     <tr>
> > >         <td>
> > >             <a href="#" id="a">ENABLE</a>
> > >             <input id="b" type="checkbox">
> > >         </td>
> > >     </tr>
> > > </table>
> > > 
> > > I now see I can only recreate the bug in chromium. What do you suggest?
> > 
> > This is why I asked you the question "does it really work to focus and
unfocus
> in the same frame". I think the test will work if you separate focusing and
> unfocusing into different cycles (using runAfterLayoutAndPaint() in
> resources/run-after-layout-and-paint.js), like this:
> > <script src="../../resources/text-based-repaint.js"></script>
> > <script>
> > function repaintTest() {
> >   document.getElementById('b').focus();
> > }
> > runAfterLayoutAndPaint(function() {
> >   document.getElementById('a').focus();
> >   runRepaintTest();
> > });
> > </script>
> > 
> > However, I think a unit test would be better to test directly the result of
> overflow recalc. You can use LayoutTableCellTest.cpp as a reference to create
> LayoutTableTest.cpp to contain the new unit test.
> 
> Yikes - do you have a pointer for something similar? I've never managed to get
a
> unit test working on a rendering issue and this seems like a really
challenging
> candidate.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
(getLayoutObjectByElementById() is more convenient to get a layout object than
through firstChild() chain in the tests.)

[rhogan, 2016-05-15 11:54:22]

On 2016/05/05 at 20:35:54, wangxianzhu wrote:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> (getLayoutObjectByElementById() is more convenient to get a layout object than
through firstChild() chain in the tests.)

Couldn't get this to work for me - it gives the same result with and without may
patch, I'd expect a larger invalidation rect with my patch applied. Any
pointers?

TEST_F(LayoutTableCellTest, RepaintContentInTableCell)
{   
     const char* bodyContent = "<table>"
        "<tr>"
        "<td>" 
        "<a href='#' id='href'>ENABLE</a>"
        "<input id='input' type='checkbox'>"
        "</td>"
        "</tr>"
        "</table>";
    setBodyInnerHTML(bodyContent);
    
    Element* input = document().getElementById(AtomicString("input"));
    
    document().page()->focusController().setActive(true);
    document().page()->focusController().setFocused(true);

    // Do focus.
    input->focus();
    document().view()->updateAllLifecyclePhases();
    input->blur();
    
    LayoutBlock* inputBlock =
toLayoutBlock(getLayoutObjectByElementId("input"));
    LayoutRect rect = inputBlock->localOverflowRectForPaintInvalidation();
    EXPECT_EQ(rect, LayoutRect(0, 0, 13, 13));
}

[Xianzhu, 2016-05-16 21:28:08]

Now I'm a bit confused by what the CL does. I know it avoids overflow recalc in
the case, but why overflow recalc got wrong result? Even if it is wrong,
wouldn't the relayout recalc and overwrite the calculated overflow?

[rhogan, 2016-05-16 21:46:54]

On 2016/05/16 at 21:28:08, wangxianzhu wrote:
> Now I'm a bit confused by what the CL does. I know it avoids overflow recalc
in the case, but why overflow recalc got wrong result? Even if it is wrong,
wouldn't the relayout recalc and overwrite the calculated overflow?

If the table needs layout, but doesn't need to rebuild its sections, it may not
visit all of those sections and recalculate their overflow when it does the
layout. For example, it may only need a simplifiedLayout(). The section itself
may not need a layout.

So we still want to recalc the overflow on sections if the table needs layout:
we can't be sure that the section/row/cell in need of a recalc will be visited
by layout.

[Xianzhu, 2016-05-16 21:51:27]

On 2016/05/16 21:46:54, rhogan wrote:
> On 2016/05/16 at 21:28:08, wangxianzhu wrote:
> > Now I'm a bit confused by what the CL does. I know it avoids overflow recalc
> in the case, but why overflow recalc got wrong result? Even if it is wrong,
> wouldn't the relayout recalc and overwrite the calculated overflow?
> 
> If the table needs layout, but doesn't need to rebuild its sections, it may
not
> visit all of those sections and recalculate their overflow when it does the
> layout. For example, it may only need a simplifiedLayout(). The section itself
> may not need a layout.
> 
> So we still want to recalc the overflow on sections if the table needs layout:
> we can't be sure that the section/row/cell in need of a recalc will be visited
> by layout.

I see. So with this patch we cover more cases to recalc overflow. The title of
the CL seems confusing. Would it be better to be "Ensure overflow recalc if
table needs layout but section doesn't".

Will look at the unit test issue asap.

[Xianzhu, 2016-05-16 22:20:28]

On 2016/05/15 11:54:22, rhogan wrote:
> On 2016/05/05 at 20:35:54, wangxianzhu wrote:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > (getLayoutObjectByElementById() is more convenient to get a layout object
than
> through firstChild() chain in the tests.)
> 
> Couldn't get this to work for me - it gives the same result with and without
may
> patch, I'd expect a larger invalidation rect with my patch applied. Any
> pointers?
> 
> TEST_F(LayoutTableCellTest, RepaintContentInTableCell)
> {   
>      const char* bodyContent = "<table>"
>         "<tr>"
>         "<td>" 
>         "<a href='#' id='href'>ENABLE</a>"
>         "<input id='input' type='checkbox'>"
>         "</td>"
>         "</tr>"
>         "</table>";
>     setBodyInnerHTML(bodyContent);
>     
>     Element* input = document().getElementById(AtomicString("input"));
>     
>     document().page()->focusController().setActive(true);
>     document().page()->focusController().setFocused(true);
> 
>     // Do focus.
>     input->focus();
>     document().view()->updateAllLifecyclePhases();
>     input->blur();

The style recalc and overflow recalc are done before the above statement
returns, so 'input' is already in blurred status and has overflow not having
focus rings.

>
>     LayoutBlock* inputBlock =
> toLayoutBlock(getLayoutObjectByElementId("input"));
>     LayoutRect rect = inputBlock->localOverflowRectForPaintInvalidation();
>     EXPECT_EQ(rect, LayoutRect(0, 0, 13, 13));
> }

I think the test can be much simplified if you just change 'outline' of a table
cell, and trigger table layout (but not section recalc), then update lifecycle
and check if overflow of the table cell is recalculated.

[rhogan, 2016-05-19 19:19:51]

On 2016/05/16 at 22:20:28, wangxianzhu wrote:
> I think the test can be much simplified if you just change 'outline' of a
table cell, and trigger table layout (but not section recalc), then update
lifecycle and check if overflow of the table cell is recalculated.

Yup that works. OK to take another look?

[Xianzhu, 2016-05-19 20:32:50]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-05-19 20:32:51]

lgtm

[commit-bot: I haz the power, 2016-05-19 20:33:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1946413002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1946413002/100001

[commit-bot: I haz the power, 2016-05-19 21:28:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-19 21:29:31]

Description was changed from

==========
Avoid overflow recalc on tables if sections need rebuild

A second go at https://codereview.chromium.org/1901203002. What I really should
have done in that CL is return early if the table sections needed rebuilding.
There are legitimate cases where a table needs layout but may not need to layout
its sections.

BUG=609018
==========

to

==========
Avoid overflow recalc on tables if sections need rebuild

A second go at https://codereview.chromium.org/1901203002. What I really should
have done in that CL is return early if the table sections needed rebuilding.
There are legitimate cases where a table needs layout but may not need to layout
its sections.

BUG=609018

Committed: https://crrev.com/806f45bd9dd5e39cdc20009f2b03bfa9d24566e0
Cr-Commit-Position: refs/heads/master@{#394871}
==========

[commit-bot: I haz the power, 2016-05-19 21:29:32]

Patchset 6 (id:??) landed as
https://crrev.com/806f45bd9dd5e39cdc20009f2b03bfa9d24566e0
Cr-Commit-Position: refs/heads/master@{#394871}