Change how NGConstraintSpaceBuilder specified destination space.

Change how NGConstraintSpaceBuilder specified destination space.

The way writing_mode for NGConstraintSpaceBuilder was set was
confusing. This change makes the API more explicit about how
output writing mode is set.

NGConstraintSpaceBuilder::writing_mode_ is eliminated
NGConstraintSpaceBuilder::SetWritingMode is eliminated

instead, writing mode becomes a parameter to:
NGConstraintSpaceBuilder::ToConstraintSpace(NGWritingMode output_mode)

BUG=635619
Review-Url: https://codereview.chromium.org/2692643002
Cr-Commit-Position: refs/heads/master@{#450163}
Committed: https://chromium.googlesource.com/chromium/src/+/b22faaaadbb08c37ceca6c55c9d5557966b85b21

[atotic, 2017-02-11 00:39:29]

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

[atotic, 2017-02-11 00:43:00]

Patchset #1 (id:1) has been deleted

[atotic, 2017-02-11 00:43:35]

ptal

[Gleb Lanbin, 2017-02-13 19:33:30]

just a couple of minor comments.

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:731:
FromPlatformWritingMode(Style().getWritingMode()));
add a temp variable

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(left):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:16:
writing_mode_(parent_space->WritingMode()),
I think you still need it. Can you run tests ?

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(right):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:112:
NGWritingMode output_mode) {
out_writing_mode ?

[cbiesinger, 2017-02-13 19:41:50]

cbiesinger@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[cbiesinger, 2017-02-13 19:41:51]

I like this concept, but it feels weird to only have this for the writing mode
but not the direction -- how do others feel about that?

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h
(right):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:59: //
NGWritingMode specifies the writing mode of the generated  space.
remove one of the spaces in "generated  space"?

[atotic, 2017-02-13 21:49:52]

ptal.

All comments have a response.

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:731:
FromPlatformWritingMode(Style().getWritingMode()));
On 2017/02/13 at 19:33:29, Gleb Lanbin wrote:
> add a temp variable

no

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(left):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:16:
writing_mode_(parent_space->WritingMode()),
On 2017/02/13 at 19:33:29, Gleb Lanbin wrote:
> I think you still need it. Can you run tests ?

I can run tests. What would I need it for?

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(right):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:112:
NGWritingMode output_mode) {
On 2017/02/13 at 19:33:30, Gleb Lanbin wrote:
> out_writing_mode ?

done. I like it

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h
(right):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:59: //
NGWritingMode specifies the writing mode of the generated  space.
On 2017/02/13 at 19:41:51, cbiesinger wrote:
> remove one of the spaces in "generated  space"?

done

[atotic, 2017-02-13 21:49:54]

ptal.

All comments have a response.

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:731:
FromPlatformWritingMode(Style().getWritingMode()));
On 2017/02/13 at 19:33:29, Gleb Lanbin wrote:
> add a temp variable

no

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(left):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:16:
writing_mode_(parent_space->WritingMode()),
On 2017/02/13 at 19:33:29, Gleb Lanbin wrote:
> I think you still need it. Can you run tests ?

I can run tests. What would I need it for?

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(right):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:112:
NGWritingMode output_mode) {
On 2017/02/13 at 19:33:30, Gleb Lanbin wrote:
> out_writing_mode ?

done. I like it

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h
(right):

https://codereview.chromium.org/2692643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:59: //
NGWritingMode specifies the writing mode of the generated  space.
On 2017/02/13 at 19:41:51, cbiesinger wrote:
> remove one of the spaces in "generated  space"?

done

[Gleb Lanbin, 2017-02-13 21:59:03]

lgtm

[atotic, 2017-02-13 22:15:59]

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

[commit-bot: I haz the power, 2017-02-13 22:16:54]

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

[atotic, 2017-02-13 23:28:30]

On 2017/02/13 at 19:41:51, cbiesinger wrote:
> I like this concept, but it feels weird to only have this for the writing mode
but not the direction -- how do others feel about that?

I have no opinion, since I do not know how is NGConstraintSpace's writing mode
used.

[commit-bot: I haz the power, 2017-02-13 23:54:53]

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

[commit-bot: I haz the power, 2017-02-13 23:54:54]

Dry run: This issue passed the CQ dry run.

[atotic, 2017-02-13 23:58:07]

The CQ bit was checked by atotic@chromium.org

[commit-bot: I haz the power, 2017-02-13 23:59:10]

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, 2017-02-14 00:06:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487030287107120,
"parent_rev": "5fcf0e88e39cbba1b2aa74668224383c1c14029f", "commit_rev":
"b22faaaadbb08c37ceca6c55c9d5557966b85b21"}

[commit-bot: I haz the power, 2017-02-14 00:07:15]

Description was changed from

==========
Change how NGConstraintSpaceBuilder specified destination space.

The way writing_mode for NGConstraintSpaceBuilder was set was
confusing. This change makes the API more explicit about how
output writing mode is set.

NGConstraintSpaceBuilder::writing_mode_ is eliminated
NGConstraintSpaceBuilder::SetWritingMode is eliminated

instead, writing mode becomes a parameter to:
NGConstraintSpaceBuilder::ToConstraintSpace(NGWritingMode output_mode)

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

to

==========
Change how NGConstraintSpaceBuilder specified destination space.

The way writing_mode for NGConstraintSpaceBuilder was set was
confusing. This change makes the API more explicit about how
output writing mode is set.

NGConstraintSpaceBuilder::writing_mode_ is eliminated
NGConstraintSpaceBuilder::SetWritingMode is eliminated

instead, writing mode becomes a parameter to:
NGConstraintSpaceBuilder::ToConstraintSpace(NGWritingMode output_mode)

BUG=635619

Review-Url: https://codereview.chromium.org/2692643002
Cr-Commit-Position: refs/heads/master@{#450163}
Committed:
https://chromium.googlesource.com/chromium/src/+/b22faaaadbb08c37ceca6c55c9d5...
==========

[commit-bot: I haz the power, 2017-02-14 00:07:16]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b22faaaadbb08c37ceca6c55c9d5...