Fix the escape from auth pop up by minimize un-authorized window. [chromium/src : main]

0 views
Skip to first unread message

April Zhou (Gerrit)

unread,
May 14, 2026, 2:08:29 PMMay 14
to Vignesh Shenvi, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, devtools...@chromium.org, cblume...@chromium.org, oshima...@chromium.org
Attention needed from Vignesh Shenvi

April Zhou added 4 comments

File chrome/browser/ash/boca/on_task/on_task_locked_session_navigation_throttle_interactive_ui_test.cc
Line 961, Patchset 13: ui_test_utils::BrowserDestroyedObserver unauthorized_popup_closed_observer(
unauthorized_popup);
Vignesh Shenvi . resolved

Do we know for sure that the unauthorized popup is not destroyed before this observer is set up? I am worried that this will otherwise introduce a race resulting in test flakiness in some environments. Same with the next test below.

April Zhou

Make sense, move the setup up to make it deterministic

Line 968, Patchset 13: // The authorized oauth popup should still be open.
EXPECT_EQ(chrome::GetTotalBrowserCount(), original_browser_count + 1);
Vignesh Shenvi . resolved

Would it make more sense to explicitly check for `popup_browser` being open instead? Same with the next test below.

April Zhou

Done

File chrome/browser/ash/boca/on_task/on_task_locked_session_window_tracker.h
Line 99, Patchset 13: ash::BrowserDelegate* browser = nullptr);
Vignesh Shenvi . resolved

Can we force this param on all callers? It will help to uncover edge cases.

April Zhou

Done

File chrome/browser/ash/boca/on_task/on_task_locked_session_window_tracker.cc
Line 116, Patchset 13: if (in_progress && browser &&
Vignesh Shenvi . resolved

When will the `browser` be nullptr?

April Zhou

This is to handle any race condition that caused the browser window to be closed before this execution - manually or by other process so that we only track valid window

Open in Gerrit

Related details

Attention is currently required from:
  • Vignesh Shenvi
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement 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: I724d8f364fd01f9c70a54d55b62d6eb075bf0524
Gerrit-Change-Number: 7744643
Gerrit-PatchSet: 15
Gerrit-Owner: April Zhou <apri...@google.com>
Gerrit-Reviewer: April Zhou <apri...@google.com>
Gerrit-Reviewer: Vignesh Shenvi <vsh...@google.com>
Gerrit-Attention: Vignesh Shenvi <vsh...@google.com>
Gerrit-Comment-Date: Thu, 14 May 2026 18:07:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Vignesh Shenvi <vsh...@google.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Vignesh Shenvi (Gerrit)

unread,
May 14, 2026, 5:59:12 PMMay 14
to April Zhou, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, devtools...@chromium.org, cblume...@chromium.org, oshima...@chromium.org
Attention needed from April Zhou

Vignesh Shenvi voted and added 3 comments

Votes added by Vignesh Shenvi

Code-Review+1

3 comments

Patchset-level comments
File-level comment, Patchset 15 (Latest):
Vignesh Shenvi . resolved

Thanks for fixing this, April! LGTM % minor test nits.

File chrome/browser/ash/boca/on_task/on_task_locked_session_navigation_throttle_interactive_ui_test.cc
Line 876, Patchset 15 (Latest): EXPECT_FALSE(popup_browser->is_delete_scheduled());
Vignesh Shenvi . unresolved

[nit] `ASSERT_FALSE` so we fail fast? :)

Line 1140, Patchset 15 (Latest): EXPECT_FALSE(popup_browser->is_delete_scheduled());
Vignesh Shenvi . unresolved

[nit] Same here about the `ASSERT_FALSE` so we fail fast.

Open in Gerrit

Related details

Attention is currently required from:
  • April Zhou
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement 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: I724d8f364fd01f9c70a54d55b62d6eb075bf0524
    Gerrit-Change-Number: 7744643
    Gerrit-PatchSet: 15
    Gerrit-Owner: April Zhou <apri...@google.com>
    Gerrit-Reviewer: April Zhou <apri...@google.com>
    Gerrit-Reviewer: Vignesh Shenvi <vsh...@google.com>
    Gerrit-Attention: April Zhou <apri...@google.com>
    Gerrit-Comment-Date: Thu, 14 May 2026 21:58:59 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    April Zhou (Gerrit)

    unread,
    May 14, 2026, 6:01:32 PMMay 14
    to Vignesh Shenvi, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, devtools...@chromium.org, cblume...@chromium.org, oshima...@chromium.org

    April Zhou added 2 comments

    File chrome/browser/ash/boca/on_task/on_task_locked_session_navigation_throttle_interactive_ui_test.cc
    Line 876, Patchset 15 (Latest): EXPECT_FALSE(popup_browser->is_delete_scheduled());
    Vignesh Shenvi . resolved

    [nit] `ASSERT_FALSE` so we fail fast? :)

    April Zhou

    I prefer to use ASSERT only when the following test case depends on the current assertion, otherwise using EXPECT would allow us to see more failure at once. Keep it as it is, let me know if any concerns.

    Line 1140, Patchset 15 (Latest): EXPECT_FALSE(popup_browser->is_delete_scheduled());
    Vignesh Shenvi . resolved

    [nit] Same here about the `ASSERT_FALSE` so we fail fast.

    April Zhou

    Ditto

    Open in Gerrit

    Related details

    Attention set is empty
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement 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: I724d8f364fd01f9c70a54d55b62d6eb075bf0524
      Gerrit-Change-Number: 7744643
      Gerrit-PatchSet: 15
      Gerrit-Owner: April Zhou <apri...@google.com>
      Gerrit-Reviewer: April Zhou <apri...@google.com>
      Gerrit-Reviewer: Vignesh Shenvi <vsh...@google.com>
      Gerrit-Comment-Date: Thu, 14 May 2026 22:01:19 +0000
      satisfied_requirement
      open
      diffy

      April Zhou (Gerrit)

      unread,
      May 14, 2026, 6:01:36 PMMay 14
      to Vignesh Shenvi, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, devtools...@chromium.org, cblume...@chromium.org, oshima...@chromium.org

      April Zhou voted Commit-Queue+2

      Commit-Queue+2
      Open in Gerrit

      Related details

      Attention set is empty
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement 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: I724d8f364fd01f9c70a54d55b62d6eb075bf0524
      Gerrit-Change-Number: 7744643
      Gerrit-PatchSet: 15
      Gerrit-Owner: April Zhou <apri...@google.com>
      Gerrit-Reviewer: April Zhou <apri...@google.com>
      Gerrit-Reviewer: Vignesh Shenvi <vsh...@google.com>
      Gerrit-Comment-Date: Thu, 14 May 2026 22:01:23 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      open
      diffy

      Chromium LUCI CQ (Gerrit)

      unread,
      May 14, 2026, 6:04:30 PMMay 14
      to April Zhou, Vignesh Shenvi, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, devtools...@chromium.org, cblume...@chromium.org, oshima...@chromium.org

      Chromium LUCI CQ submitted the change

      Change information

      Commit message:
      Fix the escape from auth pop up by minimize un-authorized window.
      Change-Id: I724d8f364fd01f9c70a54d55b62d6eb075bf0524
      Reviewed-by: Vignesh Shenvi <vsh...@google.com>
      Commit-Queue: April Zhou <apri...@google.com>
      Cr-Commit-Position: refs/heads/main@{#1630787}
      Files:
      • M chrome/browser/ash/boca/DEPS
      • M chrome/browser/ash/boca/on_task/on_task_locked_session_navigation_throttle.cc
      • M chrome/browser/ash/boca/on_task/on_task_locked_session_navigation_throttle_interactive_ui_test.cc
      • M chrome/browser/ash/boca/on_task/on_task_locked_session_window_tracker.cc
      • M chrome/browser/ash/boca/on_task/on_task_locked_session_window_tracker.h
      Change size: L
      Delta: 5 files changed, 241 insertions(+), 25 deletions(-)
      Branch: refs/heads/main
      Submit Requirements:
      • requirement satisfiedCode-Review: +1 by Vignesh Shenvi
      Open in Gerrit
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: merged
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I724d8f364fd01f9c70a54d55b62d6eb075bf0524
      Gerrit-Change-Number: 7744643
      Gerrit-PatchSet: 16
      Gerrit-Owner: April Zhou <apri...@google.com>
      Gerrit-Reviewer: April Zhou <apri...@google.com>
      open
      diffy
      satisfied_requirement
      Reply all
      Reply to author
      Forward
      0 new messages