Add an optional struct to BrowserNavigateParams for PWA-related fields. [chromium/src : main]

0 views
Skip to first unread message

Nate Chapin (Gerrit)

unread,
May 29, 2026, 6:03:12 PM (3 days ago) May 29
to Dibyajyoti Pal, chromium...@chromium.org, Chromium LUCI CQ, aixba+wat...@chromium.org, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, extension...@chromium.org, japhet+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Dibyajyoti Pal

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:
  • Dibyajyoti Pal
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ic49cc82afa2cc638d865e911e3de57206ed85824
Gerrit-Change-Number: 7885385
Gerrit-PatchSet: 6
Gerrit-Owner: Nate Chapin <jap...@chromium.org>
Gerrit-Reviewer: Dibyajyoti Pal <diby...@chromium.org>
Gerrit-Reviewer: Nate Chapin <jap...@chromium.org>
Gerrit-Attention: Dibyajyoti Pal <diby...@chromium.org>
Gerrit-Comment-Date: Fri, 29 May 2026 22:03:00 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Dibyajyoti Pal (Gerrit)

unread,
May 29, 2026, 6:06:17 PM (3 days ago) May 29
to Nate Chapin, chromium...@chromium.org, Chromium LUCI CQ, aixba+wat...@chromium.org, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, extension...@chromium.org, japhet+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Nate Chapin

Dibyajyoti Pal voted and added 1 comment

Votes added by Dibyajyoti Pal

Code-Review+1

1 comment

Patchset-level comments
File-level comment, Patchset 6 (Latest):
Dibyajyoti Pal . resolved

LGTM, thanks!

Open in Gerrit

Related details

Attention is currently required from:
  • Nate Chapin
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic49cc82afa2cc638d865e911e3de57206ed85824
    Gerrit-Change-Number: 7885385
    Gerrit-PatchSet: 6
    Gerrit-Owner: Nate Chapin <jap...@chromium.org>
    Gerrit-Reviewer: Dibyajyoti Pal <diby...@chromium.org>
    Gerrit-Reviewer: Nate Chapin <jap...@chromium.org>
    Gerrit-Attention: Nate Chapin <jap...@chromium.org>
    Gerrit-Comment-Date: Fri, 29 May 2026 22:06:00 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Dibyajyoti Pal (Gerrit)

    unread,
    May 29, 2026, 6:08:26 PM (3 days ago) May 29
    to Nate Chapin, chromium...@chromium.org, Chromium LUCI CQ, aixba+wat...@chromium.org, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, extension...@chromium.org, japhet+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
    Attention needed from Nate Chapin

    Dibyajyoti Pal added 1 comment

    File chrome/browser/extensions/api/tabs/tabs_api.cc
    Line 1256, Patchset 6 (Latest): navigate_params.web_app_navigation_data =
    Dibyajyoti Pal . unresolved

    AI review: This assignment completely overwrites the `web_app_navigation_data` that was populated inside `create_nav_params()` (which sets `navigation_capturing_force_off = true`), effectively losing that setting.

    Consider setting `launch_params` directly on the existing struct:
    ```cpp
          if (!navigate_params.web_app_navigation_data) {
            navigate_params.web_app_navigation_data = webapps::NavigationData();
          }
          navigate_params.web_app_navigation_data->launch_params = std::move(launch_params);
    ```
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Nate Chapin
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic49cc82afa2cc638d865e911e3de57206ed85824
      Gerrit-Change-Number: 7885385
      Gerrit-PatchSet: 6
      Gerrit-Owner: Nate Chapin <jap...@chromium.org>
      Gerrit-Reviewer: Dibyajyoti Pal <diby...@chromium.org>
      Gerrit-Reviewer: Nate Chapin <jap...@chromium.org>
      Gerrit-Attention: Nate Chapin <jap...@chromium.org>
      Gerrit-Comment-Date: Fri, 29 May 2026 22:08:07 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Nate Chapin (Gerrit)

      unread,
      2:39 PM (2 hours ago) 2:39 PM
      to Daniel Murphy, Thomas Lukaszewicz, chromium...@chromium.org, Chromium LUCI CQ, aixba+wat...@chromium.org, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, extension...@chromium.org, japhet+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
      Attention needed from Daniel Murphy and Thomas Lukaszewicz

      Nate Chapin added 2 comments

      Patchset-level comments
      File-level comment, Patchset 7 (Latest):
      Nate Chapin . resolved

      dmurph: please review components/webapps
      tluk: please review chrome/

      File chrome/browser/extensions/api/tabs/tabs_api.cc
      Line 1256, Patchset 6: navigate_params.web_app_navigation_data =
      Dibyajyoti Pal . resolved

      AI review: This assignment completely overwrites the `web_app_navigation_data` that was populated inside `create_nav_params()` (which sets `navigation_capturing_force_off = true`), effectively losing that setting.

      Consider setting `launch_params` directly on the existing struct:
      ```cpp
            if (!navigate_params.web_app_navigation_data) {
              navigate_params.web_app_navigation_data = webapps::NavigationData();
            }
            navigate_params.web_app_navigation_data->launch_params = std::move(launch_params);
      ```
      Nate Chapin

      In this case `create_nav_params()` guarantees that `web_app_navigation_data`, so I'm `CHECK`ing it rather than doing the conditional.

      Also updated web_app_launch_process.cc, since I couldn't immediately prove that it was safe from overwrite.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Daniel Murphy
      • Thomas Lukaszewicz
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement satisfiedReview-Enforcement
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Ic49cc82afa2cc638d865e911e3de57206ed85824
        Gerrit-Change-Number: 7885385
        Gerrit-PatchSet: 7
        Gerrit-Owner: Nate Chapin <jap...@chromium.org>
        Gerrit-Reviewer: Daniel Murphy <dmu...@chromium.org>
        Gerrit-Reviewer: Dibyajyoti Pal <diby...@chromium.org>
        Gerrit-Reviewer: Nate Chapin <jap...@chromium.org>
        Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
        Gerrit-Attention: Daniel Murphy <dmu...@chromium.org>
        Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
        Gerrit-Comment-Date: Mon, 01 Jun 2026 18:39:44 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        Comment-In-Reply-To: Dibyajyoti Pal <diby...@chromium.org>
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Daniel Murphy (Gerrit)

        unread,
        4:32 PM (21 minutes ago) 4:32 PM
        to Nate Chapin, Daniel Murphy, Thomas Lukaszewicz, chromium...@chromium.org, Chromium LUCI CQ, aixba+wat...@chromium.org, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, extension...@chromium.org, japhet+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
        Attention needed from Thomas Lukaszewicz

        Daniel Murphy voted and added 1 comment

        Votes added by Daniel Murphy

        Code-Review+1

        1 comment

        File components/webapps/browser/navigation_data.h
        Line 16, Patchset 7 (Latest):struct NavigationData {
        Daniel Murphy . unresolved

        nit: due to always regretting this, can you make this a class, and have setters & getters? it's annoying boilerplate, but really helps with future refactors.

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Thomas Lukaszewicz
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement is not satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement is not satisfiedNo-Unresolved-Comments
          • requirement satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Ic49cc82afa2cc638d865e911e3de57206ed85824
          Gerrit-Change-Number: 7885385
          Gerrit-PatchSet: 7
          Gerrit-Owner: Nate Chapin <jap...@chromium.org>
          Gerrit-Reviewer: Daniel Murphy <dmu...@chromium.org>
          Gerrit-Reviewer: Dibyajyoti Pal <diby...@chromium.org>
          Gerrit-Reviewer: Nate Chapin <jap...@chromium.org>
          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
          Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
          Gerrit-Comment-Date: Mon, 01 Jun 2026 20:32:12 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy
          Reply all
          Reply to author
          Forward
          0 new messages