Skip to content

[Tabs]: Only bring tabs from normal windows #24995

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 6, 2024
Merged

[Tabs]: Only bring tabs from normal windows #24995

merged 1 commit into from
Aug 6, 2024

Conversation

fallaciousreasoning
Copy link
Contributor

@fallaciousreasoning fallaciousreasoning commented Aug 6, 2024 •
edited by bsclifton
Loading

Resolves brave/brave-browser#40287

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

See issue

@fallaciousreasoning fallaciousreasoning changed the title [Tabs]: Only bring tabs from NORMAL windows [Tabs]: Only bring tabs from normal windows Aug 6, 2024
sangwoo108
sangwoo108 approved these changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@sangwoo108 sangwoo108 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thnak you so much!

bsclifton
bsclifton approved these changes Aug 6, 2024
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

LGTM!

browser/ui/browser_commands.cc
return from != to && !from->IsAttemptingToCloseBrowser() &&
!from->IsBrowserClosing() && !from->is_delete_scheduled() &&
to->profile() == from->profile();
return from != to && from->type() == Browser::TYPE_NORMAL &&
Copy link
Member

Choose a reason for hiding this comment

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

Nice fix! Had to look up the other types here:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser.h;l=157-190;drc=41374c974d98f8cf67134f9ddb8d96d398154dfe

We likely also had a bug if someone popped devtools out to a separate window or an extension popup is being shown. This should fix it properly 😄

Copy link
Member

Choose a reason for hiding this comment

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

Update: yeah I confirmed before this fix, it would pull the devtools into the main window as a tab 😂

@bsclifton bsclifton enabled auto-merge August 6, 2024 01:08
@bsclifton bsclifton merged commit 69ab067 into master Aug 6, 2024
22 checks passed
@bsclifton bsclifton deleted the pwas-to-browser branch August 6, 2024 01:50
@github-actions github-actions bot added this to the 1.70.x - Nightly milestone Aug 6, 2024
brave-builds added a commit that referenced this pull request Aug 6, 2024
@brave-builds
Uplift of #24995 (squashed) to beta
0d8e66a
@brave-builds brave-builds mentioned this pull request Aug 6, 2024
Merged
7 tasks
@kjozwiak
Copy link
Member

kjozwiak commented Aug 6, 2024

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.70.53 Chromium: 127.0.6533.88 (Official Build) nightly (64-bit)
-- | --
Revision | 83a4be99f404ccc245c4f5edc4690baea5beb31c
OS | Windows 11 Version 23H2 (Build 22631.3958)

Using 1.70.51 Chromium: 127.0.6533.88 and the STR/Cases mentioned via brave/brave-browser#40287 (comment), reproduced the issue regarding a PWA window being moved over into the target window when using Bring all tabs to this window as per the following:

reproducedIssue.mp4

Using the same STR/Cases as mentioned above, verified that PWA apps are not being moved when using Bring all tabs to this window with 1.70.53 Chromium: 127.0.6533.88 as per the following:

fixedPWA.mp4

kjozwiak pushed a commit that referenced this pull request Aug 6, 2024
@brave-builds
[Tabs]: Only bring tabs from normal windows (uplift to 1.69.x) (#24996)
3c702db 
Uplift of #24995 (squashed) to beta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsclifton bsclifton bsclifton approved these changes

@sangwoo108 sangwoo108 sangwoo108 approved these changes

Assignees

@fallaciousreasoning fallaciousreasoning

Labels
None yet
Projects
None yet
Milestone
1.70.x - Release
Development

Successfully merging this pull request may close these issues.

"Bring all tabs to this window" unexpectedly brings PWAs into the window as tabs
4 participants
@fallaciousreasoning @kjozwiak @bsclifton @sangwoo108