Cross-origin iframes: collect data under hypothetical loading strategies

Cross-origin iframes: collect data under hypothetical loading strategies

Log % of frames to never load if we only loaded those that are
- visible, or
- 1 screen below
- 2 screens below
- 3 screens below

We would load frames used for cross-origin talk in any of the scenarios.

Note that we only look below for frames we might have to preload and ignore
frames to the right, assuming that the vast majority of content is
consumed vertically.

Also note that logging to one bucket will also log to each lower bucket,
which makes sense: if a frame has to be loaded under the "1 screen"
scenario, it would have to be loaded under a "3 screen" scenario.

This data will inform discussion of what our policy might eventually be.

BUG=635105
Review-Url: https://codereview.chromium.org/2516473002
Cr-Commit-Position: refs/heads/master@{#453837}
Committed: https://chromium.googlesource.com/chromium/src/+/86dfe7dc82290cede82086ad6e42ecaf54af4376

[dgrogan, 2016-11-17 22:28:33]

dgrogan@chromium.org changed reviewers:
+ ojan@chromium.org

[dgrogan, 2016-11-17 22:28:34]

https://codereview.chromium.org/2516473002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/2516473002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.h:1026: bool
m_needsUpdateViewportIntersection;
A previous CL removed the last uses. Just cleaning up here.

[ojan, 2016-11-22 00:41:59]

I don't think we should measure manhattan distance. In practice, people almost
entirely scroll vertically. When they do scroll horizontally, it's not very far
usually. So, I think you want something like:
WouldLoad1ScreenAwayVertical
WouldLoad2ScreenAwayVertical
WouldLoad3ScreenAwayVertical
WouldLoad1ScreenAwayHorizontal

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

https://codereview.chromium.org/2516473002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:6413: for (int i =
m_wouldLoadReason + 1; i <= reason; ++i)
I think this means that passing WouldLoadNoParent would make it so that all of
the other WouldLoad* reasons would get set as well. Is that what you're
intending?

https://codereview.chromium.org/2516473002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/DeferredLoadingTest.cpp (right):

https://codereview.chromium.org/2516473002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/DeferredLoadingTest.cpp:243:
histogramTester.expectBucketCount(kHistogramName, WouldLoadNoParent, 1);
This test doesn't seem to be testing the two screens away codepath since it's
hitting the oopif case, right?

[dgrogan, 2016-11-29 00:57:32]

I'm still thinking about manhattan vs not. I get your point, that users scroll
vertically more often, but I surmise that iframes are also placed vertically
more often, probably in proportion to how often users scroll down.

Put another way, I suspect publishers place frames to the right when they expect
users to scroll right and (the more common case) place frames below when they
expect users to scroll down. If that's accurate then manhattan is the way to go.

Maybe because right scrolling is so much rarer (do we have data for this?) we
can ignore it altogether and just collect
WouldLoad1ScreenAwayVertical
WouldLoad2ScreenAwayVertical
WouldLoad3ScreenAwayVertical

Anyway, I'll think more about it. More thoughts are welcome.

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

https://codereview.chromium.org/2516473002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:6413: for (int i =
m_wouldLoadReason + 1; i <= reason; ++i)
On 2016/11/22 00:41:59, ojan wrote:
> I think this means that passing WouldLoadNoParent would make it so that all of
> the other WouldLoad* reasons would get set as well. Is that what you're
> intending?

Intentional; it's simpler to subtract in the postprocessing step than it is to
make this code do something smarter. But changing from manhattan distance will
make this code more complicated so it might make sense to go ahead and give
WouldLoadNoParent special treatment here.

https://codereview.chromium.org/2516473002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/DeferredLoadingTest.cpp (right):

https://codereview.chromium.org/2516473002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/DeferredLoadingTest.cpp:243:
histogramTester.expectBucketCount(kHistogramName, WouldLoadNoParent, 1);
On 2016/11/22 00:41:59, ojan wrote:
> This test doesn't seem to be testing the two screens away codepath since it's
> hitting the oopif case, right?

It's testing that the 3screen and 2screen buckets don't get counted twice. Line
238 sets display to block (maybe you missed that part?), which calls
Document::recordDeferredLoadReason(WouldLoad2ScreensAway)

[dgrogan, 2016-11-30 21:55:39]

So I think we want the existing approach.

The only scenario where it's inferior to the 3-vertical-1-horizontal approach is
where there are a bunch of frames 2 or more screens to the right of
vertically-consumed content. That seems far-fetched.

The existing approach will also capture content that is consumed horizontally.

But of course horizontally-consumed content is rarer. So if we want to discount
it, why not get data that is really simple to interpret by only gathering
vertical data and ignoring frames to the right of the fold completely?

The 3-vertical-1-horizontal data wouldn't be simple to interpret because of the
case where a frame was once to the right of the viewport and also once, say, 2
frames below. Which bucket is it in? Both? In that case we'd be overcounting #
of frames that have to be loaded. If we only logged to one bucket, the one that
wins would depend on the order the frame related to the viewport. (If it's to
the right first it'll be in the Horizontal bucket, if it's 2screens below first
it'll be in the 2screens bucket.) Then when we are looking at just the vertical
data we'll be undercounting some.

In my experience, just the misunderstandings around this bit of confusion and
explaining whatever decision we make is enough to warrant doing something else,
not to mention the inaccurate data itself.

To sum up, I'm saying my preference is
(1) This patch as is
(2) Just vertical, ignore horizontal
(3) 3 vertical and 1 horizontal

Let me know your take.

Sorry for the novel.

[Z_DONOTUSE, 2016-12-01 00:47:13]

I'd prefer #2. I think the logic we'd use in lazy loading a frame would be based
off how far it is from the viewport in the scrolling direction because it's a
question of how much runway we need to preload them if the user scrolls quickly.
Users generally don't scroll diagonally. :)

[dgrogan, 2016-12-01 20:21:41]

Patchset #2 (id:20001) has been deleted

[dgrogan, 2016-12-01 20:34:25]

Description was changed from

==========
Cross-origin iframes: collect data under hypothetical loading strategies

Log % of frames to never load if we only loaded those that are
- visible, or
- 1 screen away
- 2 screens away
- 3 screens away

We would load frames used for cross-origin talk in any of the scenarios.

Note that these numbers are "manhattan distance" -- a frame two
screens below the fold and one to the right counts as three screens
away.

Also note that logging to one bucket will also log to each lower bucket,
which makes sense: if a frame has to be loaded under the "1 screen"
scenario, it would have to be loaded under a "3 screen" scenario.

This data will inform discussion of what our policy might eventually be.

BUG=635105
==========

to

==========
Cross-origin iframes: collect data under hypothetical loading strategies

Log % of frames to never load if we only loaded those that are
- visible, or
- 1 screen below
- 2 screens below
- 3 screens below

We would load frames used for cross-origin talk in any of the scenarios.

Note that we only look below for frames we might have to preload and ignore
frames to the right, assuming that the vast majority of content is
consumed vertically.

Also note that logging to one bucket will also log to each lower bucket,
which makes sense: if a frame has to be loaded under the "1 screen"
scenario, it would have to be loaded under a "3 screen" scenario.

This data will inform discussion of what our policy might eventually be.

BUG=635105
==========

[dgrogan, 2016-12-01 20:38:51]

Here's #2.

[Z_DONOTUSE, 2016-12-11 03:39:44]

ojan@google.com changed reviewers:
+ ojan@google.com

[Z_DONOTUSE, 2016-12-11 03:39:45]

https://codereview.chromium.org/2516473002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2516473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4598: parentRect =
grandParent->contentsToRootFrame(parentRect);
Unfortunately, I think this is n^2 in the frame tree because this walks up the
tree in the middle of updateViewportIntersectionsForSubtree, which is walking
down the tree. Instead, I think you need to pass the parentRect down as an
argument during the tree-walk down so that you don't then need to walk up here.

[dgrogan, 2016-12-20 22:22:05]

Patchset #4 (id:80001) has been deleted

[dgrogan, 2016-12-20 22:27:09]

Ready for another look.

https://codereview.chromium.org/2516473002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2516473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4598: parentRect =
grandParent->contentsToRootFrame(parentRect);
On 2016/12/11 03:39:44, Z_DONOTUSE wrote:
> Unfortunately, I think this is n^2 in the frame tree because this walks up the
> tree in the middle of updateViewportIntersectionsForSubtree, which is walking
> down the tree. Instead, I think you need to pass the parentRect down as an
> argument during the tree-walk down so that you don't then need to walk up
here.

I think it's nlgn in the common case but n^2 in the degenerate case? But
whatever, it's gone, with O(1) taking its place.

[ojan, 2017-01-18 00:55:01]

Sorry for the delayed response.

https://codereview.chromium.org/2516473002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2516473002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4656: int screensAway =
frameRect().y() / parentRect.height();
Does this need to take scroll position into account? If the user is scrolled
halfway down the page, we need to subtract that from the y() value I think?

Also, the parent isn't necessarily the root for the page, right? I think this
height value does the wrong thing for nested iframes? If I'm reading the
FrameView code right, I think x/y are in absolute coordinates for the whole
page.

So, I think there's two bugs here:
1. If a parent iframe is 2 screens away, and a nested iframe that's 2 screens
away, we'll still load the nested iframe even though it's >3 screens away from
the viewport.
2. If the user scrolls, we don't adjust values appropriately.

I think ultimately, what we want to know is whether this iframe is 1/2/3 screens
away, regardless of nesting, right?

[dgrogan, 2017-01-27 19:47:51]

On 2017/01/18 00:55:01, ojan wrote:
> Sorry for the delayed response.
> 
>
https://codereview.chromium.org/2516473002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2516473002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/FrameView.cpp:4656: int screensAway =
> frameRect().y() / parentRect.height();
> Does this need to take scroll position into account? If the user is scrolled
> halfway down the page, we need to subtract that from the y() value I think?
> 
> Also, the parent isn't necessarily the root for the page, right? I think this
> height value does the wrong thing for nested iframes? If I'm reading the
> FrameView code right, I think x/y are in absolute coordinates for the whole
> page.
> 
> So, I think there's two bugs here:
> 1. If a parent iframe is 2 screens away, and a nested iframe that's 2 screens
> away, we'll still load the nested iframe even though it's >3 screens away from
> the viewport.

You're right, change is in the works.

> 2. If the user scrolls, we don't adjust values appropriately.

Right again; this got lost in the switch to arithmetic testing. No tests FTL.

> 
> I think ultimately, what we want to know is whether this iframe is 1/2/3
screens
> away, regardless of nesting, right?

[dgrogan, 2017-02-03 02:44:12]

Patchset #9 (id:200001) has been deleted

[dgrogan, 2017-02-03 02:48:36]

Patchset #9 (id:220001) has been deleted

[dgrogan, 2017-02-05 03:20:28]

Patchset #9 (id:240001) has been deleted

[dgrogan, 2017-02-05 03:20:36]

Patchset #8 (id:180001) has been deleted

[dgrogan, 2017-02-05 03:21:00]

Patchset #7 (id:160001) has been deleted

[dgrogan, 2017-02-05 03:21:06]

Patchset #6 (id:140001) has been deleted

[dgrogan, 2017-02-05 03:26:18]

rebase

[dgrogan, 2017-02-05 03:36:18]

Patchset #7 (id:280001) has been deleted

[dgrogan, 2017-02-05 03:53:13]

ptal

This takes the parent distance into account when recording the children
distance. For your last example, where a parent iframe is 2 screens away, and a
nested iframe is 2 screens away, nothing would get logged for the nested iframe,
whereas we'd previously log "2screens".

But note that nested iframes use their parent's height to calculate how many
screens away they are. Using the viewport size all the way down leads to some
cases that don't make sense, like:

<body>
  <iframe style='height:10px'>
    <iframe style='top:15px'>

Using the viewport height (assuming it's taller than 15px) to determine the
innermost frame's distance would lead to logging Visible, which it's not. So
this patch uses the intermediate frame's height and logs "1screen".

[dgrogan, 2017-02-28 22:46:44]

ping ojan@

[ojan, 2017-03-01 00:58:26]

The CQ bit was checked by ojan@chromium.org

[ojan, 2017-03-01 00:58:27]

lgtm

[commit-bot: I haz the power, 2017-03-01 00:58:58]

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

[commit-bot: I haz the power, 2017-03-01 01:02:22]

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

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

Try jobs failed on following builders:
  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_...)

[dgrogan, 2017-03-01 01:25:19]

dgrogan@chromium.org changed reviewers:
+ isherman@chromium.org
- ojan@google.com

[dgrogan, 2017-03-01 01:25:19]

isherman, can you review histograms.xml?

[Ilya Sherman, 2017-03-01 01:33:26]

histograms.xml lgtm % a nit:

https://codereview.chromium.org/2516473002/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2516473002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:85977: +  <int value="5"
label="WouldVisible"/>
nit: Missing "Load" on this line?

[dgrogan, 2017-03-01 01:38:31]

https://codereview.chromium.org/2516473002/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2516473002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:29822: +    Deprecated 11/2016 in favor
of Navigation.DeferredDocumentLoading.StatesV4.
Also updated this.

https://codereview.chromium.org/2516473002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:85977: +  <int value="5"
label="WouldVisible"/>
On 2017/03/01 01:33:26, Ilya Sherman wrote:
> nit: Missing "Load" on this line?

D'oh, thanks.

[dgrogan, 2017-03-01 01:39:08]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2017-03-01 01:39:09]

The patchset sent to the CQ was uploaded after l-g-t-m from ojan@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2516473002/#ps340001
(title: "fix histograms.xml")

[commit-bot: I haz the power, 2017-03-01 01:39:39]

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

[commit-bot: I haz the power, 2017-03-01 04:01:12]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1488332348970590,
"parent_rev": "9b080e99c502e2d8674d14b59047fb3a23721d32", "commit_rev":
"86dfe7dc82290cede82086ad6e42ecaf54af4376"}

[commit-bot: I haz the power, 2017-03-01 04:01:49]

Description was changed from

==========
Cross-origin iframes: collect data under hypothetical loading strategies

Log % of frames to never load if we only loaded those that are
- visible, or
- 1 screen below
- 2 screens below
- 3 screens below

We would load frames used for cross-origin talk in any of the scenarios.

Note that we only look below for frames we might have to preload and ignore
frames to the right, assuming that the vast majority of content is
consumed vertically.

Also note that logging to one bucket will also log to each lower bucket,
which makes sense: if a frame has to be loaded under the "1 screen"
scenario, it would have to be loaded under a "3 screen" scenario.

This data will inform discussion of what our policy might eventually be.

BUG=635105
==========

to

==========
Cross-origin iframes: collect data under hypothetical loading strategies

Log % of frames to never load if we only loaded those that are
- visible, or
- 1 screen below
- 2 screens below
- 3 screens below

We would load frames used for cross-origin talk in any of the scenarios.

Note that we only look below for frames we might have to preload and ignore
frames to the right, assuming that the vast majority of content is
consumed vertically.

Also note that logging to one bucket will also log to each lower bucket,
which makes sense: if a frame has to be loaded under the "1 screen"
scenario, it would have to be loaded under a "3 screen" scenario.

This data will inform discussion of what our policy might eventually be.

BUG=635105

Review-Url: https://codereview.chromium.org/2516473002
Cr-Commit-Position: refs/heads/master@{#453837}
Committed:
https://chromium.googlesource.com/chromium/src/+/86dfe7dc82290cede82086ad6e42...
==========

[commit-bot: I haz the power, 2017-03-01 04:01:51]

Committed patchset #9 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/86dfe7dc82290cede82086ad6e42...