Resolve ApplyAutoMargins's TODO in NGBlockLayoutAlgorithm.

Resolve ApplyAutoMargins's TODO in NGBlockLayoutAlgorithm.

Combine 2 methods related to child margins calculation into one.

BUG=635619
Committed: https://crrev.com/96a02ed3c20fcc3508b08bddf613abd5e57a225d
Cr-Commit-Position: refs/heads/master@{#427709}

[Gleb Lanbin, 2016-10-25 20:05:46]

glebl@chromium.org changed reviewers:
+ cbiesinger@chromium.org, ikilpatrick@chromium.org

[cbiesinger, 2016-10-25 20:09:57]

I don't think this is correct because margin:auto should not apply to floats,
right?

I'm not sure anymore that ApplyAutoMargins should move anywhere... it may be
fine as it is

[Gleb Lanbin, 2016-10-25 21:11:59]

On 2016/10/25 20:09:57, cbiesinger wrote:
> I don't think this is correct because margin:auto should not apply to floats,
> right?

yes, fixed.

> 
> I'm not sure anymore that ApplyAutoMargins should move anywhere... it may be
> fine as it is

here are the reasons why I think it's better in this way
1) now we calculate child margins in one place. IMO, the code is a bit better
organized in this way
2) IMO, now it's a bit more clear which functions modify the passed by reference
child_margins.
3) you're right that margins: auto has no affect on floats, as well as on
inline. So it's better to leave the decision where margin:auto should be applied
either to ComputeChildMargins or ApplyAutoMargins itself.

[Gleb Lanbin, 2016-10-25 21:54:13]

On 2016/10/25 21:11:59, glebl wrote:
> On 2016/10/25 20:09:57, cbiesinger wrote:
> > I don't think this is correct because margin:auto should not apply to
floats,
> > right?
> 
> yes, fixed.
> 
> > 
> > I'm not sure anymore that ApplyAutoMargins should move anywhere... it may be
> > fine as it is
> 
> here are the reasons why I think it's better in this way
> 1) now we calculate child margins in one place. IMO, the code is a bit better
> organized in this way
> 2) IMO, now it's a bit more clear which functions modify the passed by
reference
> child_margins.
> 3) you're right that margins: auto has no affect on floats, as well as on
> inline. So it's better to leave the decision where margin:auto should be
applied
> either to ComputeChildMargins or ApplyAutoMargins itself.

discussed offline, although margin:auto should be ignored by floats, inline,
absolute and fixed positioned elements, we decided that it's better to leave to
the caller to decide where ApplyAutoMargins should be called. We can always
revise this decision later when LayoutNG will support more layout cases.

[Gleb Lanbin, 2016-10-25 22:02:16]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 22:02:57]

Dry run: 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-10-25 23:32:52]

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

[commit-bot: I haz the power, 2016-10-25 23:32:53]

Dry run: This issue passed the CQ dry run.

[cbiesinger1, 2016-10-26 00:09:23]

Lgtm

[cbiesinger1, 2016-10-26 00:09:35]

The CQ bit was checked by cbiesinger@google.com

[commit-bot: I haz the power, 2016-10-26 00:10:03]

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-10-26 00:10:04]

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

[commit-bot: I haz the power, 2016-10-26 00:10:05]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[ikilpatrick, 2016-10-26 16:16:50]

lgtm

[ikilpatrick, 2016-10-26 16:16:59]

The CQ bit was checked by ikilpatrick@chromium.org

[commit-bot: I haz the power, 2016-10-26 16:17:22]

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-10-26 16:24:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-26 17:12:26]

Description was changed from

==========
Resolve ApplyAutoMargins's TODO in NGBlockLayoutAlgorithm.

Combine 2 methods related to child margins calculation into one.

BUG=635619
==========

to

==========
Resolve ApplyAutoMargins's TODO in NGBlockLayoutAlgorithm.

Combine 2 methods related to child margins calculation into one.

BUG=635619

Committed: https://crrev.com/96a02ed3c20fcc3508b08bddf613abd5e57a225d
Cr-Commit-Position: refs/heads/master@{#427709}
==========

[commit-bot: I haz the power, 2016-10-26 17:12:27]

Patchset 4 (id:??) landed as
https://crrev.com/96a02ed3c20fcc3508b08bddf613abd5e57a225d
Cr-Commit-Position: refs/heads/master@{#427709}