WTF::Vector: Enable static_asserts in a Vector constructor.

WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3b095c8af9c8f2649

BUG=690816

[Yuta Kitamura, 2017-02-23 08:37:15]

Description was changed from

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
==========

to

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
# This violates a DEPS rule.
COMMIT=false
==========

[Yuta Kitamura, 2017-02-23 08:37:15]

yutak@chromium.org changed reviewers:
+ haraken@chromium.org, kouhei@chromium.org

[Yuta Kitamura, 2017-02-23 08:37:35]

Description was changed from

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
# This violates a DEPS rule.
COMMIT=false
==========

to

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
# Patch Set 1 violates a DEPS rule.
COMMIT=false
==========

[Yuta Kitamura, 2017-02-23 08:44:02]

*** This is a request for comments, not a review request ***

haraken, kouhei: Patch Set 1 fixes a static_assert failure,
but it violates a DEPS rule for core/layout/line/.

The line 163 of RootInlineBox.h is the source of the error:

> 163       m_floats = WTF::wrapUnique(new Vector<LayoutBox*>(1, floatingBox));

static_assert in Vector wants LayoutBox to be a complete type
here, but it's not allowed to include LayoutBox.h here:

> You added one or more #includes that violate checkdeps rules.
>   third_party/WebKit/Source/core/layout/line/RootInlineBox.h
>       Illegal include: "core/layout/LayoutBox.h"
>     Because of "-core/layout" from
third_party/WebKit/Source/core/layout/line's include_rules.

Do you have any idea about how to resolve this?

[haraken, 2017-02-23 09:00:23]

On 2017/02/23 08:44:02, Yuta Kitamura wrote:
> *** This is a request for comments, not a review request ***
> 
> haraken, kouhei: Patch Set 1 fixes a static_assert failure,
> but it violates a DEPS rule for core/layout/line/.
> 
> The line 163 of RootInlineBox.h is the source of the error:
> 
> > 163       m_floats = WTF::wrapUnique(new Vector<LayoutBox*>(1,
floatingBox));
> 
> static_assert in Vector wants LayoutBox to be a complete type
> here, but it's not allowed to include LayoutBox.h here:

Just to confirm: Moving the method definition to a cpp file doesn't resolve the
problem because then the cpp file needs to include LayoutBox.h, right?

> > You added one or more #includes that violate checkdeps rules.
> >   third_party/WebKit/Source/core/layout/line/RootInlineBox.h
> >       Illegal include: "core/layout/LayoutBox.h"
> >     Because of "-core/layout" from
> third_party/WebKit/Source/core/layout/line's include_rules.
> 
> Do you have any idea about how to resolve this?

Maybe should we ask eae@? If we really want to forbid the dependency from
layout/line/ to layout/, the fact that layout/line/ is using a LayoutBox*
pointer is already violating the dependency rule...

[Yuta Kitamura, 2017-02-23 09:17:27]

On 2017/02/23 09:00:23, haraken wrote:
> Just to confirm: Moving the method definition to a cpp file doesn't resolve
the
> problem because then the cpp file needs to include LayoutBox.h, right?

Good question. I de-inlined the function (as in Patch Set 2), and
it compiled successfully for some reason. I'd guess the definition
of LayoutBox is pulled from some of the #includes in .cpp file...

[Yuta Kitamura, 2017-02-23 09:18:43]

yutak@chromium.org changed reviewers:
+ eae@chromium.org

[Yuta Kitamura, 2017-02-23 09:18:44]

+eae as haraken suggested

eae: Do you have an opinion about how to resolve those?
(See the comments for more information)

[Yuta Kitamura, 2017-02-23 09:19:05]

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

[commit-bot: I haz the power, 2017-02-23 09:19:15]

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-02-23 11:19:29]

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

[commit-bot: I haz the power, 2017-02-23 11:19:30]

Dry run: This issue passed the CQ dry run.

[eae, 2017-02-23 15:57:12]

I think it's fine to violate the layout/line constraint here, having the static
casts are more important than the maintaining the illusion of isolation for that
directory.

[eae, 2017-02-24 04:07:02]

LGTM

[haraken, 2017-02-24 04:08:20]

LGTM

[Yuta Kitamura, 2017-02-24 06:17:03]

On 2017/02/23 15:57:12, eae wrote:
> I think it's fine to violate the layout/line constraint here, having the
static
> casts are more important than the maintaining the illusion of isolation for
that
> directory.

Thanks, I assume you are okay with ignoring a DEPS error
here and landing PS1. Please let me know if that's not the
case.

[Yuta Kitamura, 2017-02-24 06:22:27]

Description was changed from

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
# Patch Set 1 violates a DEPS rule.
COMMIT=false
==========

to

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
==========

[Yuta Kitamura, 2017-02-24 06:22:32]

The CQ bit was checked by yutak@chromium.org

[Yuta Kitamura, 2017-02-24 06:22:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2710183002/#ps40001
(title: "Revert PS2.")

[commit-bot: I haz the power, 2017-02-24 06:22:51]

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-24 06:29:44]

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

[commit-bot: I haz the power, 2017-02-24 06:29:45]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yuta Kitamura, 2017-02-24 06:43:14]

Description was changed from

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
==========

to

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
# See comments for the reason for skipping presubmit.
NOPRESUBMIT=true
==========

[Yuta Kitamura, 2017-02-24 06:43:19]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2017-02-24 06:43:34]

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-24 07:06:08]

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

[commit-bot: I haz the power, 2017-02-24 07:06:09]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuta Kitamura, 2017-02-24 07:09:58]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2017-02-24 07:10:13]

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-24 07:41:01]

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

[commit-bot: I haz the power, 2017-02-24 07:41:02]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Yuta Kitamura, 2017-02-24 09:23:30]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2017-02-24 09:23:46]

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-24 11:13:48]

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

[commit-bot: I haz the power, 2017-02-24 11:13:50]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuta Kitamura, 2017-02-24 12:44:42]

Description was changed from

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
# See comments for the reason for skipping presubmit.
NOPRESUBMIT=true
==========

to

==========
WTF::Vector: Enable static_asserts in a Vector constructor.

Those static_asserts did not work because Vector<LayoutBox*> was created in
a context where LayoutBox was an incomplete type. Adding an #include fixes
this.

This is a follow-up of the following commit:
https://chromium.googlesource.com/chromium/src.git/+/e6cfc1a979decf08475dcef3...

BUG=690816
==========

[Yuta Kitamura, 2017-02-24 12:44:53]

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

[commit-bot: I haz the power, 2017-02-24 12:45:14]

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

[Yuta Kitamura, 2017-02-24 12:46:35]

Seems like a change with DEPS violation can't be accepted
in bots, so I added a "temporarily allowed" include rule in
DEPS (next to existing ones).

eae: Can you look at this again? Thanks!

[commit-bot: I haz the power, 2017-02-24 14:14:13]

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

[commit-bot: I haz the power, 2017-02-24 14:14:14]

Dry run: This issue passed the CQ dry run.