dominikg@ no longer work on Chromium project. I think we can just go ahead and update scroll action to use window.outerHeight.

Ulan: can you take this bug?

+gpu & input: FYI since this may regress some of the scrolling benchmarks.

Ned, who would be the most knowledgeable besides dominikg@ in synthetic scrolling? Maybe input folks?

I can change code to use indow.outerHeight, but I have no idea what I am doing :) So getting approval from an expert would be great.

Tim & Victor can provide guidance here :)

bokan@, I seem to remember you having some thoughts here?

Yah, I suggested to ulan@ to use outerWidth/Height since it looks like we really want "screen coordinates" (i.e. they shouldn't change when you zoom in). I'm happy to provide code reviews and advice but I only know the synthetic input paths at a cursory level.

I run some experiments with benchmarks.

For all existing benchmarks: innerWidth/Height <= outerWidth/Height. In this case I am not sure that we want to use outerWidth/Height because we will be adding window borders to scroll amount. With outerWidth/Height the benchmarks scroll further and throw errors at the end because WPR doesn't have data. So we would need to re-record all scrolling benchmarks if we go with outerWidth/Height.


I propose taking min(outerWidth/Height, innerWidth/Height), which will leave existing benchmarks unchanged and fix the corner case. wdyt?

I think min(outer, inner) is confusing and will make debugging harder. Additionally, if the page starts off zoomed in for some reason (e.g. viewport <meta> with minimum-scale=2 you'll run into the same problem). Might be a bit of a pain but it sounds like the right thing to do would be to re-record scrolling benchmarks.

How about we use window.devicePixelRatio to offset the zoom? From my local experiments, it seems that the following values are constant (within floating point errors) when zooming in and out:

window.innerHeight * (window.devicePixelRatio || 1)
window.innerWidth * (window.devicePixelRatio || 1)

This (devicePixelRatio↑) actually seems like a more stable approach than taking the min of outer and inner width/height.

Are we sure that this would cause failure on some exiting scrolling benchmarks because telemetry throw errors at the end of scrolling when WPR doesn't have data? I can't think why telemetry would throw exception there.

devicePixelRatio doesn't change with pinch-zoom, I'm assuming you were doing ctrl+/- zoom? That works quite differently from pinch zoom.

If you can use private APIs, what you might be able to do is use GpuBenchmarkingExtensions::pageScaleFactor which gives you the amount of scale due to pinch zoom. in that case, the "original" (unzoomed) innerHeight would be window.innerHeight * pageScaleFactor. You'd have to be careful about floating point though if 1 px differences are important.

> Are we sure that this would cause failure on some exiting scrolling benchmarks because telemetry throw errors at the end of scrolling when WPR doesn't have data? I can't think why telemetry would throw exception there.
JS code in benchmark starts throwing exceptions. I don't think we want to benchmark and optimize for error paths.

If I understand correctly, zoom is not the only source of the problem.
The problem also occurs when page layout is incomplete during page load? Would pageScaleFactor help in that case too?

You might get some transient values as the page loads (as the page content gets more and more width, we'll keep zooming out) but innerWidth|Height * pageScaleFactor will remain constants.

That's good news. Using pageScaleFactor seems cleaner solution to me.

It was added three months ago https://codereview.chromium.org/1866793003/, so it should be in Chrome 51.

The reference build in catapult is Chrome 49. I can wait until the reference build is updated and then land proper fix.

Ulan: you can land the proper fix & add "@Disabled('reference') # Remove this when reference build updated" to the test to unblock the fix :-)

I would have to disable reference for all benchmarks that use scroll action (proper fix will change scroll action). That is probably not ideal?

I could also add runtime check:
if (chrome.gpuBenchmarking.pageScaleFactor !== undefined) {properfix}
else {old code}.
but it increases code complexity.

Oh right. The runtime check you mentioned sounds like a good bandaid fix for now :-(

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c451c080637ae63b1335b610122ca78697ed2bf3

commit c451c080637ae63b1335b610122ca78697ed2bf3
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Mon Jul 18 12:54:05 2016

Roll src/third_party/catapult/ 8ae70d714..d2b666834 (1 commit).

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/8ae70d714551..d2b666834a28

$ git log 8ae70d714..d2b666834 --date=short --no-merges --format='%ad %ae %s'

BUG= 627166 

TBR=catapult-sheriff@chromium.org

Review-Url: https://codereview.chromium.org/2157923002
Cr-Commit-Position: refs/heads/master@{#405981}

[modify] https://crrev.com/c451c080637ae63b1335b610122ca78697ed2bf3/DEPS

browse:news:qq claims to be disabled on Android due to this bug. Can that page be re-enabled again?

https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/browsing_stories.py?q=browse:news:qq&sq=package:chromium&dr=C&l=160

#23: I think so. If not, then a new bug should be filed.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7e9a61a40dbe068092704d6c9f9c2fe9a1d12e1d

commit 7e9a61a40dbe068092704d6c9f9c2fe9a1d12e1d
Author: nednguyen <nednguyen@google.com>
Date: Mon Oct 03 14:30:02 2016

[tools/perf] Reenable qq mobile story

BUG= 627166 

Review-Url: https://codereview.chromium.org/2385163002
Cr-Commit-Position: refs/heads/master@{#422417}

[modify] https://crrev.com/7e9a61a40dbe068092704d6c9f9c2fe9a1d12e1d/tools/perf/page_sets/system_health/browsing_stories.py