Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable ConsumeGestureOnNavigation by default #10200

Merged
merged 2 commits into from Apr 10, 2018

Conversation

Projects
None yet
6 participants
@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Mar 27, 2018

See blink-dev thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kPli8ZCUeok

A browser test is moved to be a tentative WPT due to this change.

Bug: 772515
Change-Id: Icf99c8c303c5055dcbcdace6ae94e3fcd1a01921
Reviewed-on: https://chromium-review.googlesource.com/980599
Reviewed-by: Nasko Oskov nasko@chromium.org
Reviewed-by: Mustaq Ahmed mustaq@chromium.org
Reviewed-by: Jonathon Kereliuk kereliuk@chromium.org
Commit-Queue: Charlie Harrison csharrison@chromium.org
Cr-Commit-Position: refs/heads/master@{#549293}


Revert "Enable ConsumeGestureOnNavigation by default"

This reverts commit 2ca250d409adfa73a666daabd3ba19b94d6443a7.

Reason for revert: A leak bot complains navigation-consumes-user-activation.tentative.sub.html is leaking.
Sample build:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linux%20Trusty%20Leak/builds/17509
Change-Id: I0c998798d1367be61c633db76429c18ac554e4ff
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 772515
Reviewed-on: https://chromium-review.googlesource.com/1003437
Reviewed-by: Makoto Shimazu shimazu@chromium.org
Commit-Queue: Makoto Shimazu shimazu@chromium.org
Cr-Commit-Position: refs/heads/master@{#549363}


This change is Reviewable

@wpt-pr-bot
Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 27, 2018

Build ERRORED

Started: 2018-03-27 22:31:40
Finished: 2018-03-27 22:39:23

Failing Jobs

  • chrome:dev

Unstable Results

Browser: "Chrome Dev"

View in: WPT PR Status | TravisCI

Test Subtest Results Messages
/html/browsers/browsing-the-web/navigating-across-documents/navigation-consumes-user-activation.tentative.sub.html   TIMEOUT: 1
OK: 9
  Navigations consume user activation TIMEOUT: 1
PASS: 9
Test timed out

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-980599 branch from 983104e to e6eb1a1 Apr 9, 2018

Enable ConsumeGestureOnNavigation by default
See blink-dev thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kPli8ZCUeok

A browser test is moved to be a tentative WPT due to this change.

Bug: 772515
Change-Id: Icf99c8c303c5055dcbcdace6ae94e3fcd1a01921
Reviewed-on: https://chromium-review.googlesource.com/980599
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Mustaq Ahmed <mustaq@chromium.org>
Reviewed-by: Jonathon Kereliuk <kereliuk@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549293}
@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Apr 10, 2018

This has already been reverted in Chromium, but I commented on the original review about the flakiness:
https://chromium-review.googlesource.com/c/chromium/src/+/980599#message-bffa3de13ffbe6053e79806252afb17d34b6b24b

@csharrison

This comment has been minimized.

Copy link
Contributor

csharrison commented Apr 10, 2018

@kereliuk hm I wonder if the timeout is due to races with the test instrumentation and the final top-level navigation this test does?

Does WPT have any slow endpoint that takes a while for a response to come back? Then we might be able to abort the navigation before it commits.

Revert "Enable ConsumeGestureOnNavigation by default"
This reverts commit 2ca250d409adfa73a666daabd3ba19b94d6443a7.

Reason for revert: A leak bot complains navigation-consumes-user-activation.tentative.sub.html is leaking.
Sample build:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linux%20Trusty%20Leak/builds/17509

Original change's description:
> Enable ConsumeGestureOnNavigation by default
>
> See blink-dev thread:
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kPli8ZCUeok
>
> A browser test is moved to be a tentative WPT due to this change.
>
> Bug: 772515
> Change-Id: Icf99c8c303c5055dcbcdace6ae94e3fcd1a01921
> Reviewed-on: https://chromium-review.googlesource.com/980599
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Mustaq Ahmed <mustaq@chromium.org>
> Reviewed-by: Jonathon Kereliuk <kereliuk@chromium.org>
> Commit-Queue: Charlie Harrison <csharrison@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#549293}

TBR=nasko@chromium.org,mustaq@chromium.org,csharrison@chromium.org,kereliuk@chromium.org

Change-Id: I0c998798d1367be61c633db76429c18ac554e4ff
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 772515
Reviewed-on: https://chromium-review.googlesource.com/1003437
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549363}
@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Apr 10, 2018

@csharrison it uses wptserve on the same machine as the tests are running, but it's possible the Travis VMs are unload load. Is there a way to make the test robust against those timing differences without making it test less than it should?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Apr 10, 2018

OK. To keep our export pipeline flowing I've manually put the revert commit on this as well and will rebase+merge both. This will be a no-op change which is a bit silly, but we currently don't have a way to mark Chromium commits as not needing export once they've landed, and we'd otherwise keep trying to export the revert which would never apply.

@foolip foolip merged commit 1e801bf into master Apr 10, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@foolip foolip deleted the chromium-export-cl-980599 branch Apr 10, 2018

@csharrison

This comment has been minimized.

Copy link
Contributor

csharrison commented Apr 10, 2018

Thanks @foolip. Can you clarify what you mean by " it's possible the Travis VMs are unload load"? I was thinking of making the test navigate to some endpoint that would delay for arbitrary lengths of time (e.g. a minute), but then immediately abort the navigation (via window.stop).

I think the issue comes from the fact that navigating away from the document under test is confusing to the wpt test harness (and potentially also to the leak detector).

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Apr 11, 2018

I mean that if the test assumes that some operation will be fast, or the network snappy, then that might not be true in testing infrastructure like Travis.

I think the issue comes from the fact that navigating away from the document under test is confusing to the wpt test harness (and potentially also to the leak detector).

Hmm, actually sounds a bit like #10186. @jugglinmike, another category of things that might go wrong for your collection.

@csharrison, can the test be written to do the navigation in a new window, opened with window.open()?

@csharrison

This comment has been minimized.

Copy link
Contributor

csharrison commented Apr 13, 2018

@foolip I don't think it would work, the feature only governs navigations in the current main frame. Additionally, any API that ends up creating a new window should consume a user gesture / user activation anyway in Chrome, so we wouldn't really be testing the feature.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Apr 18, 2018

Hmm, OK. This didn't land, so I guess the answer to the stability questions can wait until it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.