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

App history API to navigation API rename (1/n) #33132

Merged
merged 1 commit into from
Mar 14, 2022
Merged

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Mar 9, 2022

See WICG/navigation-api#83 and WICG/navigation-api#203 for context.

This CL implements the following renames:

  • window.appHistory → window.navigation
  • appHistory.current → navigation.currentEntry
  • appHistory.updateCurrent() → navigation.updateCurrentEntry()
  • appHistory currentchange event → navigation currententrychange event
  • appHistory.goTo() → navigation.traverseTo()

Notably, window.navigation has to be [Replaceable], as otherwise various internal tests that do var navigation = ... start failing.

No files are renamed at this point (including test files). No classes/IDL interfaces are renamed either.

Thus, the majority of this renaming is done in the web platform tests corpus, and to method names of the AppHistory C++ class.

Bug: 1300246
Change-Id: I29a01c6992c06a5d5099d831273ca8041281d557
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3510372
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#980648}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

See WICG/navigation-api#83 and WICG/navigation-api#203 for context.

This CL implements the following renames:

* window.appHistory → window.navigation
* appHistory.current → navigation.currentEntry
* appHistory.updateCurrent() → navigation.updateCurrentEntry()
* appHistory currentchange event → navigation currententrychange event
* appHistory.goTo() → navigation.traverseTo()

Notably, window.navigation has to be [Replaceable], as otherwise various internal tests that do `var navigation = ...` start failing.

No files are renamed at this point (including test files). No classes/IDL interfaces are renamed either.

Thus, the majority of this renaming is done in the web platform tests corpus, and to method names of the AppHistory C++ class.

Bug: 1300246
Change-Id: I29a01c6992c06a5d5099d831273ca8041281d557
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3510372
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#980648}
@WeizhongX
Copy link
Contributor

WPT Command: python3 ./wpt run --channel=nightly --verify --verify-no-chaos-mode --verify-repeat-loop=0 --verify-repeat-restart=10 --github-checks-text-file=/home/test/artifacts/checkrun.md --affected base_head --log-mach-level=info --log-mach=- -y --no-pause --no-restart-on-unexpected --install-fonts --no-headless --verify-log-full --binary=/home/test/build/firefox/firefox firefox

Some affected tests had inconsistent (flaky) results:

Unstable results

Test Subtest Results Messages
/app-history/navigate/navigate-replace-same-document.html   ERROR: 8/10, OK: 2/10  

These may be pre-existing or new flakes. Please try to reproduce (see the above WPT command, though some flags may not be needed when running locally) and determine if your change introduced the flake. If you are unable to reproduce the problem, please tag @web-platform-tests/wpt-core-team in a comment for help.

@WeizhongX
Copy link
Contributor

@past Pinged the owner on the chromium CL. Can you pls admin merge this?

There is a second CL depending on this. Now the exporter is red.

@WeizhongX
Copy link
Contributor

Per the author: Firefox does not implement this API, so any flakes there are do to that. It'd be best to just merge it anyways, regardless of Firefox flakes.

@past past merged commit 610b365 into master Mar 14, 2022
@past past deleted the chromium-export-cl-3510372 branch March 14, 2022 23:29
@past
Copy link
Member

past commented Mar 14, 2022

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants