Don't call isURLAllowed() from layout.

Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

Since this breaks some unit tests that manually bootstrap child
frame creation, this CL also remove the custom bootstrapping code
in favor of using the standard FrameLoaderClient interfaces: this
removes a bunch of code duplication for setting up the FrameView
of a child frame.

BUG=618138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/8a8505dc2c0cd9d4b3e347b7a9ccd9fb3e62491b
Cr-Commit-Position: refs/heads/master@{#433341}

[dcheng, 2016-11-15 08:29:35]

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

[commit-bot: I haz the power, 2016-11-15 08:29:51]

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

[dcheng, 2016-11-15 08:31:03]

Description was changed from

==========
Don't call isURLAllowed() from layout.

Some of the checks in isURLAllowed() can't be done in layout.

BUG=631330
==========

to

==========
Don't call isURLAllowed() from layout.

Some of the checks in isURLAllowed() can't be done in layout.

BUG=618138
==========

[dcheng, 2016-11-15 08:31:44]

Description was changed from

==========
Don't call isURLAllowed() from layout.

Some of the checks in isURLAllowed() can't be done in layout.

BUG=618138
==========

to

==========
Don't call isURLAllowed() from layout.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

BUG=618138
==========

[dcheng, 2016-11-15 08:32:55]

Description was changed from

==========
Don't call isURLAllowed() from layout.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

BUG=618138
==========

to

==========
Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

BUG=618138
==========

[dcheng, 2016-11-17 08:10:57]

Description was changed from

==========
Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

BUG=618138
==========

to

==========
Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

BUG=618138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[dcheng, 2016-11-17 08:47:22]

Description was changed from

==========
Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

BUG=618138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

Since this breaks some unit tests that manually bootstrap child
frame creation, this CL also remove the custom bootstrapping code
in favor of using the standard FrameLoaderClient interfaces: this
removes a bunch of code duplication for setting up the FrameView
of a child frame.

BUG=618138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[dcheng, 2016-11-17 08:47:23]

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

[dcheng, 2016-11-17 08:49:58]

https://codereview.chromium.org/2504573002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/misc/bubble-drag-events-expected.txt
(left):

https://codereview.chromium.org/2504573002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/misc/bubble-drag-events-expected.txt:6:

These diffs are because we no longer have a content frame in this case. If
there's no content frame attached, there's no point in giving the element a
LayoutObject.

https://codereview.chromium.org/2504573002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right):

https://codereview.chromium.org/2504573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:5: #include
"core/html/HTMLFrameOwnerElement.h"
So that line 223 can convert from HTMLFrameOwnerElement to Element.

https://codereview.chromium.org/2504573002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp
(right):

https://codereview.chromium.org/2504573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp:20:
: RenderingTest(EmptyFrameLoaderClient::create()) {}
These tests don't actually need iframes, so just default to the empty frame
loader client.

[dcheng, 2016-11-17 08:50:14]

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

[commit-bot: I haz the power, 2016-11-17 08:50:31]

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

[dcheng, 2016-11-17 11:25:47]

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

[commit-bot: I haz the power, 2016-11-17 11:26:08]

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-11-17 13:43:19]

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

[commit-bot: I haz the power, 2016-11-17 13:43:20]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-11-18 19:27:43]

ping

[esprehn, 2016-11-18 19:43:02]

What do other browsers do here? Do iframes really not paint their border and
backgrounds when the frame load is blocked?

lgtm

https://codereview.chromium.org/2504573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right):

https://codereview.chromium.org/2504573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:194:
setChildFrameHTML(
Why did you need to rearrange all of these tests?

https://codereview.chromium.org/2504573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:223: Element*
frameElement = childDocument().localOwner();
I prefer the getElementById since that's using fewer internal APIs.

[dcheng, 2016-11-18 21:16:40]

On 2016/11/18 19:43:02, esprehn wrote:
> What do other browsers do here? Do iframes really not paint their border and
> backgrounds when the frame load is blocked?

isURLAllowed() is weird and probably needs to be tweaked further in followups.
It serves two purposes:

- Block infinite recursive frames.
- Block javascript: URLs.

Note that things like loading file:// URLs from web content are blocked at
another level...

For infinitely recursive frames, it seems OK to not have a layout object:
there's badness happening here. I think Edge / FF match this, but I need to
double check their behavior.

For javascript URLs, we almost always have a contentFrame() already anyway. An
iframe with an initial src=javascript:... will always be permitted (and thus
will have a content frame). An iframe that later fails the javascript: URL check
will already have a content frame, so nothing changes...

However, if the iframe was initially blocked (for being infinitely recursive)
and then navigates somewhere else, things can get weird. But overall, I think
this makes things a little less weird.

https://codereview.chromium.org/2504573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right):

https://codereview.chromium.org/2504573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:194:
setChildFrameHTML(
On 2016/11/18 19:43:02, esprehn wrote:
> Why did you need to rearrange all of these tests?

I reverted this to the original ordering to reduce the size of the diffs. The
design evolved a bit while I was hacking on LayoutTestHelper, and there's no
reason this needs to be reversed anymore.

https://codereview.chromium.org/2504573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:223: Element*
frameElement = childDocument().localOwner();
On 2016/11/18 19:43:02, esprehn wrote:
> I prefer the getElementById since that's using fewer internal APIs.

Done.

[dcheng, 2016-11-18 21:16:50]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-11-18 21:16:50]

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/2504573002/#ps140001
(title: "Fix bad formatting that caused clang-format-diff to get confused.")

[commit-bot: I haz the power, 2016-11-18 21:17:49]

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-11-18 23:20:37]

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

[commit-bot: I haz the power, 2016-11-18 23:20:38]

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

[dcheng, 2016-11-18 23:22:59]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-11-18 23:23:50]

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-11-19 00:05:19]

Description was changed from

==========
Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

Since this breaks some unit tests that manually bootstrap child
frame creation, this CL also remove the custom bootstrapping code
in favor of using the standard FrameLoaderClient interfaces: this
removes a bunch of code duplication for setting up the FrameView
of a child frame.

BUG=618138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

Since this breaks some unit tests that manually bootstrap child
frame creation, this CL also remove the custom bootstrapping code
in favor of using the standard FrameLoaderClient interfaces: this
removes a bunch of code duplication for setting up the FrameView
of a child frame.

BUG=618138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-11-19 00:05:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-19 00:08:02]

Description was changed from

==========
Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

Since this breaks some unit tests that manually bootstrap child
frame creation, this CL also remove the custom bootstrapping code
in favor of using the standard FrameLoaderClient interfaces: this
removes a bunch of code duplication for setting up the FrameView
of a child frame.

BUG=618138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Don't use isURLAllowed() to determine if a frame element gets a LayoutObject.

What the style recalc code really wants to do is avoid creating a
LayoutObject if the frame load was blocked. The best way to detect
this is checking if contentFrame() is non-null.

Since this breaks some unit tests that manually bootstrap child
frame creation, this CL also remove the custom bootstrapping code
in favor of using the standard FrameLoaderClient interfaces: this
removes a bunch of code duplication for setting up the FrameView
of a child frame.

BUG=618138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/8a8505dc2c0cd9d4b3e347b7a9ccd9fb3e62491b
Cr-Commit-Position: refs/heads/master@{#433341}
==========

[commit-bot: I haz the power, 2016-11-19 00:08:03]

Patchset 8 (id:??) landed as
https://crrev.com/8a8505dc2c0cd9d4b3e347b7a9ccd9fb3e62491b
Cr-Commit-Position: refs/heads/master@{#433341}