Revert of Improve how the column balancer handles top margins on floats.

Revert of Improve how the column balancer handles top margins on floats. (patchset #3 id:40001 of https://codereview.chromium.org/2465363003/ )

Reason for revert:
Speculative revert to fix csspaint/invalidation-background-image.html failure on "WebKit Win7 (dbg)" bot.

Failed build:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29/builds/7961

Original issue's description:
> Improve how the column balancer handles top margins on floats.
>
> Float margins do not collapse with column boundaries, so we should make room
> for them after the break, if the border box of the float starts in the next
> column.
>
> Let the balancer work on the margin box of the float (and the border box for
> all other objects). For floats, we want to insert breaks before the
> margin-before edge, not the border-before edge. This lets us remove
> some special-code for unbreakable floats in InitialColumnHeightFinder, which
> was the only place that previously bothered about this.
>
> Changed how we determine which objects to process. We used to include the
> overflow both before and after the border box, but we really don't have to
> bother with content preceding it, since that shouldn't undergo fragmentation
> anyway.
>
> Discovered (one test regressed) that logicalHeightIncludingOverflow() also
> included clipped overflow, which certainly wasn't the intention. This didn't
> make much of a difference as long as the method was only called to check if we
> could skip re-layout. But now we also use it to determine the column height.
> Fixed it to only include visible overflow and renamed it to
> logicalHeightWithVisibleOverflow().
>
> Committed: https://crrev.com/7c82da727f64121aa34aa1decf82452c37ef7a2d
> Cr-Commit-Position: refs/heads/master@{#429245}

TBR=eae@chromium.org,mstensho@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Committed: https://crrev.com/58f81484437d367285de9f0fc1fdd4034eb5c333
Cr-Commit-Position: refs/heads/master@{#429265}

[please use gerrit instead, 2016-11-02 13:43:55]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-11-02 13:43:56]

Created Revert of Improve how the column balancer handles top margins on floats.

[commit-bot: I haz the power, 2016-11-02 13:44:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 13:45:22]

Description was changed from

==========
Revert of Improve how the column balancer handles top margins on floats.
(patchset #3 id:40001 of https://codereview.chromium.org/2465363003/ )

Reason for revert:
Speculative revert to fix csspaint/invalidation-background-image.html failure on
"WebKit Win7 (dbg)" bot.

Failed build:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

Original issue's description:
> Improve how the column balancer handles top margins on floats.
> 
> Float margins do not collapse with column boundaries, so we should make room
> for them after the break, if the border box of the float starts in the next
> column.
> 
> Let the balancer work on the margin box of the float (and the border box for
> all other objects). For floats, we want to insert breaks before the
> margin-before edge, not the border-before edge. This lets us remove
> some special-code for unbreakable floats in InitialColumnHeightFinder, which
> was the only place that previously bothered about this.
> 
> Changed how we determine which objects to process. We used to include the
> overflow both before and after the border box, but we really don't have to
> bother with content preceding it, since that shouldn't undergo fragmentation
> anyway.
> 
> Discovered (one test regressed) that logicalHeightIncludingOverflow() also
> included clipped overflow, which certainly wasn't the intention. This didn't
> make much of a difference as long as the method was only called to check if we
> could skip re-layout. But now we also use it to determine the column height.
> Fixed it to only include visible overflow and renamed it to
> logicalHeightWithVisibleOverflow().
> 
> Committed: https://crrev.com/7c82da727f64121aa34aa1decf82452c37ef7a2d
> Cr-Commit-Position: refs/heads/master@{#429245}

TBR=eae@chromium.org,mstensho@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
==========

to

==========
Revert of Improve how the column balancer handles top margins on floats.
(patchset #3 id:40001 of https://codereview.chromium.org/2465363003/ )

Reason for revert:
Speculative revert to fix csspaint/invalidation-background-image.html failure on
"WebKit Win7 (dbg)" bot.

Failed build:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

Original issue's description:
> Improve how the column balancer handles top margins on floats.
> 
> Float margins do not collapse with column boundaries, so we should make room
> for them after the break, if the border box of the float starts in the next
> column.
> 
> Let the balancer work on the margin box of the float (and the border box for
> all other objects). For floats, we want to insert breaks before the
> margin-before edge, not the border-before edge. This lets us remove
> some special-code for unbreakable floats in InitialColumnHeightFinder, which
> was the only place that previously bothered about this.
> 
> Changed how we determine which objects to process. We used to include the
> overflow both before and after the border box, but we really don't have to
> bother with content preceding it, since that shouldn't undergo fragmentation
> anyway.
> 
> Discovered (one test regressed) that logicalHeightIncludingOverflow() also
> included clipped overflow, which certainly wasn't the intention. This didn't
> make much of a difference as long as the method was only called to check if we
> could skip re-layout. But now we also use it to determine the column height.
> Fixed it to only include visible overflow and renamed it to
> logicalHeightWithVisibleOverflow().
> 
> Committed: https://crrev.com/7c82da727f64121aa34aa1decf82452c37ef7a2d
> Cr-Commit-Position: refs/heads/master@{#429245}

TBR=eae@chromium.org,mstensho@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
==========

[commit-bot: I haz the power, 2016-11-02 13:45:23]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-02 13:47:14]

Description was changed from

==========
Revert of Improve how the column balancer handles top margins on floats.
(patchset #3 id:40001 of https://codereview.chromium.org/2465363003/ )

Reason for revert:
Speculative revert to fix csspaint/invalidation-background-image.html failure on
"WebKit Win7 (dbg)" bot.

Failed build:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

Original issue's description:
> Improve how the column balancer handles top margins on floats.
> 
> Float margins do not collapse with column boundaries, so we should make room
> for them after the break, if the border box of the float starts in the next
> column.
> 
> Let the balancer work on the margin box of the float (and the border box for
> all other objects). For floats, we want to insert breaks before the
> margin-before edge, not the border-before edge. This lets us remove
> some special-code for unbreakable floats in InitialColumnHeightFinder, which
> was the only place that previously bothered about this.
> 
> Changed how we determine which objects to process. We used to include the
> overflow both before and after the border box, but we really don't have to
> bother with content preceding it, since that shouldn't undergo fragmentation
> anyway.
> 
> Discovered (one test regressed) that logicalHeightIncludingOverflow() also
> included clipped overflow, which certainly wasn't the intention. This didn't
> make much of a difference as long as the method was only called to check if we
> could skip re-layout. But now we also use it to determine the column height.
> Fixed it to only include visible overflow and renamed it to
> logicalHeightWithVisibleOverflow().
> 
> Committed: https://crrev.com/7c82da727f64121aa34aa1decf82452c37ef7a2d
> Cr-Commit-Position: refs/heads/master@{#429245}

TBR=eae@chromium.org,mstensho@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
==========

to

==========
Revert of Improve how the column balancer handles top margins on floats.
(patchset #3 id:40001 of https://codereview.chromium.org/2465363003/ )

Reason for revert:
Speculative revert to fix csspaint/invalidation-background-image.html failure on
"WebKit Win7 (dbg)" bot.

Failed build:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

Original issue's description:
> Improve how the column balancer handles top margins on floats.
>
> Float margins do not collapse with column boundaries, so we should make room
> for them after the break, if the border box of the float starts in the next
> column.
>
> Let the balancer work on the margin box of the float (and the border box for
> all other objects). For floats, we want to insert breaks before the
> margin-before edge, not the border-before edge. This lets us remove
> some special-code for unbreakable floats in InitialColumnHeightFinder, which
> was the only place that previously bothered about this.
>
> Changed how we determine which objects to process. We used to include the
> overflow both before and after the border box, but we really don't have to
> bother with content preceding it, since that shouldn't undergo fragmentation
> anyway.
>
> Discovered (one test regressed) that logicalHeightIncludingOverflow() also
> included clipped overflow, which certainly wasn't the intention. This didn't
> make much of a difference as long as the method was only called to check if we
> could skip re-layout. But now we also use it to determine the column height.
> Fixed it to only include visible overflow and renamed it to
> logicalHeightWithVisibleOverflow().
>
> Committed: https://crrev.com/7c82da727f64121aa34aa1decf82452c37ef7a2d
> Cr-Commit-Position: refs/heads/master@{#429245}

TBR=eae@chromium.org,mstensho@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Committed: https://crrev.com/58f81484437d367285de9f0fc1fdd4034eb5c333
Cr-Commit-Position: refs/heads/master@{#429265}
==========

[commit-bot: I haz the power, 2016-11-02 13:47:15]

Patchset 1 (id:??) landed as
https://crrev.com/58f81484437d367285de9f0fc1fdd4034eb5c333
Cr-Commit-Position: refs/heads/master@{#429265}

[mstensho (USE GERRIT), 2016-11-02 13:57:24]

On 2016/11/02 13:43:56, rouslan wrote:
> Created Revert of Improve how the column balancer handles top margins on
floats.

Will you reland it if it wasn't the culprit? I'm asking, because I'm pretty sure
that this CL was innocent (I see no multicol in the failing test), and also
because I have other patches in the works depending on this CL.

[please use gerrit instead, 2016-11-02 13:59:44]

On 2016/11/02 13:57:24, mstensho wrote:
> On 2016/11/02 13:43:56, rouslan wrote:
> > Created Revert of Improve how the column balancer handles top margins on
> floats.
> 
> Will you reland it if it wasn't the culprit? I'm asking, because I'm pretty
sure
> that this CL was innocent (I see no multicol in the failing test), and also
> because I have other patches in the works depending on this CL.

Yes, I will reland if it's not the culprit.

[eae, 2016-11-02 15:39:39]

LGTM

[please use gerrit instead, 2016-11-02 18:12:44]

So far 1 build with the revert has passed:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

I will continue to monitor consequent builds for
csspaint/invalidation-background-image.html failures.

[please use gerrit instead, 2016-11-02 21:09:53]

2 builds ran so far and both passed. Sorry, this bot is slow.

[please use gerrit instead, 2016-11-02 21:51:38]

The 3rd build failed with
virtual/service-worker-navigation-preload/http/tests/serviceworker/foreign-fetch-cors.html
, but csspaint/invalidation-background-image.html is still passing. If you
strongly feel that your patch did not cause
csspaint/invalidation-background-image.html failure, then you can reland it, but
please be sure to keep an eye on
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29
to make sure that csspaint/invalidation-background-image.html does not start
failing again. Thank you!

[mstensho (USE GERRIT), 2016-11-02 22:00:49]

On 2016/11/02 21:51:38, rouslan wrote:
> The 3rd build failed with
>
virtual/service-worker-navigation-preload/http/tests/serviceworker/foreign-fetch-cors.html
> , but csspaint/invalidation-background-image.html is still passing. If you
> strongly feel that your patch did not cause
> csspaint/invalidation-background-image.html failure, then you can reland it,
but
> please be sure to keep an eye on
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29
> to make sure that csspaint/invalidation-background-image.html does not start
> failing again. Thank you!

Seeing that csspaint/invalidation-background-image.html also failed before my
patch landed [1], I feel pretty innocent. :)

[1]
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

What's the best way of relanding? Clicking "Revert Patchset" here?

[please use gerrit instead, 2016-11-02 22:03:39]

Yes

On Nov 2, 2016 6:00 PM, <mstensho@opera.com> wrote:

> On 2016/11/02 21:51:38, rouslan wrote:
> > The 3rd build failed with
> >
> virtual/service-worker-navigation-preload/http/tests/
> serviceworker/foreign-fetch-cors.html
> > , but csspaint/invalidation-background-image.html is still passing. If
> you
> > strongly feel that your patch did not cause
> > csspaint/invalidation-background-image.html failure, then you can
> reland it,
> but
> > please be sure to keep an eye on
> >
> https://build.chromium.org/p/chromium.webkit/builders/
> WebKit%20Win7%20%28dbg%29
> > to make sure that csspaint/invalidation-background-image.html does not
> start
> > failing again. Thank you!
>
> Seeing that csspaint/invalidation-background-image.html also failed
> before my
> patch landed [1], I feel pretty innocent. :)
>
> [1]
> https://build.chromium.org/p/chromium.webkit/builders/
> WebKit%20Win7%20%28dbg%29/builds/7949
>
> What's the best way of relanding? Clicking "Revert Patchset" here?
>
> https://codereview.chromium.org/2468193002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[please use gerrit instead, 2016-11-02 22:03:39]

Yes

On Nov 2, 2016 6:00 PM, <mstensho@opera.com> wrote:

> On 2016/11/02 21:51:38, rouslan wrote:
> > The 3rd build failed with
> >
> virtual/service-worker-navigation-preload/http/tests/
> serviceworker/foreign-fetch-cors.html
> > , but csspaint/invalidation-background-image.html is still passing. If
> you
> > strongly feel that your patch did not cause
> > csspaint/invalidation-background-image.html failure, then you can
> reland it,
> but
> > please be sure to keep an eye on
> >
> https://build.chromium.org/p/chromium.webkit/builders/
> WebKit%20Win7%20%28dbg%29
> > to make sure that csspaint/invalidation-background-image.html does not
> start
> > failing again. Thank you!
>
> Seeing that csspaint/invalidation-background-image.html also failed
> before my
> patch landed [1], I feel pretty innocent. :)
>
> [1]
> https://build.chromium.org/p/chromium.webkit/builders/
> WebKit%20Win7%20%28dbg%29/builds/7949
>
> What's the best way of relanding? Clicking "Revert Patchset" here?
>
> https://codereview.chromium.org/2468193002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mstensho (USE GERRIT), 2016-11-02 22:10:38]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2471933002/ by mstensho@opera.com.

The reason for reverting is: csspaint/invalidation-background-image.html was
also failing (flaky) before landing this CL..