Don't create layout objects for children of display-none iframes.

Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is "DisplayNone" if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2564633002
Cr-Commit-Position: refs/heads/master@{#450748}
Committed: https://chromium.googlesource.com/chromium/src/+/962434d34e2c9bcd17488f499515920c298b5395

[erikchen, 2016-12-08 21:43:13]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo.

BUG=
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-08 22:09:11]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo.
Most of this CL is plumbing. The relevant files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Open questions:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary? Need to make sure that this flag is plumbed recursively,
  and that it's initial value is set appropriately when creating the frame.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-08 22:09:38]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo.
Most of this CL is plumbing. The relevant files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Open questions:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary? Need to make sure that this flag is plumbed recursively,
  and that it's initial value is set appropriately when creating the frame.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo.
Most of this CL is plumbing. The relevant files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Open questions:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-08 22:09:58]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo.
Most of this CL is plumbing. The relevant files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Open questions:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Open questions:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-08 22:10:38]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Open questions:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Open questions:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.
  * There's currently two logic paths in LayoutTreeBuilder for local/remote
  frames. I suspect that this can/should be unified into a single path.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-08 22:10:46]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Open questions:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.
  * There's currently two logic paths in LayoutTreeBuilder for local/remote
  frames. I suspect that this can/should be unified into a single path.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.
  * There's currently two logic paths in LayoutTreeBuilder for local/remote
  frames. I suspect that this can/should be unified into a single path.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-08 22:11:27]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.
  * There's currently two logic paths in LayoutTreeBuilder for local/remote
  frames. I suspect that this can/should be unified into a single path.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.
  * There's currently two logic paths in LayoutTreeBuilder for local/remote
  frames. I suspect that this can/should be unified into a single path.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-08 22:13:57]

erikchen@chromium.org changed reviewers:
+ esprehn@chromium.org

[erikchen, 2016-12-08 22:13:57]

esprehn: Can you take a look, or forward to a more appropriate reviewer? I want
to make sure that I'm on the right path before sinking too much time into this.

Asides:
* Never worked in Source/core before, so please excuse me if my entire approach
is wrong.
* Please see the CL description for more details.

[erikchen, 2016-12-08 22:14:08]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that it's initial value is set appropriately when creating
  the frame.
  * There's currently two logic paths in LayoutTreeBuilder for local/remote
  frames. I suspect that this can/should be unified into a single path.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that its initial value is set appropriately when creating
  the frame.
  * There's currently two logic paths in LayoutTreeBuilder for local/remote
  frames. I suspect that this can/should be unified into a single path.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dcheng, 2016-12-08 22:21:17]

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

[dcheng, 2016-12-08 22:21:17]

I think we should plumb this as part of FrameOwnerProperties: this lets us keep
the display:none treatment consistent by exposing it off FrameOwner. It also
keeps this behavior specific to child frames (as the top-level frame itself
cannot be display:none).

[dcheng, 2016-12-08 22:25:29]

On 2016/12/08 22:21:17, dcheng wrote:
> I think we should plumb this as part of FrameOwnerProperties: this lets us
keep
> the display:none treatment consistent by exposing it off FrameOwner. It also
> keeps this behavior specific to child frames (as the top-level frame itself
> cannot be display:none).

Ah, found Elliot's CL for this: https://codereview.chromium.org/2513153002/

LayoutView::canHaveChildren() currently consults HTMLFrameOwnerElement. To
abstract this for OOPIF, we'd have a FrameOwner::isDisplayNone() virtual. For
HTMLFrameOwnerElement, it'd just return whether or not it has a layout object.
For RemoteFrameOwner, it'd return the value replicated.

[esprehn, 2016-12-08 23:10:21]

https://codereview.chromium.org/2564633002/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2564633002/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:1649: void
RenderWidget::OnUpdateDisplayNone(bool d) {
Lets use the OOPIF replication plumbing and avoid adding all of this.

https://codereview.chromium.org/2564633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp (right):

https://codereview.chromium.org/2564633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp:95: if
(m_layoutObjectParent->frame()->getDisplayNone()) {
This should be handled inside LayoutView::canHaveChildren() and with a virtual
to abstract the local vs remote frame.

https://codereview.chromium.org/2564633002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2564633002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:221:
m_localRoot->frame()->view()->scheduleRelayout();
setNeedsLayout already schedules a layout.

https://codereview.chromium.org/2564633002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:222:
m_localRoot->frame()->view()->updateLifecycleToLayoutClean();
We should not be forcing a sync update here, you just want to mark things as
dirty and schedule the frame.

[esprehn, 2016-12-08 23:10:32]

On 2016/12/08 at 22:25:29, dcheng wrote:
> On 2016/12/08 22:21:17, dcheng wrote:
> > I think we should plumb this as part of FrameOwnerProperties: this lets us
keep
> > the display:none treatment consistent by exposing it off FrameOwner. It also
> > keeps this behavior specific to child frames (as the top-level frame itself
> > cannot be display:none).
> 
> Ah, found Elliot's CL for this: https://codereview.chromium.org/2513153002/
> 
> LayoutView::canHaveChildren() currently consults HTMLFrameOwnerElement. To
abstract this for OOPIF, we'd have a FrameOwner::isDisplayNone() virtual. For
HTMLFrameOwnerElement, it'd just return whether or not it has a layout object.
For RemoteFrameOwner, it'd return the value replicated.

Yeah that should be much simpler.

[erikchen, 2016-12-09 19:19:46]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

This CL is not intended to be committed. It purely exists as a concept demo
[which isn't even fully functional]. Most of this CL is plumbing. The relevant
files are:
  * third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
  * third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp
  * third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  WebFrameWidgetImpl.cpp? scheduleRelayout() doesn't seem to have the
  appropriate effect.
  * This CL plumbs a new flag [display-none] through the OOPIF machinery. Is a
  new flag necessary [I think so]? Need to make sure that this flag is plumbed
  recursively, and that its initial value is set appropriately when creating
  the frame.
  * There's currently two logic paths in LayoutTreeBuilder for local/remote
  frames. I suspect that this can/should be unified into a single path.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * Does render_frame_host_manager.cc need to hold on to FrameOwnerProperties so
  that it can be sent to a newly attached render frame host?
  * What is the appropriate way to trigger a re-build of the layout tree in
  Document.cpp? setNeedsLayout() doesn't seem to have the appropriate effect.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-09 19:26:52]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * Does render_frame_host_manager.cc need to hold on to FrameOwnerProperties so
  that it can be sent to a newly attached render frame host?
  * What is the appropriate way to trigger a re-build of the layout tree in
  Document.cpp? setNeedsLayout() doesn't seem to have the appropriate effect.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  Document.cpp? setNeedsLayout() doesn't seem to have the appropriate effect.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-09 19:34:56]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  Document.cpp? setNeedsLayout() doesn't seem to have the appropriate effect.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  Document.cpp? Neither setNeedsLayout() or setNeedsStyleRecalc() seems to have
  the appropriate effect.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-09 20:27:56]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  Document.cpp? Neither setNeedsLayout() or setNeedsStyleRecalc() seems to have
  the appropriate effect.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

Most of this CL is plumbing. Interesting files:
  * third_party/WebKit/Source/core/dom/Document.cpp
  ...

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  Document.cpp? Neither setNeedsLayout() or setNeedsStyleRecalc() seems to have
  the appropriate effect.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-10 00:24:24]

Patchset #5 (id:70001) has been deleted

[erikchen, 2016-12-10 00:26:21]

Patchset #5 (id:90001) has been deleted

[erikchen, 2016-12-10 00:39:17]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

Most of this CL is plumbing. Interesting files:
  * third_party/WebKit/Source/core/dom/Document.cpp
  ...

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

Discussion points:
  * What is the appropriate way to trigger a re-build of the layout tree in
  Document.cpp? Neither setNeedsLayout() or setNeedsStyleRecalc() seems to have
  the appropriate effect.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-10 00:42:10]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change, or adding an internal-only style-property to reflect isDisplayNone.

Open Questions:
  * Why did esprehn modify HTMLFrameElementBase::attachLayoutTree?

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-10 01:18:54]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change, or adding an internal-only style-property to reflect isDisplayNone.

Open Questions:
  * Why did esprehn modify HTMLFrameElementBase::attachLayoutTree?

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test CL for "Don't emit layout objects for display-none iframes."

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change, or adding an internal-only style-property to reflect isDisplayNone.

Open Questions:
  * It's not clear to me why esprehn modified
    HTMLFrameElementBase::attachLayoutTree. Removing the call to
    lazyReattachIfAttached() seems to have no effect.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-10 01:19:06]

Description was changed from

==========
Test CL for "Don't emit layout objects for display-none iframes."

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change, or adding an internal-only style-property to reflect isDisplayNone.

Open Questions:
  * It's not clear to me why esprehn modified
    HTMLFrameElementBase::attachLayoutTree. Removing the call to
    lazyReattachIfAttached() seems to have no effect.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change, or adding an internal-only style-property to reflect isDisplayNone.

Open Questions:
  * It's not clear to me why esprehn modified
    HTMLFrameElementBase::attachLayoutTree. Removing the call to
    lazyReattachIfAttached() seems to have no effect.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-10 01:19:18]

Description was changed from

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change, or adding an internal-only style-property to reflect isDisplayNone.

Open Questions:
  * It's not clear to me why esprehn modified
    HTMLFrameElementBase::attachLayoutTree. Removing the call to
    lazyReattachIfAttached() seems to have no effect.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change, or adding an internal-only style-property to reflect isDisplayNone.

Open Questions:
  * It's not clear to me why esprehn modified
    HTMLFrameElementBase::attachLayoutTree. Removing the call to
    lazyReattachIfAttached() seems to have no effect.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-10 01:20:17]

dcheng: PTAL

Please see the CL description for a detailed update.

[esprehn, 2016-12-10 02:20:55]

https://codereview.chromium.org/2564633002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2564633002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4473:
documentElement()->setForceReattach();
You don't need this.

https://codereview.chromium.org/2564633002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4474:
setNeedsStyleRecalc(NeedsReattachStyleChange,
It's not valid to set a reattach on the document itself. You want to just do
lazyReattach on the documentElement.

https://codereview.chromium.org/2564633002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2564633002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:1949: if
(forceReattachDuringStyleRecalc())
Remove this, we don't want an extra branch for every element in the entire
document. Just doing lazyReattach on the documentElement is enough.

https://codereview.chromium.org/2564633002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/2564633002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.h:168: m_forceReattach = false;
You don't need any of this. lazyReattach does the same thing.

[erikchen, 2016-12-12 22:55:06]

Patchset #7 (id:150001) has been deleted

[erikchen, 2016-12-12 22:56:28]

Description was changed from

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.
  * Hack in Element.cpp to force the HTML element to reattach on isDisplayNone
    change. There's been no style change, so I don't see any other way to do
    this. I suspect a proper solution might involve actually propagating a style
    change, or adding an internal-only style-property to reflect isDisplayNone.

Open Questions:
  * It's not clear to me why esprehn modified
    HTMLFrameElementBase::attachLayoutTree. Removing the call to
    lazyReattachIfAttached() seems to have no effect.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-12 23:02:06]

Thanks esprehn, PTAL.

If you're worried about the cost of an extra branch, then you'll probably be
displeased by the fact that this CL makes setLayoutObject virtual. That seems
like the simplest solution, since I want to send a frameOwnerPropertiesChanged
message if and only if the FrameOwnerElement gains or loses a layout object. I
could change this so that FrameOwnerElement keeps this as additional state
[specifically, m_hadLayoutObject] which is checked during calls to
attachLayoutTree and detachLayoutTree. When it changes [for remote frames], I
can send a frameOwnerPropertiesChanged message. WDYT?

[esprehn, 2016-12-12 23:18:35]

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.h (right):

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.h:569: virtual void
setLayoutObject(LayoutObject* layoutObject) {
Yeah we don't want this virtual. :)

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:204:
documentElement->lazyReattachIfAttached();
I think we can actually remove this logic and just rely on setWidget handling
it.

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:241: void
HTMLFrameOwnerElement::setLayoutObject(LayoutObject* layoutObject) {
You want to put code in HTMLFrameOwnerElement::setWidget instead. When the
widget is null the frame is display none'ed.

[erikchen, 2016-12-13 01:12:55]

Description was changed from

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * Override HTMLFrameOwnerElement::setLayoutObject to trigger a
    frameOwnerPropertiesChanged message.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
  FrameOwnerProperties.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-13 01:12:59]

Description was changed from

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
  FrameOwnerProperties.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-13 01:15:45]

thanks esprehn, this is already looking a lot better, even from my green eyes.

I'll add some tests, and then send an intent to implement?

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.h (right):

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.h:569: virtual void
setLayoutObject(LayoutObject* layoutObject) {
On 2016/12/12 23:18:35, esprehn wrote:
> Yeah we don't want this virtual. :)

Done.

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:204:
documentElement->lazyReattachIfAttached();
On 2016/12/12 23:18:35, esprehn wrote:
> I think we can actually remove this logic and just rely on setWidget handling
> it.

agreed. done.

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2564633002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:241: void
HTMLFrameOwnerElement::setLayoutObject(LayoutObject* layoutObject) {
On 2016/12/12 23:18:35, esprehn wrote:
> You want to put code in HTMLFrameOwnerElement::setWidget instead. When the
> widget is null the frame is display none'ed.

Done.

[esprehn, 2016-12-13 01:26:18]

What handles local frames now? Maybe I'm just not following the code properly,
but I don't see how we end up inside willChangeFrameOwnerProperties for a
LocalFrame inside a LocalFrame? Does it require a round trip through IPC system
now? Maybe that's okay.

This patch looks great though, I think we'd usually add a RuntimeEnabledFeature
flag for it just in case we need to turn it off. You should send the Intent to
Implement & Ship now while you're writing the tests so it can be handled in
parallel. :)

[erikchen, 2016-12-13 03:29:47]

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

[commit-bot: I haz the power, 2016-12-13 03:30:14]

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

[esprehn, 2016-12-13 04:45:53]

On 2016/12/13 at 01:26:18, esprehn wrote:
> What handles local frames now? Maybe I'm just not following the code properly,
but I don't see how we end up inside willChangeFrameOwnerProperties for a
LocalFrame inside a LocalFrame? Does it require a round trip through IPC system
now? Maybe that's okay.
> 

Actually yeah this isn't okay because we need to handle changes observable to JS
sync. If you make an iframe display none and then reach into it and call
getBoundingClientRect it needs to be handled synchronously making everything
display none inside as well.

We probably need that frame properties code to understand how to deal with a
local change underneath the setWidget call.

[commit-bot: I haz the power, 2016-12-13 05:18:55]

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

[commit-bot: I haz the power, 2016-12-13 05:18:56]

Dry run: 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_...)

[erikchen, 2016-12-13 22:05:56]

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

[commit-bot: I haz the power, 2016-12-13 22:06:52]

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

[erikchen, 2016-12-15 20:53:26]

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

[commit-bot: I haz the power, 2016-12-15 20:54:27]

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

[erikchen, 2016-12-15 20:54:52]

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

[commit-bot: I haz the power, 2016-12-15 20:55:26]

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-12-15 20:57:09]

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

[commit-bot: I haz the power, 2016-12-15 20:57:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[erikchen, 2016-12-15 21:25:50]

esprehn: PTAL. 

The LocalFrame in LocalFrame case was an oversight on my part - I've fixed it.
I've added tests to WebFrameTest as well as two new ./paint/invalidation
LayoutTests. Some of the LayoutTest failures just need a rebaseline. I'm
currently still investigating the shadow dom failures. Anything else you want to
see on the testing front?

[dcheng, 2016-12-15 21:35:04]

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4420: if (isDisplayNone !=
owner->isDisplayNone() && documentElement()) {
Nit: this is already null-checked at the beginning of the functoin.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:253: Document* doc
= contentDocument();
I feel like setWidget() is a less typical choice to hook for determining
visibility.

An alternative is to override attachLayoutTree / detachLayoutTree, which is
directly related to whether or not this thing has a layout object.

esprehn, WDYT?

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:254: if (doc &&
doc->frame()) {
I don't think it's possible to have a contentDocument() with no frame (since
that would imply it's not attached)

[esprehn, 2016-12-15 21:36:48]

Very excited, this is looking great.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10786: for (Node* child =
doc->documentElement()->firstChild(); child;
There's no need to check this, if the documentElement has no layoutObject, then
it's children can't have them either.

!documentElement()->layoutObject() is enough.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10795: TEST_F(WebFrameTest,
DisplayNoneIFrameHasNoLayoutObjects) {
If you use a SimTest instead this whole this can be much simpler and you don't
need any of the external files.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10821:
frame->executeScript(
No need for JS, you can just do
element->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10830: TEST_F(WebFrameTest,
NormalIFrameHasLayoutObjects) {
SimTests will make this simpler:
ex.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Docu...

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/data/single_invisible_iframe.html
(right):

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/data/single_invisible_iframe.html:1: <html>
Missing <!DOCTYPE html>

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/data/single_invisible_iframe.html:5: </html>
You only need:

<!DOCTYPE html>
<iframe style="display:none;" src="visible_iframe.html"></iframe>

the html, head and body are all optional.

[esprehn, 2016-12-15 21:45:56]

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:253: Document* doc
= contentDocument();
On 2016/12/15 at 21:35:04, dcheng wrote:
> I feel like setWidget() is a less typical choice to hook for determining
visibility.
> 
> An alternative is to override attachLayoutTree / detachLayoutTree, which is
directly related to whether or not this thing has a layout object.
> 
> esprehn, WDYT?

Since we already have all of this machinery, and setWidget is what powers the
show() and hide() notifications on the FrameView (for display: none and
visibility hidden) this seemed like a reasonable place for it.

At first I didn't suggest attach/detach because of complexity about reattach,
but this is actually not bad because if an iframe is removed from the DOM we
destroy the layout tree when we do Document::shutdown so we don't need to handle
detach at all, just attach (all style changes will be a pair of detach/attach,
we only get unpaired detach when lazyReattaching, where style will run attach
later, or when removing nodes from the dom).

This pseudo code should work:

void attachLayoutObject(context) {
  bool hasLayoutObject = static_cast<bool>(layoutObject());
  super(context);
  if (hasLayoutObject != static_cast<bool>(layoutObject()))
    notifyDisplayNoneChanged();
}

[erikchen, 2016-12-15 22:24:23]

Description was changed from

==========
Don't emit layout objects for display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't emit layout objects for display-none iframes.

TODO(erikchen): note to self - launch behind experiment.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-16 00:50:16]

On 2016/12/15 21:45:56, esprehn wrote:
>
https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):
> 
>
https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:253: Document*
doc
> = contentDocument();
> On 2016/12/15 at 21:35:04, dcheng wrote:
> > I feel like setWidget() is a less typical choice to hook for determining
> visibility.
> > 
> > An alternative is to override attachLayoutTree / detachLayoutTree, which is
> directly related to whether or not this thing has a layout object.
> > 
> > esprehn, WDYT?
> 
> Since we already have all of this machinery, and setWidget is what powers the
> show() and hide() notifications on the FrameView (for display: none and
> visibility hidden) this seemed like a reasonable place for it.
> 
> At first I didn't suggest attach/detach because of complexity about reattach,
> but this is actually not bad because if an iframe is removed from the DOM we
> destroy the layout tree when we do Document::shutdown so we don't need to
handle
> detach at all, just attach (all style changes will be a pair of detach/attach,
> we only get unpaired detach when lazyReattaching, where style will run attach
> later, or when removing nodes from the dom).
> 
> This pseudo code should work:
> 
> void attachLayoutObject(context) {
>   bool hasLayoutObject = static_cast<bool>(layoutObject());
>   super(context);
>   if (hasLayoutObject != static_cast<bool>(layoutObject()))
>     notifyDisplayNoneChanged();
> }

This pseudocode doesn't work when setting the display of a frame from !none ->
none, since the layoutObject() has already been detached from the previous call
to detachLayoutObject(). My suggestion in
https://codereview.chromium.org/2564633002/#msg31 was to add some state
"m_hadLayoutObject" that gets tracked during
attachLayoutObject/detachLayoutObject, but this doesn't seem very clean, since
it relies on implicit assumptions about the ordering of calls to
attach/detachLayoutObject. It would be easy to add more logic into layout to
support this more cleanly, but you seem quite concerned about the performance of
node/containernode/layoutobject. I think that tracking this via m_widget and
setWidget() is simpler and cleaner, but would be happen to be proven wrong [and
to learn more about Blink in the process!]

[dcheng, 2016-12-16 01:09:50]

On 2016/12/16 00:50:16, erikchen wrote:
> On 2016/12/15 21:45:56, esprehn wrote:
> >
>
https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:253: Document*
> doc
> > = contentDocument();
> > On 2016/12/15 at 21:35:04, dcheng wrote:
> > > I feel like setWidget() is a less typical choice to hook for determining
> > visibility.
> > > 
> > > An alternative is to override attachLayoutTree / detachLayoutTree, which
is
> > directly related to whether or not this thing has a layout object.
> > > 
> > > esprehn, WDYT?
> > 
> > Since we already have all of this machinery, and setWidget is what powers
the
> > show() and hide() notifications on the FrameView (for display: none and
> > visibility hidden) this seemed like a reasonable place for it.
> > 
> > At first I didn't suggest attach/detach because of complexity about
reattach,
> > but this is actually not bad because if an iframe is removed from the DOM we
> > destroy the layout tree when we do Document::shutdown so we don't need to
> handle
> > detach at all, just attach (all style changes will be a pair of
detach/attach,
> > we only get unpaired detach when lazyReattaching, where style will run
attach
> > later, or when removing nodes from the dom).
> > 
> > This pseudo code should work:
> > 
> > void attachLayoutObject(context) {
> >   bool hasLayoutObject = static_cast<bool>(layoutObject());
> >   super(context);
> >   if (hasLayoutObject != static_cast<bool>(layoutObject()))
> >     notifyDisplayNoneChanged();
> > }
> 
> This pseudocode doesn't work when setting the display of a frame from !none ->
> none, since the layoutObject() has already been detached from the previous
call
> to detachLayoutObject(). My suggestion in
> https://codereview.chromium.org/2564633002/#msg31 was to add some state
> "m_hadLayoutObject" that gets tracked during
> attachLayoutObject/detachLayoutObject, but this doesn't seem very clean, since
> it relies on implicit assumptions about the ordering of calls to
> attach/detachLayoutObject. It would be easy to add more logic into layout to
> support this more cleanly, but you seem quite concerned about the performance
of
> node/containernode/layoutobject. I think that tracking this via m_widget and
> setWidget() is simpler and cleaner, but would be happen to be proven wrong
[and
> to learn more about Blink in the process!]

I'm just a little worried about tying this to widget: HTMLPlugInElement uses
this as a base, and my recollection is plugins can be instantiated with a widget
even if they're display:none.

[esprehn, 2016-12-16 01:50:16]

On 2016/12/16 at 00:50:16, erikchen wrote:
> On 2016/12/15 21:45:56, esprehn wrote:
> > ...
> > 
> > This pseudo code should work:
> > 
> > void attachLayoutObject(context) {
> >   bool hasLayoutObject = static_cast<bool>(layoutObject());
> >   super(context);
> >   if (hasLayoutObject != static_cast<bool>(layoutObject()))
> >     notifyDisplayNoneChanged();
> > }
> 
> This pseudocode doesn't work when setting the display of a frame from !none ->
none, since the layoutObject() has already been detached from the previous call
to detachLayoutObject(). My suggestion in
https://codereview.chromium.org/2564633002/#msg31 was to add some state
"m_hadLayoutObject" that gets tracked during
attachLayoutObject/detachLayoutObject, but this doesn't seem very clean, since
it relies on implicit assumptions about the ordering of calls to
attach/detachLayoutObject.

fwiw it's not implicit, attach will only ever happen after calling detach. We
might call detach and then attach at some later time, but it'll always happen in
that order. You're right though, now I remember why I chose setWidget() to hook,
it's because we lose the "hadLayoutObject" state when doing a reattach, so you
end up adding more state which is a replication of the widget state.

> It would be easy to add more logic into layout to support this more cleanly,
but you seem quite concerned about the performance of
node/containernode/layoutobject. I think that tracking this via m_widget and
setWidget() is simpler and cleaner, but would be happen to be proven wrong [and
to learn more about Blink in the process!]

Typical pages have thousands (5-20k) of elements and layout objects, we need to
keep the core infrastructure of what we run for every element very fast. We also
iterate the entire document calling certain methods many times, layoutObject()
is super hot.

[esprehn, 2016-12-16 01:51:47]

On 2016/12/16 at 01:09:50, dcheng wrote:
> On 2016/12/16 00:50:16, erikchen wrote:
> > On 2016/12/15 21:45:56, esprehn wrote:
> > > [...]
> > 
> > This pseudocode doesn't work when setting the display of a frame from !none
->
> > none, since the layoutObject() has already been detached from the previous
call
> > to detachLayoutObject(). My suggestion in
> > https://codereview.chromium.org/2564633002/#msg31 was to add some state
> > "m_hadLayoutObject" that gets tracked during
> > attachLayoutObject/detachLayoutObject, but this doesn't seem very clean,
since
> > it relies on implicit assumptions about the ordering of calls to
> > attach/detachLayoutObject. It would be easy to add more logic into layout to
> > support this more cleanly, but you seem quite concerned about the
performance of
> > node/containernode/layoutobject. I think that tracking this via m_widget and
> > setWidget() is simpler and cleaner, but would be happen to be proven wrong
[and
> > to learn more about Blink in the process!]
> 
> I'm just a little worried about tying this to widget: HTMLPlugInElement uses
this as a base, and my recollection is plugins can be instantiated with a widget
even if they're display:none.

We don't load display: none plugins, except maybe for the guest view thing on
cros? In general though I'm not very concerned about this, plugins don't have a
document inside the frame.

I think we should stick with setWidget for now, if it explodes we'll catch it
and come up with a more complex solution.

[dominicc (has gone to gerrit), 2016-12-16 02:02:10]

Nit with the title: Are you talking about emitting the layout objects, or
omitting them? It might be clearer to say something like "Don't create layout
objects ...".

FYI, this bug may be of interest, or you may have some insight to add:
crbug.com/637916

[erikchen, 2016-12-16 02:26:02]

> 
> > It would be easy to add more logic into layout to support this more cleanly,
> but you seem quite concerned about the performance of
> node/containernode/layoutobject. I think that tracking this via m_widget and
> setWidget() is simpler and cleaner, but would be happen to be proven wrong
[and
> to learn more about Blink in the process!]
> 
> Typical pages have thousands (5-20k) of elements and layout objects, we need
to
> keep the core infrastructure of what we run for every element very fast. We
also
> iterate the entire document calling certain methods many times, layoutObject()
> is super hot.

Sorry, I wasn't trying to imply that the performance implications weren't
significant. Just that that's why we can't muck around with the layout logic,
which would provide a "clean" solution without needing to hook into setWidget.

[erikchen, 2016-12-16 02:31:06]

Description was changed from

==========
Don't emit layout objects for display-none iframes.

TODO(erikchen): note to self - launch behind experiment.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * tests
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't emit layout objects for display-none iframes.

TODO(erikchen): note to self - launch behind experiment.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dcheng, 2016-12-16 02:36:34]

On 2016/12/16 01:51:47, esprehn wrote:
> On 2016/12/16 at 01:09:50, dcheng wrote:
> > On 2016/12/16 00:50:16, erikchen wrote:
> > > On 2016/12/15 21:45:56, esprehn wrote:
> > > > [...]
> > > 
> > > This pseudocode doesn't work when setting the display of a frame from
!none
> ->
> > > none, since the layoutObject() has already been detached from the previous
> call
> > > to detachLayoutObject(). My suggestion in
> > > https://codereview.chromium.org/2564633002/#msg31 was to add some state
> > > "m_hadLayoutObject" that gets tracked during
> > > attachLayoutObject/detachLayoutObject, but this doesn't seem very clean,
> since
> > > it relies on implicit assumptions about the ordering of calls to
> > > attach/detachLayoutObject. It would be easy to add more logic into layout
to
> > > support this more cleanly, but you seem quite concerned about the
> performance of
> > > node/containernode/layoutobject. I think that tracking this via m_widget
and
> > > setWidget() is simpler and cleaner, but would be happen to be proven wrong
> [and
> > > to learn more about Blink in the process!]
> > 
> > I'm just a little worried about tying this to widget: HTMLPlugInElement uses
> this as a base, and my recollection is plugins can be instantiated with a
widget
> even if they're display:none.
> 
> We don't load display: none plugins, except maybe for the guest view thing on
> cros? In general though I'm not very concerned about this, plugins don't have
a
> document inside the frame.
> 
> I think we should stick with setWidget for now, if it explodes we'll catch it
> and come up with a more complex solution.

OK, fair enough. I just hate adding more dependencies on the widget bits here,
but if that's the most straightforward approach, I'm OK with it.

[erikchen, 2016-12-16 02:37:13]

Description was changed from

==========
Don't emit layout objects for display-none iframes.

TODO(erikchen): note to self - launch behind experiment.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't create layout objects for display-none iframes.

TODO(erikchen): note to self - launch behind experiment.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-16 20:52:11]

Description was changed from

==========
Don't create layout objects for display-none iframes.

TODO(erikchen): note to self - launch behind experiment.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't create layout objects for display-none iframes.

TODO(erikchen): note to self - launch behind experiment.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-16 20:56:12]

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4420: if (isDisplayNone !=
owner->isDisplayNone() && documentElement()) {
On 2016/12/15 21:35:04, dcheng wrote:
> Nit: this is already null-checked at the beginning of the functoin.

Done.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:254: if (doc &&
doc->frame()) {
On 2016/12/15 21:35:04, dcheng wrote:
> I don't think it's possible to have a contentDocument() with no frame (since
> that would imply it's not attached)

This happens on creation. Attached a partial stack trace that gets emitted
during the crash if this conditional is missing.
"""
4   libblink_core.dylib                 0x0000000114a92ddd
_ZN5blink8Document30willChangeFrameOwnerPropertiesEiiNS_13ScrollbarModeEb + 317
5   libblink_core.dylib                 0x0000000115177a82
_ZN5blink21HTMLFrameOwnerElement9setWidgetEPNS_6WidgetE + 370
6   libblink_core.dylib                 0x000000011506833f
_ZN5blink10LocalFrame10createViewERKNS_7IntSizeERKNS_5ColorEbNS_13ScrollbarModeEbS7_b
+ 623
7   libblink_web.dylib                  0x000000011017b7b8
_ZN5blink17WebLocalFrameImpl15createFrameViewEv + 920
8   libblink_web.dylib                  0x0000000110051ff1
_ZN5blink21FrameLoaderClientImpl31transitionToCommittedForNewPageEv + 33
9   libblink_core.dylib                 0x0000000115a452b0
_ZN5blink11FrameLoader21commitProvisionalLoadEv + 784
"""

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10786: for (Node* child =
doc->documentElement()->firstChild(); child;
On 2016/12/15 21:36:48, esprehn wrote:
> There's no need to check this, if the documentElement has no layoutObject,
then
> it's children can't have them either.
> 
> !documentElement()->layoutObject() is enough.

but the document element does have a layout object. We overrode
LayoutView::canHaveChildren() so that it can't have children.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10795: TEST_F(WebFrameTest,
DisplayNoneIFrameHasNoLayoutObjects) {
On 2016/12/15 21:36:48, esprehn wrote:
> If you use a SimTest instead this whole this can be much simpler and you don't
> need any of the external files.

Done.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10821:
frame->executeScript(
On 2016/12/15 21:36:48, esprehn wrote:
> No need for JS, you can just do
> element->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);

Done.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10830: TEST_F(WebFrameTest,
NormalIFrameHasLayoutObjects) {
On 2016/12/15 21:36:48, esprehn wrote:
> SimTests will make this simpler:
> ex.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Docu...

Done.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/data/single_invisible_iframe.html
(right):

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/data/single_invisible_iframe.html:1: <html>
On 2016/12/15 21:36:48, esprehn wrote:
> Missing <!DOCTYPE html>

Done.

https://codereview.chromium.org/2564633002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/data/single_invisible_iframe.html:5: </html>
On 2016/12/15 21:36:48, esprehn wrote:
> You only need:
> 
> <!DOCTYPE html>
> <iframe style="display:none;" src="visible_iframe.html"></iframe>
> 
> the html, head and body are all optional.

the web is magical. done.

[esprehn, 2016-12-16 21:03:05]

The documentElement is not the LayoutView, that's the document itself.

document => LayoutView.
<html> (or any root element) => documentElement.

So we're preventing the document itself from having children, not the
documentElement.

ASSERT_TRUE(!documentElement->layoutObject());

should pass if this patch works.

[erikchen, 2016-12-16 21:12:23]

On 2016/12/16 21:03:05, esprehn wrote:
> The documentElement is not the LayoutView, that's the document itself.
> 
> document => LayoutView.
> <html> (or any root element) => documentElement.
> 
> So we're preventing the document itself from having children, not the
> documentElement.
> 
> ASSERT_TRUE(!documentElement->layoutObject());
> 
> should pass if this patch works.

ah, I see, thanks. Fixed and the tests still pass.

[erikchen, 2016-12-16 21:16:54]

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

[commit-bot: I haz the power, 2016-12-16 21:18:06]

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

[esprehn, 2016-12-16 21:29:44]

Bunch of nits, otherwise this lgtm, If you want to put this behind a
RuntimeEnabledFeature flag (just guard the lazyReattach call) and mark it
experimental we can land it before the intent to ship is approved if you want.
Or you can wait. :)

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html
(right):

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html:2:
<html>
I'd leave out html and body, the test doesn't need it.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html:3:
<script src="resources/text-based-repaint.js" type="text/javascript"></script>
no type attr

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html:7:
iframe.style.display = "none";
document.getElementById("iframe") avoids the magic that makes this lookup by id
work.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html:9:
window.addEventListener("load", runRepaintAndPixelTest, false);
no false

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-none-to-display-block.html
(right):

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-none-to-display-block.html:2:
<html>
ditto

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-none-to-display-block.html:3:
<script src="resources/text-based-repaint.js" type="text/javascript"></script>
remove type attr

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-none-to-display-block.html:9:
window.addEventListener("load", runRepaintAndPixelTest, false);
default is false, you can leave it out

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:255: bool
isDisplayNone = this->isDisplayNone();
If you remove the local you can void the this->

if (isDisplayNone() != willBeDisplayNone)

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11048: runPendingTasks();
compositor().beginFrame(), no need to do updateAllLifecyclePhases or
runPendingTasks.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11065: Element* element =
frame->frame()->document()->querySelector("iframe");
document().querySelector("iframe");

no need to go through the webView().mainFrameImpl().

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11067:
static_cast<HTMLFrameOwnerElement*>(element);
toHTMLFrameOwnerElement(element)

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11076: runPendingTasks();
ditto

[erikchen, 2016-12-16 22:42:11]

I put this behind a RuntimeEnabledFeatures flag. Note that I put a single check
in LayoutView::canHaveChildren().

The alternatives would be to change HTMLFrameOwnerElement::isDisplayNone(), or
try to catch all the places where this state is propagated [both for local and
remote frames].

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html
(right):

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html:2:
<html>
On 2016/12/16 21:29:44, esprehn wrote:
> I'd leave out html and body, the test doesn't need it.

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html:3:
<script src="resources/text-based-repaint.js" type="text/javascript"></script>
On 2016/12/16 21:29:44, esprehn wrote:
> no type attr

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html:7:
iframe.style.display = "none";
On 2016/12/16 21:29:44, esprehn wrote:
> document.getElementById("iframe") avoids the magic that makes this lookup by
id
> work.

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-block-to-display-none.html:9:
window.addEventListener("load", runRepaintAndPixelTest, false);
On 2016/12/16 21:29:44, esprehn wrote:
> no false

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-none-to-display-block.html
(right):

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-none-to-display-block.html:2:
<html>
On 2016/12/16 21:29:44, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-none-to-display-block.html:3:
<script src="resources/text-based-repaint.js" type="text/javascript"></script>
On 2016/12/16 21:29:44, esprehn wrote:
> remove type attr

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/iframe-display-none-to-display-block.html:9:
window.addEventListener("load", runRepaintAndPixelTest, false);
On 2016/12/16 21:29:44, esprehn wrote:
> default is false, you can leave it out

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:255: bool
isDisplayNone = this->isDisplayNone();
On 2016/12/16 21:29:44, esprehn wrote:
> If you remove the local you can void the this->
> 
> if (isDisplayNone() != willBeDisplayNone)

Done. [If you wish to avoid locals, sure, not sure why you brought up the
"this->"]

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11048: runPendingTasks();
On 2016/12/16 21:29:44, esprehn wrote:
> compositor().beginFrame(), no need to do updateAllLifecyclePhases or
> runPendingTasks.

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11065: Element* element =
frame->frame()->document()->querySelector("iframe");
On 2016/12/16 21:29:44, esprehn wrote:
> document().querySelector("iframe");
> 
> no need to go through the webView().mainFrameImpl().

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11067:
static_cast<HTMLFrameOwnerElement*>(element);
On 2016/12/16 21:29:44, esprehn wrote:
> toHTMLFrameOwnerElement(element)

Done.

https://codereview.chromium.org/2564633002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11076: runPendingTasks();
On 2016/12/16 21:29:44, esprehn wrote:
> ditto

Done.

[erikchen, 2016-12-16 22:42:41]

Description was changed from

==========
Don't create layout objects for display-none iframes.

TODO(erikchen): note to self - launch behind experiment.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Missing from this CL:
  * Intent to implement [which will need to be sent separately]

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-16 22:42:59]

Description was changed from

==========
Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is DisplayNone if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is "DisplayNone" if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[esprehn, 2016-12-16 22:48:04]

lgtm for landing, could you add a link to the spec discussion
https://github.com/whatwg/html/issues/1813 in the change description too?

[erikchen, 2016-12-16 22:48:59]

Description was changed from

==========
Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is "DisplayNone" if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is "DisplayNone" if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-16 22:50:12]

Description was changed from

==========
Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is "DisplayNone" if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is "DisplayNone" if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[erikchen, 2016-12-16 23:52:24]

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

[commit-bot: I haz the power, 2016-12-16 23:53:21]

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

[erikchen, 2016-12-17 01:08:29]

On 2016/12/16 22:48:04, esprehn wrote:
> lgtm for landing, could you add a link to the spec discussion
> https://github.com/whatwg/html/issues/1813 in the change description too?

esprehn: Looking at the test failures on ps#16 [I think ps#18 may have hit some
flaky errors - not sure why there are so many more], about half of the failures
are in wpt. Doing some digging, the root cause is that many of the wpt tests add
children to a display:none iframe and expect valid results.
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...

I got the open source version of web-platform-test. I ran the shadow-dom tests
on my local machine. Chrome fails 15 of them. Firefox fails 492. It seems like
their test suite needs to be updated. What's the right way forward here?

[commit-bot: I haz the power, 2016-12-17 01:49:08]

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

[commit-bot: I haz the power, 2016-12-17 01:49:09]

Dry run: 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_...)

[esprehn, 2016-12-17 01:57:42]

On 2016/12/17 at 01:08:29, erikchen wrote:
> On 2016/12/16 22:48:04, esprehn wrote:
> > lgtm for landing, could you add a link to the spec discussion
> > https://github.com/whatwg/html/issues/1813 in the change description too?
> 
> esprehn: Looking at the test failures on ps#16 [I think ps#18 may have hit
some flaky errors - not sure why there are so many more], about half of the
failures are in wpt. Doing some digging, the root cause is that many of the wpt
tests add children to a display:none iframe and expect valid results.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...
> 
> I got the open source version of web-platform-test. I ran the shadow-dom tests
on my local machine. Chrome fails 15 of them. Firefox fails 492. It seems like
their test suite needs to be updated. What's the right way forward here?

Oh yeah I forgot I failed those tests with my patch too. Those are the old
Shadow DOM v0 tests which are essentially dead (no other browser will ever
implement that spec). I would just remove the display = "none" here, it doesn't
make sense given it's used in newRenderedHTMLDocument anyway.
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...

[erikchen, 2016-12-17 02:18:34]

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

[commit-bot: I haz the power, 2016-12-17 02:19:11]

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-12-17 04:20:10]

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

[commit-bot: I haz the power, 2016-12-17 04:20:12]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[erikchen, 2016-12-20 19:24:51]

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

[commit-bot: I haz the power, 2016-12-20 19:25:35]

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

[erikchen, 2016-12-20 19:29:58]

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

[commit-bot: I haz the power, 2016-12-20 19:30:25]

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

[erikchen, 2016-12-20 19:34:29]

On 2016/12/17 01:57:42, esprehn wrote:
> On 2016/12/17 at 01:08:29, erikchen wrote:
> > On 2016/12/16 22:48:04, esprehn wrote:
> > > lgtm for landing, could you add a link to the spec discussion
> > > https://github.com/whatwg/html/issues/1813 in the change description too?
> > 
> > esprehn: Looking at the test failures on ps#16 [I think ps#18 may have hit
> some flaky errors - not sure why there are so many more], about half of the
> failures are in wpt. Doing some digging, the root cause is that many of the
wpt
> tests add children to a display:none iframe and expect valid results.
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...
> > 
> > I got the open source version of web-platform-test. I ran the shadow-dom
tests
> on my local machine. Chrome fails 15 of them. Firefox fails 492. It seems like
> their test suite needs to be updated. What's the right way forward here?
> 
> Oh yeah I forgot I failed those tests with my patch too. Those are the old
> Shadow DOM v0 tests which are essentially dead (no other browser will ever
> implement that spec). I would just remove the display = "none" here, it
doesn't
> make sense given it's used in newRenderedHTMLDocument anyway.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...

esprehn: PTAL

Three major changes since you've last reviewed:

1) I added a new layout test
"animations/display-none-iframe-has-no-animation.html"

2) I modified "fullscreen/resources/empty.html" to include an HTML tag.
[Apparently the page has a body() element without a documentElement(), which
confuses the new logic in Document.cpp. But in general, the new iframe
display:none logic assumes that all layoutobjects are rooted in the
documentElement().]

3) I rebaselined the failing layout tests. The new expectations all look
reasonable except for
"plugins/createScriptableObject-before-start-expected.txt", which is now a no-op
test, since it's trying to test some behavior in a display:none iframe. The test
itself looks really outdated [based on 2007 webkit bug]. Maybe I should add a
follow-up CL that just deletes the test?

[commit-bot: I haz the power, 2016-12-20 21:28:01]

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

[commit-bot: I haz the power, 2016-12-20 21:28:02]

Dry run: 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_...)

[erikchen, 2016-12-20 22:01:34]

On 2016/12/20 19:34:29, erikchen wrote:
> On 2016/12/17 01:57:42, esprehn wrote:
> > On 2016/12/17 at 01:08:29, erikchen wrote:
> > > On 2016/12/16 22:48:04, esprehn wrote:
> > > > lgtm for landing, could you add a link to the spec discussion
> > > > https://github.com/whatwg/html/issues/1813 in the change description
too?
> > > 
> > > esprehn: Looking at the test failures on ps#16 [I think ps#18 may have hit
> > some flaky errors - not sure why there are so many more], about half of the
> > failures are in wpt. Doing some digging, the root cause is that many of the
> wpt
> > tests add children to a display:none iframe and expect valid results.
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...
> > > 
> > > I got the open source version of web-platform-test. I ran the shadow-dom
> tests
> > on my local machine. Chrome fails 15 of them. Firefox fails 492. It seems
like
> > their test suite needs to be updated. What's the right way forward here?
> > 
> > Oh yeah I forgot I failed those tests with my patch too. Those are the old
> > Shadow DOM v0 tests which are essentially dead (no other browser will ever
> > implement that spec). I would just remove the display = "none" here, it
> doesn't
> > make sense given it's used in newRenderedHTMLDocument anyway.
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...
> 
> esprehn: PTAL
> 
> Three major changes since you've last reviewed:
> 
> 1) I added a new layout test
> "animations/display-none-iframe-has-no-animation.html"
> 
> 2) I modified "fullscreen/resources/empty.html" to include an HTML tag.
> [Apparently the page has a body() element without a documentElement(), which
> confuses the new logic in Document.cpp. But in general, the new iframe
> display:none logic assumes that all layoutobjects are rooted in the
> documentElement().]
> 
> 3) I rebaselined the failing layout tests. The new expectations all look
> reasonable except for
> "plugins/createScriptableObject-before-start-expected.txt", which is now a
no-op
> test, since it's trying to test some behavior in a display:none iframe. The
test
> itself looks really outdated [based on 2007 webkit bug]. Maybe I should add a
> follow-up CL that just deletes the test?

Ooph. This CL still has issues. Let me enumerate them.
1) We need to guard the changes to Document::willChangeFrameOwnerProperties in a
RuntimeEnabledFeatures::displayNoneIFrameCreatesNoLayoutObjectEnabled()
conditional. [otherwise causes unnecessary reattaches].
2) The current logic doesn't work. When
fullscreen/full-screen-iframe-zIndex.html enters fullscreen, the page is still
all black, which is really surprising since nothing has a black background.
Here's what I think is happening:
  2a) During style recalculation we call HTMLFrameOwnerElement::setWidget().
"""
2   libblink_core.dylib                 0x00000001273c915a
_ZN5blink8Document30willChangeFrameOwnerPropertiesEiiNS_13ScrollbarModeEb + 426
3   libblink_core.dylib                 0x0000000127aaf12c
_ZN5blink21HTMLFrameOwnerElement9setWidgetEPNS_6WidgetE + 380
4   libblink_core.dylib                 0x0000000127aacda7
_ZN5blink20HTMLFrameElementBase16attachLayoutTreeERKNS_4Node13AttachContextE +
151
5   libblink_core.dylib                 0x0000000127511428
_ZN5blink4Node18reattachLayoutTreeERKNS0_13AttachContextE + 120
6   libblink_core.dylib                 0x0000000127467d05
_ZN5blink7Element17rebuildLayoutTreeEv + 997
7   libblink_core.dylib                 0x0000000127466dea
_ZN5blink7Element14recalcOwnStyleENS_17StyleRecalcChangeEPNS_4TextE + 1706
8   libblink_core.dylib                 0x0000000127466460
_ZN5blink7Element11recalcStyleENS_17StyleRecalcChangeEPNS_4TextE + 1152
9   libblink_core.dylib                 0x00000001273612cf
_ZN5blink13ContainerNode22recalcDescendantStylesENS_17StyleRecalcChangeE + 911
10  libblink_core.dylib                 0x00000001274665a7
_ZN5blink7Element11recalcStyleENS_17StyleRecalcChangeEPNS_4TextE + 1479
11  libblink_core.dylib                 0x00000001273612cf
_ZN5blink13ContainerNode22recalcDescendantStylesENS_17StyleRecalcChangeE + 911
12  libblink_core.dylib                 0x00000001274665a7
_ZN5blink7Element11recalcStyleENS_17StyleRecalcChangeEPNS_4TextE + 1479
13  libblink_core.dylib                 0x00000001273b5883
_ZN5blink8Document11updateStyleEv + 1187
14  libblink_core.dylib                 0x00000001273b0364
_ZN5blink8Document24updateStyleAndLayoutTreeEv + 1284
"""
  2b) This results in a call to documentElement()->lazyReattachIfAttached().
This causes re-entrancy in the layout logic, leaving the layout tree in an
incorrect state.

I'm not sure what the appropriate way to deal with this is - thoughts esprehn?

Open question:
Should we support frames that have a body() but no documentElement()?

[erikchen, 2016-12-20 22:06:46]

On 2016/12/20 22:01:34, erikchen wrote:
> On 2016/12/20 19:34:29, erikchen wrote:
> > On 2016/12/17 01:57:42, esprehn wrote:
> > > On 2016/12/17 at 01:08:29, erikchen wrote:
> > > > On 2016/12/16 22:48:04, esprehn wrote:
> > > > > lgtm for landing, could you add a link to the spec discussion
> > > > > https://github.com/whatwg/html/issues/1813 in the change description
> too?
> > > > 
> > > > esprehn: Looking at the test failures on ps#16 [I think ps#18 may have
hit
> > > some flaky errors - not sure why there are so many more], about half of
the
> > > failures are in wpt. Doing some digging, the root cause is that many of
the
> > wpt
> > > tests add children to a display:none iframe and expect valid results.
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...
> > > > 
> > > > I got the open source version of web-platform-test. I ran the shadow-dom
> > tests
> > > on my local machine. Chrome fails 15 of them. Firefox fails 492. It seems
> like
> > > their test suite needs to be updated. What's the right way forward here?
> > > 
> > > Oh yeah I forgot I failed those tests with my patch too. Those are the old
> > > Shadow DOM v0 tests which are essentially dead (no other browser will ever
> > > implement that spec). I would just remove the display = "none" here, it
> > doesn't
> > > make sense given it's used in newRenderedHTMLDocument anyway.
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...
> > 
> > esprehn: PTAL
> > 
> > Three major changes since you've last reviewed:
> > 
> > 1) I added a new layout test
> > "animations/display-none-iframe-has-no-animation.html"
> > 
> > 2) I modified "fullscreen/resources/empty.html" to include an HTML tag.
> > [Apparently the page has a body() element without a documentElement(), which
> > confuses the new logic in Document.cpp. But in general, the new iframe
> > display:none logic assumes that all layoutobjects are rooted in the
> > documentElement().]
> > 
> > 3) I rebaselined the failing layout tests. The new expectations all look
> > reasonable except for
> > "plugins/createScriptableObject-before-start-expected.txt", which is now a
> no-op
> > test, since it's trying to test some behavior in a display:none iframe. The
> test
> > itself looks really outdated [based on 2007 webkit bug]. Maybe I should add
a
> > follow-up CL that just deletes the test?
> 
> Ooph. This CL still has issues. Let me enumerate them.
> 1) We need to guard the changes to Document::willChangeFrameOwnerProperties in
a
> RuntimeEnabledFeatures::displayNoneIFrameCreatesNoLayoutObjectEnabled()
> conditional. [otherwise causes unnecessary reattaches].
> 2) The current logic doesn't work. When
> fullscreen/full-screen-iframe-zIndex.html enters fullscreen, the page is still
> all black, which is really surprising since nothing has a black background.
> Here's what I think is happening:
>   2a) During style recalculation we call HTMLFrameOwnerElement::setWidget().
> """
> 2   libblink_core.dylib                 0x00000001273c915a
> _ZN5blink8Document30willChangeFrameOwnerPropertiesEiiNS_13ScrollbarModeEb +
426
> 3   libblink_core.dylib                 0x0000000127aaf12c
> _ZN5blink21HTMLFrameOwnerElement9setWidgetEPNS_6WidgetE + 380
> 4   libblink_core.dylib                 0x0000000127aacda7
> _ZN5blink20HTMLFrameElementBase16attachLayoutTreeERKNS_4Node13AttachContextE +
> 151
> 5   libblink_core.dylib                 0x0000000127511428
> _ZN5blink4Node18reattachLayoutTreeERKNS0_13AttachContextE + 120
> 6   libblink_core.dylib                 0x0000000127467d05
> _ZN5blink7Element17rebuildLayoutTreeEv + 997
> 7   libblink_core.dylib                 0x0000000127466dea
> _ZN5blink7Element14recalcOwnStyleENS_17StyleRecalcChangeEPNS_4TextE + 1706
> 8   libblink_core.dylib                 0x0000000127466460
> _ZN5blink7Element11recalcStyleENS_17StyleRecalcChangeEPNS_4TextE + 1152
> 9   libblink_core.dylib                 0x00000001273612cf
> _ZN5blink13ContainerNode22recalcDescendantStylesENS_17StyleRecalcChangeE + 911
> 10  libblink_core.dylib                 0x00000001274665a7
> _ZN5blink7Element11recalcStyleENS_17StyleRecalcChangeEPNS_4TextE + 1479
> 11  libblink_core.dylib                 0x00000001273612cf
> _ZN5blink13ContainerNode22recalcDescendantStylesENS_17StyleRecalcChangeE + 911
> 12  libblink_core.dylib                 0x00000001274665a7
> _ZN5blink7Element11recalcStyleENS_17StyleRecalcChangeEPNS_4TextE + 1479
> 13  libblink_core.dylib                 0x00000001273b5883
> _ZN5blink8Document11updateStyleEv + 1187
> 14  libblink_core.dylib                 0x00000001273b0364
> _ZN5blink8Document24updateStyleAndLayoutTreeEv + 1284
> """
>   2b) This results in a call to documentElement()->lazyReattachIfAttached().
> This causes re-entrancy in the layout logic, leaving the layout tree in an
> incorrect state.
> 
> I'm not sure what the appropriate way to deal with this is - thoughts esprehn?
> 
> Open question:
> Should we support frames that have a body() but no documentElement()?

The "easy" solution that comes to mind is we could dispatch the
documentElement()->lazyReattachIfNecessary() as a task on the runloop. This
nicely avoids re-entrancy issues, but has potential performance implications.

[esprehn, 2016-12-20 22:14:51]

You can't have a body and no documentElement. The body is the first element
inside the documentElement with tag name "body". This feature doesn't have
anything to do with bodies, it doesn't matter if the page has one or not.

There shouldn't be any reentrancy, you're always calling downward into frames.

[erikchen, 2016-12-20 22:35:35]

On 2016/12/20 22:14:51, esprehn wrote:
> You can't have a body and no documentElement. The body is the first element
> inside the documentElement with tag name "body". This feature doesn't have
> anything to do with bodies, it doesn't matter if the page has one or not.
You're right. 

> 
> There shouldn't be any reentrancy, you're always calling downward into frames.
I will continue to investigate further then.

[erikchen, 2016-12-21 03:17:09]

I have a partial explanation. Still need to dig further. The problem is lies at
the intersection of "auto" sizing, fullscreen, and layout of the HTML element.

Observations: 
1) Layout in fullscreen, with last two numbers representing width/height. Whole
page is black.
"""
[44123:775:1220/190553.177609:ERROR:LayoutView.cpp(244)] #document
0x1ccef34041c0 0x1ccef34140f0 0x1ccef34041c0 1920 1120
[44123:775:1220/190553.177918:ERROR:LayoutView.cpp(244)]   HTML 0x1ccef341c700
0x1ccef3414470 0x1ccef341c700 16 1120
[44123:775:1220/190553.178284:ERROR:LayoutView.cpp(246)]     HEAD 0x0
[44123:775:1220/190553.178620:ERROR:LayoutView.cpp(244)]     BODY
style="background:#FF0000;" 0x1ccef341c828 0x1ccef3414470 0x1ccef341c828 0 1104
[44123:775:1220/190553.178927:ERROR:LayoutView.cpp(246)]       #text "\n" 0x0
"""

2) Explicit sizing for "body" element. [width:100%;height:100%]. Now we see a
red vertical stripe down the middle.
"""
[44198:775:1220/190746.685446:ERROR:LayoutView.cpp(244)] #document 0x53ce041c0
0x53ce140f0 0x53ce041c0 1920 1080
[44198:775:1220/190746.685746:ERROR:LayoutView.cpp(244)]   HTML 0x53ce1c700
0x53ce14470 0x53ce1c700 16 1096
[44198:775:1220/190746.686057:ERROR:LayoutView.cpp(246)]     HEAD 0x0
[44198:775:1220/190746.686345:ERROR:LayoutView.cpp(244)]     BODY
style="background:#FF0000; width:100%; height:100%" 0x53ce1c828 0x53ce14470
0x53ce1c828 16 1080
[44198:775:1220/190746.686657:ERROR:LayoutView.cpp(246)]       #text "\n" 0x0
"""

3) Explicit sizing for "html" element.[width:100%;height:100%]. Whole page is
red.
"""
[44215:775:1220/190849.098662:ERROR:LayoutView.cpp(244)] #document
0x20c622a041c0 0x20c622a140f0 0x20c622a041c0 1920 1120
[44215:775:1220/190849.099014:ERROR:LayoutView.cpp(244)]   HTML
style="background:#FFFF00; width:100%; height:100%" 0x20c622a1c700
0x20c622a14470 0x20c622a1c700 1920 1120
[44215:775:1220/190849.099374:ERROR:LayoutView.cpp(246)]     HEAD 0x0
[44215:775:1220/190849.099735:ERROR:LayoutView.cpp(244)]     BODY
style="background:#FF0000; width:100%; height:100%" 0x20c622a1c828
0x20c622a14470 0x20c622a1c828 1920 1120
[44215:775:1220/190849.100038:ERROR:LayoutView.cpp(246)]       #text "\n\n" 0x0
"""

4)  Looking at the layouts, the general flow seems to be:
  4a) user clicks fullscreen. This triggers a transition which triggers a
setWidget(nullptr); setWidget(<previous widget>). This schedules
layoutview::layout. 
  4b) When the layoutObjects are recreated, they lose their stored state
[width=200] and become [width = 0]. This also schedules layoutview::layout.
  4c) During layout, the HTML element decides that its appropriate width is "16
px".

My *guess* is that layout for entering fullscreen is stateful, and looks at the
currently layout state [stretches elements appropriately?], so the fact that
(4b) changes layout right before a fullscreen-triggered relayout causes a
different result.

[erikchen, 2016-12-21 19:23:00]

ah ha!. The layout tree in fullscreen is actually different pre/post patch.

Pre patch:
[84103:775:1221/111005.305536:ERROR:LayoutView.cpp(244)] #document
0x189de04041c0 1905 1105
[84103:775:1221/111005.305850:ERROR:LayoutView.cpp(244)]   HTML 0x189de041c700
1905 1121
[84103:775:1221/111005.306209:ERROR:LayoutView.cpp(244)]     BODY
style="background:#FF0000; width:100%; height:100%" 0x189de041c828 1905 1105

Post patch:
[84343:775:1221/111501.353602:ERROR:LayoutView.cpp(244)] #document
0x313a61c041c0 1920 1120
[84343:775:1221/111501.353967:ERROR:LayoutView.cpp(246)]   0x313a61c38198 1920
1120
[84343:775:1221/111501.354253:ERROR:LayoutView.cpp(244)]     HTML 0x313a61c1c700
16 1136
[84343:775:1221/111501.354559:ERROR:LayoutView.cpp(244)]       BODY
style="background:#FF0000; width:100%; height:100%" 0x313a61c1c828 16 1120

There's an anonymous "flexible" box layoutobject that is causing the observed
difference with the HTML/BODY elements.

[esprehn, 2016-12-21 19:25:45]

On 2016/12/21 at 19:23:00, erikchen wrote:
> ah ha!. The layout tree in fullscreen is actually different pre/post patch.
> 
> ...
> 
> There's an anonymous "flexible" box layoutobject that is causing the observed
difference with the HTML/BODY elements.

Sounds like a bug, foolip@ can help with Fullscreen. :)

[erikchen, 2016-12-28 00:13:29]

On 2016/12/21 19:25:45, esprehn wrote:
> On 2016/12/21 at 19:23:00, erikchen wrote:
> > ah ha!. The layout tree in fullscreen is actually different pre/post patch.
> > 
> > ...
> > 
> > There's an anonymous "flexible" box layoutobject that is causing the
observed
> difference with the HTML/BODY elements.
> 
> Sounds like a bug, foolip@ can help with Fullscreen. :)

Filed at https://bugs.chromium.org/p/chromium/issues/detail?id=676432

[erikchen, 2016-12-28 00:15:24]

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

[commit-bot: I haz the power, 2016-12-28 00:15:41]

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-12-28 01:57:18]

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

[commit-bot: I haz the power, 2016-12-28 01:57:20]

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

[erikchen, 2017-01-18 02:15:52]

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

[commit-bot: I haz the power, 2017-01-18 02:16:12]

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-01-18 02:18:58]

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

[commit-bot: I haz the power, 2017-01-18 02:18:59]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[erikchen, 2017-01-18 02:29:16]

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

[commit-bot: I haz the power, 2017-01-18 02:29:42]

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-01-18 02:32:25]

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

[commit-bot: I haz the power, 2017-01-18 02:32:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[erikchen, 2017-01-19 21:18:39]

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

[commit-bot: I haz the power, 2017-01-19 21:19:00]

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-01-19 21:39:22]

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

[commit-bot: I haz the power, 2017-01-19 21:39:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-01-19 21:47:37]

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

[commit-bot: I haz the power, 2017-01-19 21:48:09]

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-01-19 23:33:55]

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

[commit-bot: I haz the power, 2017-01-19 23:33:56]

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

[erikchen, 2017-01-21 01:02:20]

On 2016/12/28 00:13:29, erikchen wrote:
> On 2016/12/21 19:25:45, esprehn wrote:
> > On 2016/12/21 at 19:23:00, erikchen wrote:
> > > ah ha!. The layout tree in fullscreen is actually different pre/post
patch.
> > > 
> > > ...
> > > 
> > > There's an anonymous "flexible" box layoutobject that is causing the
> observed
> > difference with the HTML/BODY elements.
> > 
> > Sounds like a bug, foolip@ can help with Fullscreen. :)
> 
> Filed at https://bugs.chromium.org/p/chromium/issues/detail?id=676432

esprehn: The relevant test that fails is
fullscreen/full-screen-iframe-zIndex.html. As I indicate in
https://bugs.chromium.org/p/chromium/issues/detail?id=676432 with a minimized
repro, the relevant bug has been present for quite some time. What is the
appropriate approach here, given that foolip@ has said that they are working on
a more-general refactor, which may-or-may-not fix this issue?

[erikchen, 2017-01-23 19:40:06]

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

[commit-bot: I haz the power, 2017-01-23 19:40:39]

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-01-23 21:30:18]

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

[commit-bot: I haz the power, 2017-01-23 21:30:21]

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

[erikchen, 2017-01-23 23:01:13]

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

[commit-bot: I haz the power, 2017-01-23 23:02:50]

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-01-24 00:52:59]

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

[commit-bot: I haz the power, 2017-01-24 00:53:00]

Dry run: 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_...)

[erikchen, 2017-02-03 21:33:02]

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

[commit-bot: I haz the power, 2017-02-03 21:33:35]

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

[erikchen, 2017-02-03 22:14:45]

erikchen@chromium.org changed reviewers:
+ avi@chromium.org

[erikchen, 2017-02-03 22:14:46]

dcheng: Please review content/common/frame_messages.h.
avi: Please review content/

[Avi (use Gerrit), 2017-02-03 22:28:32]

On 2017/02/03 22:14:46, erikchen wrote:
> dcheng: Please review content/common/frame_messages.h.
> avi: Please review content/

LGTM

[commit-bot: I haz the power, 2017-02-03 23:11:32]

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

[commit-bot: I haz the power, 2017-02-03 23:11:34]

Dry run: 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_...)

[dcheng, 2017-02-05 09:56:08]

https://codereview.chromium.org/2564633002/diff/610001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2564633002/diff/610001/content/common/frame_m...
content/common/frame_messages.h:179: IPC_STRUCT_TRAITS_MEMBER(is_display_none)
Currently this is passed via IPC, but isn't actually replicated. Will a followup
patch implement the browser-side replication?

https://codereview.chromium.org/2564633002/diff/610001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2564633002/diff/610001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4503: if (isDisplayNone !=
owner->isDisplayNone()) {
Nit: combine with the previous if ()

[erikchen, 2017-02-10 17:19:31]

https://codereview.chromium.org/2564633002/diff/610001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2564633002/diff/610001/content/common/frame_m...
content/common/frame_messages.h:179: IPC_STRUCT_TRAITS_MEMBER(is_display_none)
On 2017/02/05 09:56:08, dcheng wrote:
> Currently this is passed via IPC, but isn't actually replicated. Will a
followup
> patch implement the browser-side replication?

I believe this CL correctly passes is_display_none state [and there are tests
that pass that should fail if it weren't]. Perhaps we could discuss more
offline?

[dcheng, 2017-02-13 23:13:55]

Sorry for the delayed review. You're right that I think this Just Works
(unfortunately, we have two different ways of plumbing replicated state and I
mixed things up). That being said, I'm not sure that any of the layout tests
actually cover the OOPIF case: they would need to be http tests with
cross-origin frames to actually trigger coverage.

In any case, IPC LGTM but we may want to consider adding OOPIF specific tests in
a followup.

[erikchen, 2017-02-13 23:27:24]

I've noted your request for tests in the main bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=650433#c16

[erikchen, 2017-02-13 23:27:53]

On 2017/02/13 23:27:24, erikchen wrote:
> I've noted your request for tests in the main bug:
> https://bugs.chromium.org/p/chromium/issues/detail?id=650433#c16

*OOPIF tests

[erikchen, 2017-02-13 23:28:10]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-02-13 23:28:11]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2564633002/#ps610001
(title: "Rebase.")

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

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-13 23:33:45]

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

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

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-02-14 01:03:46]

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

[commit-bot: I haz the power, 2017-02-14 01:04:42]

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-14 03:03:34]

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

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

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

[erikchen, 2017-02-14 18:25:07]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-02-14 18:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
esprehn@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2564633002/#ps650001
(title: "Rebaseline.")

[commit-bot: I haz the power, 2017-02-14 18:25:35]

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 22:11:28]

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

[commit-bot: I haz the power, 2017-02-14 22:11:30]

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_...)

[erikchen, 2017-02-15 16:55:17]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-02-15 16:55:45]

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-15 18:54:59]

CQ is committing da patch.

Bot data: {"patchset_id": 650001, "attempt_start_ts": 1487177717076250,
"parent_rev": "224a858114095586e1d7a2f782a8a7ba21328656", "commit_rev":
"962434d34e2c9bcd17488f499515920c298b5395"}

[commit-bot: I haz the power, 2017-02-15 18:55:48]

Description was changed from

==========
Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is "DisplayNone" if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't create layout objects for children of display-none iframes.

Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
  * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
  * A local frame is "DisplayNone" if and only if it has no widget [which should
    correspond to the actual display:none style].
  * The DisplayNone property is propagated to remote frames via
    FrameOwnerProperties.

Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2564633002
Cr-Commit-Position: refs/heads/master@{#450748}
Committed:
https://chromium.googlesource.com/chromium/src/+/962434d34e2c9bcd17488f499515...
==========

[commit-bot: I haz the power, 2017-02-15 18:55:51]

Committed patchset #31 (id:650001) as
https://chromium.googlesource.com/chromium/src/+/962434d34e2c9bcd17488f499515...

[Matt Giuca, 2017-02-16 03:03:21]

A revert of this CL (patchset #31 id:650001) has been created in
https://codereview.chromium.org/2696283003/ by mgiuca@chromium.org.

The reason for reverting is: Suspect for webkit_tests leak failures
(https://crbug.com/692872).

See bug for details..

[Matt Giuca, 2017-02-16 03:06:48]

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

[Matt Giuca, 2017-02-16 03:06:50]

Sorry to have done this... it was painful hitting that revert button on such a
huge CL. I will monitor whether that fixes it. -- Your friendly sheriff.