Pin Brave to taskbar from FirstRun dialog #14311

simonhong · 2022-07-25T03:06:47Z

When user set Brave as a default browser from FirstRun dialog,
try to pin it to taskbar also.

Resolves brave/brave-browser#24241

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
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, npm run lint, 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:

Do clean install and launch Brave
Accept seting as a default browser and check it's pinned to taskbar
NOTE: This pin feature is supported from Win10 RS5.

sangwoo108 · 2022-07-27T04:55:20Z

browser/brave_shell_integration_win.cc

+  if (!base::PathService::Get(base::FILE_EXE, &chrome_exe))
+    return;
+
+  ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER);


Q. If we call properties.set_pin_to_taskbar(true), then ShellUtil::PinShortcut() will be called. https://source.chromium.org/chromium/chromium/src/+/main:chrome/installer/util/shell_util.h;l=865

To me, Chromium's implementation looks quite similar to FF's implementation but more modern as they're using ComPtr(RAII obj?). Doesn't Chromium's implementation work and is it why you brought implementation from FF?

Q. If we call properties.set_pin_to_taskbar(true), then ShellUtil::PinShortcut() will be called. https://source.chromium.org/chromium/chromium/src/+/main:chrome/installer/util/shell_util.h;l=865

properties.set_pin_to_taskbar(true) will not work for Win10 and later version.
AFAIK, taskbarpin is disabled by MS from Win10.

bool CanPinShortcutToTaskbar() { // "Pin to taskbar" stopped being supported in Windows 10. return GetVersion() < Version::WIN10; } bool PinShortcutToTaskbar(const FilePath& shortcut) { ScopedBlockingCall scoped_blocking_call(FROM_HERE, BlockingType::MAY_BLOCK); DCHECK(CanPinShortcutToTaskbar()); intptr_t result = reinterpret_cast<intptr_t>(ShellExecute( nullptr, L"taskbarpin", shortcut.value().c_str(), nullptr, nullptr, 0)); return result > 32; }

To me, Chromium's implementation looks quite similar to FF's implementation but more modern as they're using ComPtr(RAII obj?). Doesn't Chromium's implementation work and is it why you brought implementation from FF?

Hmm, it's not yet available for our master. Should we copy and test in advance?

Trying to test chromium's method.

Oh, there's another implementation in base::win::shortcut.cc , which uses "ShellExecute()" . Maybe this has been stopped working from Win10.

Hmm, it's not yet available for our master

Aha, What I found was the latest one and we don't have it yet. Thanks.

sangwoo108 · 2022-07-27T05:53:06Z

browser/brave_shell_integration_win.cc

+  if (!base::PathService::Get(base::FILE_EXE, &chrome_exe))
+    return;
+
+  ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER);


Oh, there's another implementation in base::win::shortcut.cc , which uses "ShellExecute()" . Maybe this has been stopped working from Win10.

Hmm, it's not yet available for our master

Aha, What I found was the latest one and we don't have it yet. Thanks.

sangwoo108 · 2022-07-27T05:58:14Z

browser/brave_shell_integration_win.cc

+  // See PinCurrentAppToTaskbarAsyncImpl() at
+  // https://github.com/mozilla/gecko-dev/blob/master/browser/components/shell/nsWindowsShellService.cpp
+  if (base::win::GetVersion() < base::win::Version::WIN10_RS5)
+    return;


Then, can we try something like this as fallback?

Suggested change

return;

if (base::win::CanPinShortcut()) {

...// call ShellExecute version base::win::PinShortcutToTaskbar() in callback

}

return;

Test with upstream methods (IsPinned/Pin) works nicely. Copied them used.
We can delete copied one when it's available.

base::win::CanPinShortcutToTaskbar() uses different condition with this one.
base::win::GetVersion() < base::win::Version::WIN10 vs.
base::win::GetVersion() >= base::win::Version::WIN10_RS5

It seems IPinnedList3 is availble from base::win::Version::WIN10_RS5

Yeah, so what i mean is

if (base::win::GetVersion() >= WIN10_RS5) PinTaskBar with IPinnedList3 - ShellUtill's implemntation else if (base::win::GetVersion() < base::win::Version::WIN10) PinTaskBar with ShellExecute() - Shorcut's implementation.

so that we can cover more versions?

Ok, I see what you mean. How about handling it as a next step?
As this PR is for pinning from first run dialog, we don't need to care about win7/8.
The reason is windows installer tries to pin to taskbar and it works on win7/8. So, don't need to try to pin again on Win7/8. I planned to handle it as a next task for pinning when user set Brave as a default from settings menu.

Oh, sure! Fair enough. Didn't know that 7/8 already works. You can skip it.

@sangwoo108 Thanks for review! 👍🏼

When user set Brave as a default browser from FirstRun dialog, try to pin it to taskbar also. Resolves brave/brave-browser#24241

sangwoo108

LGTM!

fix brave/brave-browser#24363 This is f/u PR for #14311. I accidently omitted calling pin method while rebasing from above PR.

simonhong self-assigned this Jul 25, 2022

simonhong force-pushed the pin_to_taskbar_from_first_run_win branch 2 times, most recently from 05514ff to 2ef34d3 Compare July 25, 2022 04:50

simonhong changed the title ~~WIP: Pin Brave to taskbar from FirstRun dialog~~ Pin Brave to taskbar from FirstRun dialog Jul 25, 2022

simonhong marked this pull request as ready for review July 25, 2022 05:01

simonhong force-pushed the pin_to_taskbar_from_first_run_win branch from 2ef34d3 to ecaaa9d Compare July 25, 2022 05:02

simonhong requested a review from goodov July 25, 2022 11:57

simonhong force-pushed the pin_to_taskbar_from_first_run_win branch from ecaaa9d to 4962476 Compare July 26, 2022 03:06

simonhong requested a review from sangwoo108 July 27, 2022 01:34

sangwoo108 reviewed Jul 27, 2022

View reviewed changes

simonhong force-pushed the pin_to_taskbar_from_first_run_win branch from 4962476 to 09340f4 Compare July 27, 2022 06:05

Pin Brave to taskbar from FirstRun dialog

e2c2872

When user set Brave as a default browser from FirstRun dialog, try to pin it to taskbar also. Resolves brave/brave-browser#24241

simonhong force-pushed the pin_to_taskbar_from_first_run_win branch from 09340f4 to e2c2872 Compare July 27, 2022 06:17

sangwoo108 approved these changes Jul 27, 2022

View reviewed changes

simonhong merged commit e272084 into master Jul 27, 2022

simonhong deleted the pin_to_taskbar_from_first_run_win branch July 27, 2022 11:57

simonhong added this to the 1.44.x - Nightly milestone Jul 27, 2022

simonhong added a commit that referenced this pull request Aug 8, 2022

Run pin to task method from first run dialog

d735e2c

fix brave/brave-browser#24363 This is f/u PR for #14311. I accidently omitted calling pin method while rebasing from above PR.

simonhong mentioned this pull request Aug 8, 2022

Run pin to task method from first run dialog #14520

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pin Brave to taskbar from FirstRun dialog #14311

Pin Brave to taskbar from FirstRun dialog #14311

simonhong commented Jul 25, 2022 •

edited

Loading

sangwoo108 Jul 27, 2022

simonhong Jul 27, 2022

simonhong Jul 27, 2022

sangwoo108 Jul 27, 2022

sangwoo108 Jul 27, 2022

sangwoo108 Jul 27, 2022 •

edited

Loading

simonhong Jul 27, 2022

simonhong Jul 27, 2022 •

edited

Loading

sangwoo108 Jul 27, 2022

simonhong Jul 27, 2022

sangwoo108 Jul 27, 2022 •

edited

Loading

simonhong Jul 27, 2022

sangwoo108 left a comment

-    return;
+  if (base::win::CanPinShortcut()) {
+       ...// call ShellExecute version base::win::PinShortcutToTaskbar() in callback
+  }
+     return;

Pin Brave to taskbar from FirstRun dialog #14311

Pin Brave to taskbar from FirstRun dialog #14311

Conversation

simonhong commented Jul 25, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangwoo108 Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangwoo108 Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangwoo108 left a comment

Choose a reason for hiding this comment

simonhong commented Jul 25, 2022 •

edited

Loading

sangwoo108 Jul 27, 2022 •

edited

Loading

simonhong Jul 27, 2022 •

edited

Loading

sangwoo108 Jul 27, 2022 •

edited

Loading