Skip to content
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.

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

Pin Brave to taskbar from FirstRun dialog #14311

Merged
merged 1 commit into from
Jul 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 173 additions & 0 deletions browser/brave_shell_integration_win.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
/* Copyright (c) 2022 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/browser/brave_shell_integration_win.h"

#include <shlobj.h>
#include <wrl/client.h>

#include <memory>
#include <string>

#include "base/base_paths.h"
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/path_service.h"
#include "base/strings/string_util.h"
#include "base/task/thread_pool.h"
#include "base/win/windows_version.h"
#include "chrome/browser/shell_integration_win.h"
#include "chrome/installer/util/install_util.h"
#include "chrome/installer/util/shell_util.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace {

std::wstring ExtractShortcutNameFromProperties(
const ShellUtil::ShortcutProperties& properties) {
std::wstring shortcut_name = properties.has_shortcut_name()
? properties.shortcut_name
: InstallUtil::GetShortcutName();

if (!base::EndsWith(shortcut_name, installer::kLnkExt,
base::CompareCase::INSENSITIVE_ASCII))
shortcut_name += installer::kLnkExt;

return shortcut_name;
}

// NOTE: Below Pin/IsPin method is copied lastest chromium.
// Delete and use upstreams one when it's available from our trunk.

// ScopedPIDLFromPath class, and the idea of using IPinnedList3::Modify,
// are thanks to Gee Law <https://geelaw.blog/entries/msedge-pins/>
class ScopedPIDLFromPath {
public:
explicit ScopedPIDLFromPath(PCWSTR path)
: p_id_list_(ILCreateFromPath(path)) {}
~ScopedPIDLFromPath() {
if (p_id_list_)
ILFree(p_id_list_);
}
PIDLIST_ABSOLUTE Get() const { return p_id_list_; }

private:
PIDLIST_ABSOLUTE const p_id_list_;
};

enum class PinnedListModifyCaller { kExplorer = 4 };

constexpr GUID CLSID_TaskbandPin = {
0x90aa3a4e,
0x1cba,
0x4233,
{0xb8, 0xbb, 0x53, 0x57, 0x73, 0xd4, 0x84, 0x49}};

// Undocumented COM interface for manipulating taskbar pinned list.
class __declspec(uuid("0DD79AE2-D156-45D4-9EEB-3B549769E940")) IPinnedList3
: public IUnknown {
public:
virtual HRESULT STDMETHODCALLTYPE EnumObjects() = 0;
virtual HRESULT STDMETHODCALLTYPE GetPinnableInfo() = 0;
virtual HRESULT STDMETHODCALLTYPE IsPinnable() = 0;
virtual HRESULT STDMETHODCALLTYPE Resolve() = 0;
virtual HRESULT STDMETHODCALLTYPE LegacyModify() = 0;
virtual HRESULT STDMETHODCALLTYPE GetChangeCount() = 0;
virtual HRESULT STDMETHODCALLTYPE IsPinned(PCIDLIST_ABSOLUTE) = 0;
virtual HRESULT STDMETHODCALLTYPE GetPinnedItem() = 0;
virtual HRESULT STDMETHODCALLTYPE GetAppIDForPinnedItem() = 0;
virtual HRESULT STDMETHODCALLTYPE ItemChangeNotify() = 0;
virtual HRESULT STDMETHODCALLTYPE UpdateForRemovedItemsAsNecessary() = 0;
virtual HRESULT STDMETHODCALLTYPE PinShellLink() = 0;
virtual HRESULT STDMETHODCALLTYPE GetPinnedItemForAppID() = 0;
virtual HRESULT STDMETHODCALLTYPE Modify(PCIDLIST_ABSOLUTE unpin,
PCIDLIST_ABSOLUTE pin,
PinnedListModifyCaller caller) = 0;
};

// Returns the taskbar pinned list if successful, an empty ComPtr otherwise.
Microsoft::WRL::ComPtr<IPinnedList3> GetTaskbarPinnedList() {
if (base::win::GetVersion() < base::win::Version::WIN10_RS5)
return nullptr;

Microsoft::WRL::ComPtr<IPinnedList3> pinned_list;
if (FAILED(CoCreateInstance(CLSID_TaskbandPin, nullptr, CLSCTX_INPROC_SERVER,
IID_PPV_ARGS(&pinned_list)))) {
return nullptr;
}

return pinned_list;
}

bool PinShortcutWin10(const base::FilePath& shortcut) {
Microsoft::WRL::ComPtr<IPinnedList3> pinned_list = GetTaskbarPinnedList();
if (!pinned_list)
return false;

ScopedPIDLFromPath item_id_list(shortcut.value().data());
HRESULT hr = pinned_list->Modify(nullptr, item_id_list.Get(),
PinnedListModifyCaller::kExplorer);
return SUCCEEDED(hr);
}

absl::optional<bool> IsShortcutPinnedWin10(const base::FilePath& shortcut) {
Microsoft::WRL::ComPtr<IPinnedList3> pinned_list = GetTaskbarPinnedList();
if (!pinned_list.Get())
return absl::nullopt;

ScopedPIDLFromPath item_id_list(shortcut.value().data());
HRESULT hr = pinned_list->IsPinned(item_id_list.Get());
// S_OK means `shortcut` is pinned, S_FALSE mean it's not pinned.
return SUCCEEDED(hr) ? absl::optional<bool>(hr == S_OK) : absl::nullopt;
}

void PinToTaskbarImpl() {
base::FilePath chrome_exe;
if (!base::PathService::Get(base::FILE_EXE, &chrome_exe))
return;

ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to test chromium's method.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

ShellUtil::AddDefaultShortcutProperties(chrome_exe, &properties);
// Generate the shortcut path and ask to pin it.
base::FilePath shortcut_path;
ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
ShellUtil::CURRENT_USER, &shortcut_path);
std::wstring shortcut_name(ExtractShortcutNameFromProperties(properties));
shortcut_path = shortcut_path.Append(shortcut_name);

bool create_shortcut = true;
if (base::PathExists(shortcut_path))
create_shortcut = false;

if (create_shortcut && !ShellUtil::CreateOrUpdateShortcut(
ShellUtil::SHORTCUT_LOCATION_DESKTOP, properties,
ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS)) {
LOG(ERROR) << __func__ << " Failed to create shortcut.";
return;
}

auto pinned = IsShortcutPinnedWin10(shortcut_path);
// Don't try to pin when checking is failed or it's already pinned.
if (!pinned || *pinned)
return;

PinShortcutWin10(shortcut_path);
}

} // namespace

namespace shell_integration::win {

// TODO(simonhong): Support profile specific shortcut pinning.
// For now, only default profile's shortcut is created and pinned.
void PinToTaskbar() {
if (base::win::GetVersion() < base::win::Version::WIN10_RS5)
return;
Copy link
Contributor

@sangwoo108 sangwoo108 Jul 27, 2022

Choose a reason for hiding this comment

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

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;

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@simonhong simonhong Jul 27, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@sangwoo108 sangwoo108 Jul 27, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@sangwoo108 Thanks for review! 👍🏼


base::ThreadPool::CreateCOMSTATaskRunner({base::MayBlock()})
->PostTask(FROM_HERE, base::BindOnce(&PinToTaskbarImpl));
}

} // namespace shell_integration::win
15 changes: 15 additions & 0 deletions browser/brave_shell_integration_win.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/* Copyright (c) 2022 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_BROWSER_BRAVE_SHELL_INTEGRATION_WIN_H_
#define BRAVE_BROWSER_BRAVE_SHELL_INTEGRATION_WIN_H_

namespace shell_integration::win {

void PinToTaskbar();

} // namespace shell_integration::win

#endif // BRAVE_BROWSER_BRAVE_SHELL_INTEGRATION_WIN_H_
4 changes: 4 additions & 0 deletions browser/sources.gni
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ brave_chrome_browser_deps = [
"//net",
"//services/metrics/public/cpp:metrics_cpp",
"//services/network/public/cpp",
"//third_party/abseil-cpp:absl",
"//third_party/blink/public/mojom:mojom_platform_headers",
"//third_party/widevine/cdm:buildflags",
"//ui/base",
Expand Down Expand Up @@ -336,9 +337,12 @@ if (enable_brave_translate_go || enable_brave_translate_extension) {

if (is_win) {
brave_chrome_browser_sources += [
"//brave/browser/brave_shell_integration_win.cc",
"//brave/browser/brave_shell_integration_win.h",
"//brave/browser/default_protocol_handler_utils_win.cc",
"//brave/browser/default_protocol_handler_utils_win.h",
]

brave_chrome_browser_deps += [
"//chrome/install_static:install_static_util",
"//chrome/installer/util:with_no_strings",
Expand Down