[<install> element] Update attributes to installurl and manifest id [chromium/src : main]

0 views

Skip to first unread message

Lia Hiscock (Gerrit)

unread,

Dec 11, 2025, 1:43:34 PM (yesterday) Dec 11

to Lu Huang, Kristin Lee, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Permissions Reviews, loyso...@chromium.org, jmedle...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, blink-rev...@chromium.org, kuragin+web-ap...@chromium.org, zelin+watch-we...@chromium.org, philli...@chromium.org, webap...@microsoft.com, mgiuca...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, ipc-securi...@chromium.org, kinuko...@chromium.org

Attention needed from Kristin Lee and Lu Huang

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:

Kristin Lee
Lu Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Kristin Lee (Gerrit)

unread,

7:18 PM (1 hour ago) 7:18 PM

to Lia Hiscock, Lu Huang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Permissions Reviews, loyso...@chromium.org, jmedle...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, blink-rev...@chromium.org, kuragin+web-ap...@chromium.org, zelin+watch-we...@chromium.org, philli...@chromium.org, webap...@microsoft.com, mgiuca...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, ipc-securi...@chromium.org, kinuko...@chromium.org

Attention needed from Lia Hiscock and Lu Huang

Kristin Lee added 2 comments

File chrome/browser/web_applications/install_element_browsertest.cc

Line 190, Patchset 13 (Latest):

  // The manifest of this test page does not contain an `id`, so we must set it
  // on the element. It's the same as the install_url because the start_url is
  // set to manifest_test_page.html.

Kristin Lee . unresolved

Suggestion - Consider using this test page instead where the install_url is different from the manifest_id: https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/data/web_apps/install_url/?q=install_url%20test%2Fdata&ss=chromium

In a previous comment from Dan, he pointed out install url should be different than the start_url and manifest_id to prevent potential bugs and from the logic potentially mistaking one for the other, so it's possible he might suggest that again: https://chromium-review.googlesource.com/c/chromium/src/+/7006552/comment/5e35d593_2c76fd45/
Here's my CL where I updated our test sites to his suggestion: https://chromium-review.googlesource.com/c/chromium/src/+/7233302/3/chrome/browser/web_applications/commands/web_install_from_url_command_browsertest.cc

We could also consider updating the start_urls on the new test pages created in this CL so they're different from the install_url and manifest_id too.

File chrome/test/data/banners/install_element.html

Line 4, Patchset 13 (Latest): <link rel="manifest" href="manifest_for_install_element.json">

Kristin Lee . unresolved

nit: For all the new test pages created, here's another potential location for them to consider: `chrome/test/data/web_apps/`