[LayoutNG] Move NGFloatingObject off Oilpan

[LayoutNG] Move NGFloatingObject off Oilpan

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

[Gleb Lanbin, 2017-03-15 20:55:52]

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

[Gleb Lanbin, 2017-03-15 21:44:33]

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

[commit-bot: I haz the power, 2017-03-15 21:45:20]

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, 2017-03-15 22:08:43]

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

[commit-bot: I haz the power, 2017-03-15 22:08:44]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ikilpatrick, 2017-03-15 22:45:40]

lgtm w/ nits.

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:225: const
RefPtr<NGFloatingObject>& floating_object,
so for all these in the anon namespace its probably ok just to pass the raw ptr?
(and potentially DCHECK) I don't think the compiler is smart enough to remove
the RefPtr from incrementing&dec for this function...

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right):

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:54:
RefPtr<NGFloatingObject> ng_floating_object,
same as comment in block_algorithm, just raw ptr with .get() when calling...?

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:243: for (const auto&
floating_object : fragment->PositionedFloats()) {
/sigh, i wish we had clearer guidelines around const auto etc. so fine with this
as it is, but should try and work out some stage for a non-trivial type like
this (const RefPtr<NGFloatingObject>&) if we need to spell this out or not.

Looks like half of blink does and doesn't.

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right):

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:176:
Vector<RefPtr<NGFloatingObject>> positioned_floats;
not sure why this exists? :)

[Gleb Lanbin, 2017-03-15 23:24:25]

thanks for the review

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:225: const
RefPtr<NGFloatingObject>& floating_object,
On 2017/03/15 22:45:40, ikilpatrick wrote:
> so for all these in the anon namespace its probably ok just to pass the raw
ptr?
> (and potentially DCHECK) I don't think the compiler is smart enough to remove
> the RefPtr from incrementing&dec for this function...

Done.

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right):

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:54:
RefPtr<NGFloatingObject> ng_floating_object,
On 2017/03/15 22:45:40, ikilpatrick wrote:
> same as comment in block_algorithm, just raw ptr with .get() when calling...?

Done.

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:243: for (const auto&
floating_object : fragment->PositionedFloats()) {
On 2017/03/15 22:45:40, ikilpatrick wrote:
> /sigh, i wish we had clearer guidelines around const auto etc. so fine with
this
> as it is, but should try and work out some stage for a non-trivial type like
> this (const RefPtr<NGFloatingObject>&) if we need to spell this out or not.
> 
> Looks like half of blink does and doesn't.

Done.

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right):

https://codereview.chromium.org/2752123002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:176:
Vector<RefPtr<NGFloatingObject>> positioned_floats;
On 2017/03/15 22:45:40, ikilpatrick wrote:
> not sure why this exists? :)

Done.

[Gleb Lanbin, 2017-03-15 23:24:30]

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

[commit-bot: I haz the power, 2017-03-15 23:25:23]

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

[ikilpatrick, 2017-03-15 23:39:18]

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

https://codereview.chromium.org/2752123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:539:
NGFloatingObject* floating_object = new NGFloatingObject(
this should have an adoptRef instead of AddUnpositionedFloat.

Typically for these we add a NGFloatingObject::create static factory function.

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

https://codereview.chromium.org/2752123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:121:
unpositioned_floats_.push_back(adoptRef(floating_object));
this should happen in caller of this function.

[Gleb Lanbin, 2017-03-16 01:14:35]

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

https://codereview.chromium.org/2752123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:539:
NGFloatingObject* floating_object = new NGFloatingObject(
On 2017/03/15 23:39:18, ikilpatrick wrote:
> this should have an adoptRef instead of AddUnpositionedFloat.
> 
> Typically for these we add a NGFloatingObject::create static factory function.

Done.

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

https://codereview.chromium.org/2752123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:121:
unpositioned_floats_.push_back(adoptRef(floating_object));
On 2017/03/15 23:39:18, ikilpatrick wrote:
> this should happen in caller of this function. 

Done.

[Gleb Lanbin, 2017-03-16 01:14:39]

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

[commit-bot: I haz the power, 2017-03-16 01:15:18]

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, 2017-03-16 02:34:36]

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

[commit-bot: I haz the power, 2017-03-16 02:34:37]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2017-03-16 02:36:24]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2017-03-16 02:36:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2752123002/#ps40001
(title: "more comments")

[commit-bot: I haz the power, 2017-03-16 02:36:48]

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-03-16 02:41:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489631784005230,
"parent_rev": "206866029e5e2fddc1cff642fbfc5318007a6574", "commit_rev":
"5d5c1c5c6d96ba9431897acd2a0eea2a2eabcac2"}

[commit-bot: I haz the power, 2017-03-16 02:42:36]

Description was changed from

==========
[LayoutNG] Move NGFloatingObject off Oilpan

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

to

==========
[LayoutNG] Move NGFloatingObject off Oilpan

BUG=635619

Review-Url: https://codereview.chromium.org/2752123002
Cr-Commit-Position: refs/heads/master@{#457326}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d5c1c5c6d96ba9431897acd2a0e...
==========

[commit-bot: I haz the power, 2017-03-16 02:42:37]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5d5c1c5c6d96ba9431897acd2a0e...