Move runin to original position when sibling element is destroyed. Additionally, if If moveRunInUnd…

Move runin to original position when sibling element is destroyed. Additionally, if If moveRunInUnderSiblingBlockIfNeeded is called when the sibling run-in block is being destroyed, it means that the run-in is moving to original position and we do not need to do nothing.

R=
BUG=260388
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154557

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

[Julien - ping for review, 2013-07-17 00:04:18]

The code change is fine.

https://codereview.chromium.org/19290002/diff/1/LayoutTests/fast/runin/runin-...
File LayoutTests/fast/runin/runin-remove-child-simple-expected.txt (right):

https://codereview.chromium.org/19290002/diff/1/LayoutTests/fast/runin/runin-...
LayoutTests/fast/runin/runin-remove-child-simple-expected.txt:1: This should
The output of this test is just horrible (the other one is not great though):
1. What are you testing? (unless the person is familiar with runins, it's
difficult to see what you test)
2. What is the expected result? (and "This should" is definitely not a
satisfactory output).

Ideally, someone who has no clue about what you are testing should be able to
know if the output fails just by looking at it.

[igoroliveira, 2013-07-17 00:38:12]

On 2013/07/17 00:04:18, Julien Chaffraix wrote:
> The code change is fine.
> 
>
https://codereview.chromium.org/19290002/diff/1/LayoutTests/fast/runin/runin-...
> File LayoutTests/fast/runin/runin-remove-child-simple-expected.txt (right):
> 
>
https://codereview.chromium.org/19290002/diff/1/LayoutTests/fast/runin/runin-...
> LayoutTests/fast/runin/runin-remove-child-simple-expected.txt:1: This should
> The output of this test is just horrible (the other one is not great though):
> 1. What are you testing? (unless the person is familiar with runins, it's
> difficult to see what you test)
> 2. What is the expected result? (and "This should" is definitely not a
> satisfactory output).
> 
> Ideally, someone who has no clue about what you are testing should be able to
> know if the output fails just by looking at it.

Indeed, it is really confusing. I updated the test making the output much
better.

[Julien - ping for review, 2013-07-17 17:28:23]

lgtm but let's have tests with CLEAR descriptions and outcomes.

https://codereview.chromium.org/19290002/diff/6001/LayoutTests/fast/runin/run...
File LayoutTests/fast/runin/runin-remove-child-simple.html (right):

https://codereview.chromium.org/19290002/diff/6001/LayoutTests/fast/runin/run...
LayoutTests/fast/runin/runin-remove-child-simple.html:8: <div>
Please add a description for what you are testing (I would have added an
expected outcome too but here it's probably an overkill), it's always valuable
when you come back 6 months later to know what you were testing.

https://codereview.chromium.org/19290002/diff/6001/LayoutTests/fast/runin/run...
File LayoutTests/fast/runin/runin-sibling-inline-expected.txt (right):

https://codereview.chromium.org/19290002/diff/6001/LayoutTests/fast/runin/run...
LayoutTests/fast/runin/runin-sibling-inline-expected.txt:2: run in.
Still not happy with this one. Your output doesn't make it clear why:

"This should run in."

is not satisfactory for example. Here is a better output:

This test checks that dynamically changing 'display' from block to inline moves
the run-in to its own block.

For this test to pass there should be 2 lines below with PASS, each PASS should
be on its own line:

PASS (run-in)
PASS (div that changed 'display')

[igoroliveira, 2013-07-18 20:45:42]

Ok, i was being lazy :). Updated layout tests again.

On 2013/07/17 17:28:23, Julien Chaffraix wrote:
> lgtm but let's have tests with CLEAR descriptions and outcomes.
> 
>
https://codereview.chromium.org/19290002/diff/6001/LayoutTests/fast/runin/run...
> File LayoutTests/fast/runin/runin-remove-child-simple.html (right):
> 
>
https://codereview.chromium.org/19290002/diff/6001/LayoutTests/fast/runin/run...
> LayoutTests/fast/runin/runin-remove-child-simple.html:8: <div>
> Please add a description for what you are testing (I would have added an
> expected outcome too but here it's probably an overkill), it's always valuable
> when you come back 6 months later to know what you were testing.
> 
>
https://codereview.chromium.org/19290002/diff/6001/LayoutTests/fast/runin/run...
> File LayoutTests/fast/runin/runin-sibling-inline-expected.txt (right):
> 
>
https://codereview.chromium.org/19290002/diff/6001/LayoutTests/fast/runin/run...
> LayoutTests/fast/runin/runin-sibling-inline-expected.txt:2: run in.
> Still not happy with this one. Your output doesn't make it clear why:
> 
> "This should run in."
> 
> is not satisfactory for example. Here is a better output:
> 
> This test checks that dynamically changing 'display' from block to inline
moves
> the run-in to its own block.
> 
> For this test to pass there should be 2 lines below with PASS, each PASS
should
> be on its own line:
> 
> PASS (run-in)
> PASS (div that changed 'display')

[Julien - ping for review, 2013-07-19 00:11:49]

lgtm

Thanks for make some good test cases.

[commit-bot: I haz the power, 2013-07-19 00:11:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/19290002/12001

[commit-bot: I haz the power, 2013-07-19 03:35:29]

Change committed as 154557

[igoroliveira, 2013-07-23 18:50:41]

reopening since it was reverted.
On 2013/07/19 03:35:29, I haz the power (commit-bot) wrote:
> Change committed as 154557

[Julien - ping for review, 2013-07-23 21:40:12]

lgtm

https://codereview.chromium.org/19290002/diff/32001/LayoutTests/fast/runin/ru...
File LayoutTests/fast/runin/run-in-sibling-inline-block.html (right):

https://codereview.chromium.org/19290002/diff/32001/LayoutTests/fast/runin/ru...
LayoutTests/fast/runin/run-in-sibling-inline-block.html:1: <style>
The test is missing a DOCTYPE (or do we *really* need it to run in quirks mode?)

https://codereview.chromium.org/19290002/diff/32001/LayoutTests/fast/runin/ru...
LayoutTests/fast/runin/run-in-sibling-inline-block.html:9: var fieldSet;
We don't need to define these variables here as you only use them inside the
function below.

https://codereview.chromium.org/19290002/diff/32001/LayoutTests/fast/runin/ru...
LayoutTests/fast/runin/run-in-sibling-inline-block.html:22: PASS if test does
not crash.
Instructions should be complete English sentences:

This test has PASSED if it did not crash.

[igoroliveira, 2013-07-23 22:30:25]

On 2013/07/23 21:40:12, Julien Chaffraix wrote:
> lgtm
> 
>
https://codereview.chromium.org/19290002/diff/32001/LayoutTests/fast/runin/ru...
> File LayoutTests/fast/runin/run-in-sibling-inline-block.html (right):
> 
>
https://codereview.chromium.org/19290002/diff/32001/LayoutTests/fast/runin/ru...
> LayoutTests/fast/runin/run-in-sibling-inline-block.html:1: <style>
> The test is missing a DOCTYPE (or do we *really* need it to run in quirks
mode?)

Nop.
> 
>
https://codereview.chromium.org/19290002/diff/32001/LayoutTests/fast/runin/ru...
> LayoutTests/fast/runin/run-in-sibling-inline-block.html:9: var fieldSet;
> We don't need to define these variables here as you only use them inside the
> function below.

Ok

> 
>
https://codereview.chromium.org/19290002/diff/32001/LayoutTests/fast/runin/ru...
> LayoutTests/fast/runin/run-in-sibling-inline-block.html:22: PASS if test does
> not crash.
> Instructions should be complete English sentences:
> 
> This test has PASSED if it did not crash.

Fixed

[Julien - ping for review, 2013-07-24 23:47:39]

lgtm

https://codereview.chromium.org/19290002/diff/42001/LayoutTests/fast/runin/ru...
File LayoutTests/fast/runin/run-in-sibling-inline-block.html (right):

https://codereview.chromium.org/19290002/diff/42001/LayoutTests/fast/runin/ru...
LayoutTests/fast/runin/run-in-sibling-inline-block.html:1: <!DOCTYPE html PUBLIC
"-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
Let's use HTML5 doctype: <!DOCTYPE html>

[commit-bot: I haz the power, 2013-07-25 00:02:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/19290002/49001

[commit-bot: I haz the power, 2013-07-25 02:24:55]

Change committed as 154884