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

Revert "Add WPT tests for feature policy" #9192

Merged
merged 1 commit into from Jan 27, 2018

Conversation

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

chromium-wpt-export-bot commented Jan 25, 2018

This reverts commit 6252427ab5415839618a0d25e4f6e61becce3923.

Reason for revert: Fails site_per_process_webkit_layout_tests

https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests/builds/66700

Original change's description:

Add WPT tests for feature policy

  1. Added tests for header policy.
    a. document.policy shows correctly parsed policy
    b. local / remote iframes without allow attribute correctly inherit
    document.policy
    c. dynamically update allow attribute updates the policy correctly.

  2. Added tests for nested policies.

Bug: 732003
Change-Id: I869449f6bba89fc58997355df27249f403d76808
Reviewed-on: https://chromium-review.googlesource.com/796952
Commit-Queue: Luna Lu loonybear@chromium.org
Reviewed-by: Ian Clelland iclelland@chromium.org
Cr-Commit-Position: refs/heads/master@{#531698}

TBR=iclelland@chromium.org,loonybear@chromium.org

Change-Id: I7b12b6809313f91df3f742e949439719adf8b867
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 732003
Reviewed-on: https://chromium-review.googlesource.com/885741
Reviewed-by: Giovanni Ortuño Urquidi ortuno@chromium.org
Commit-Queue: Giovanni Ortuño Urquidi ortuno@chromium.org
Cr-Commit-Position: refs/heads/master@{#531812}

Revert "Add WPT tests for feature policy"
This reverts commit 6252427ab5415839618a0d25e4f6e61becce3923.

Reason for revert: Fails site_per_process_webkit_layout_tests

https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests/builds/66700

Original change's description:
> Add WPT tests for feature policy
>
> 1. Added tests for header policy.
>     a. document.policy shows correctly parsed policy
>     b. local / remote iframes without allow attribute correctly inherit
>        document.policy
>     c. dynamically update allow attribute updates the policy correctly.
>
> 2. Added tests for nested policies.
>
> Bug: 732003
> Change-Id: I869449f6bba89fc58997355df27249f403d76808
> Reviewed-on: https://chromium-review.googlesource.com/796952
> Commit-Queue: Luna Lu <loonybear@chromium.org>
> Reviewed-by: Ian Clelland <iclelland@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#531698}

TBR=iclelland@chromium.org,loonybear@chromium.org

Change-Id: I7b12b6809313f91df3f742e949439719adf8b867
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 732003
Reviewed-on: https://chromium-review.googlesource.com/885741
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531812}
@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Jan 25, 2018

Build PASSED

Started: 2018-01-25 04:45:59
Finished: 2018-01-25 05:32:49

Failing Jobs

  • safari:11.0
  • MicrosoftEdge:14.14393

View more information about this build on:

@Ms2ger
Copy link
Contributor

Ms2ger left a comment

Why?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Jan 25, 2018

@wpt-pr-bot is down (#9195) and this would normally have been auto-approved and merged, as we don't do anything special for reverts. Last 10 reverts:

We haven't come up with anything that seems like it'd be an improvement, since it's possible the revert was due to a mistake in the test, causing it to flake or fail persistently. At most I think we could:

  • Not auto-merge reverts PRs like this and require review for them.
  • Manually review all reverts after the fact.

But I'd like to wait until we auto-create a mess of some sort until we add humans in the loop.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Jan 25, 2018

Why?

@Ms2ger reverted by a Chromium sheriff feature-policy/feature-policy-nested-header-policy.https.sub.html was failing on one configuration:

This is a testharness.js-based test.
FAIL Feature Policy nested test assert_true: Top-level>Remote-iframe>Remote-iframe: accelerometer enabled expected true got false
Harness: the test ran to completion.

Anyway, since we have humans in the loop now, we have at least two options:

  1. Land this revert and reland+reexport with the fix, which may or may not be in the tests.
  2. Close this PR so that the tests are imported back into Chromium, and somehow sort it out there.

I'd like to go with the first for simplicity. @clelland @loonybear, WDYT?

@wpt-pr-bot
Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Jan 26, 2018

@foolip generally, we shouldn't treat revert specially. Only full Chromium committers have the power to land a revert directly, and we trust they have legitimate reasons to do so. In almost all cases, reverts are investigated by the original authors, and the reverts will eventually be reverted ("Revert: Revert") if it's a false alarm, or a fixed version of the original revert will land (reland). And importer works fine with the whole process. The only downside is polluting the git history a bit...

Regarding this particular case, @clelland told me it might be a genuine bug in the implementation (interaction with site isolation). The plan was to split the CL into smaller ones and reland, one of which would contain the failing test (and supposedly a fix of the implementation or a temporary failure expectation). Does that make sense to you, @Ms2ger ?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Jan 27, 2018

I agree with not treating reverts specially and just accepting some churn. Since @Ms2ger hasn't responded and this would normally have been merged automatically, I'll merge it.

@foolip foolip merged commit 26330a4 into master Jan 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@foolip foolip deleted the chromium-export-2928474c39 branch Jan 27, 2018

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.