Add WPT tests for Prefetch Activation Beacon API [chromium/src : main]

0 views

Skip to first unread message

Hiroki Nakagawa (Gerrit)

unread,

5:42 AM (10 hours ago) 5:42 AM

to Jiacheng Guo, Chromium Metrics Reviews, Chromium LUCI CQ, chromium...@chromium.org, prerendering-reviews, asvitki...@chromium.org, jmedle...@chromium.org, blink-rev...@chromium.org, asvitkine...@chromium.org, blink-revie...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, crmulli...@chromium.org, feature-me...@chromium.org, gavin...@chromium.org, ipc-securi...@chromium.org, jorgel...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, navigation...@chromium.org, network-ser...@chromium.org, nicolas...@chromium.org, ramyagopa...@google.com, tburkar...@chromium.org

Attention needed from Jiacheng Guo

Hiroki Nakagawa added 10 comments

Patchset-level comments

File-level comment, Patchset 22 (Latest):

Hiroki Nakagawa . resolved

Sorry for the late review.

File third_party/blink/web_tests/external/wpt/speculation-rules/prefetch/prefetch-activation-beacon.tentative.https.html

Line 14, Patchset 22 (Latest): let uuid = token();

Hiroki Nakagawa . unresolved

Can we use `const` instead of `let` by default?

File third_party/blink/web_tests/external/wpt/speculation-rules/prefetch/prerender-activation-beacon.tentative.https.html

Line 1, Patchset 22 (Latest):<!DOCTYPE html>

Hiroki Nakagawa . unresolved

Can we move these on-prefetch-activation tests into its own directory (e.g., `speculation-rules/activation-header/`)?

This test is now under speculation-rules/prefetch/, while this is for prerendering. Moving the tests into the new directory would resolve the inconsistency.
Other browser vendors can more easily disable all the tests for activation header.

Line 14, Patchset 22 (Latest): let uuid = token();

Hiroki Nakagawa . unresolved

s/let/const/

Line 25, Patchset 22 (Latest): prerender: [{source: 'list', urls: [url]}]

Hiroki Nakagawa . unresolved

The `source` param is deprecated. Can you remove this?

Line 70, Patchset 22 (Latest): prerender: [{source: 'list', urls: [url]}]

Hiroki Nakagawa . unresolved

Ditto: `source` is deprecated.

File third_party/blink/web_tests/external/wpt/speculation-rules/prefetch/resources/activation_beacon.py

Line 15, Patchset 22 (Latest): pass

Hiroki Nakagawa . unresolved

Is this `pass` necessary?

Line 27, Patchset 22 (Latest):

        log = state.get("log", [])
        log.append(
            f"{request.method.decode('utf-8') if isinstance(request.method, bytes) else request.method} {action_str}"
        )
        state["log"] = log

Hiroki Nakagawa . unresolved

Is this used?

Line 74, Patchset 22 (Latest): pass

Hiroki Nakagawa . unresolved

Ditto.

File third_party/blink/web_tests/wpt_internal/speculation-rules/prefetch/resources/executor-non-immediate.html

Line 105, Patchset 22 (Latest):const discard_uuid = params.get('discard_uuid') || uuid;

Hiroki Nakagawa . unresolved

Is `discard_uuid` used in this CL?

Open in Gerrit

Related details

Attention is currently required from:

Jiacheng Guo

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages