Groups

Move ComBase functions and HSTRING helpers to base/win. [chromium/src : master]

90 views

Skip to first unread message

Finnur Thorarinsson (Gerrit)

unread,

Sep 5, 2017, 10:46:07 AM9/5/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Takashi Toyoshima, Peter Beverloo, Commit Bot, chromium...@chromium.org

Rob, this is what I was thinking of, in terms of moving the duplicated code into base/win. Let me know if you think this should be done differently.

Peter, can you please look at notification_platform_bridge_win.cc?
Takashi-san, can you please look at midi_manager_winrt.cc? Also, what's the level of manual test coverage needed for this? I'm not sure how to test this... Do the automated tests provide enough coverage?

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Robert Liao (Gerrit)

unread,

Sep 5, 2017, 5:39:43 PM9/5/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Peter Beverloo, Commit Bot, chromium...@chromium.org

2 comments:

File base/win/com_base_util.h:

Patch Set #4, Line 36:

BASE_EXPORT static HRESULT WindowsCreateString(const base::char16* src,
                                                 uint32_t len,
                                                 HSTRING* out_hstr);

  BASE_EXPORT static HRESULT WindowsDeleteString(HSTRING hstr);

  BASE_EXPORT static HRESULT WindowsCreateStringReference(
      const base::char16* src,
      uint32_t len,
      HSTRING_HEADER* out_header,
      HSTRING* out_hstr);

  BASE_EXPORT static const base::char16* WindowsGetStringRawBuffer(
      HSTRING hstr,
      uint32_t* out_len);

These would be useful in a separate HSTRING helpers similar to HstringRef

File base/win/com_base_util.cc:
- Patch Set #4, Line 88: std::string name("RoGetActivationFactory");
  Instead of the preload, it's sufficient (but not necessary) to have the caller request the library ahead of time. The function lookup should be "pretty quick" as it's just looking through the exports table.
  The functions would be resolved at the first call and would report errors similar to how Windows does (a la returning false/E_FAIL/going through GetLastError()).
  This is similar to how the delayload system works.
  This avoids having callers specify these APIs by string.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 6, 2017, 3:59:13 AM9/6/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

7 comments:

File base/win/com_base_util.h:
- Patch Set #4, Line 1: /
  base/win/win_util seems to have a code to load combase.dll too.
  will you modify it to use this?
- Patch Set #4, Line 22: class
  optionally can be a namespace instead of class since all meaningful methods are static (and I believe no one instantiate this though ctor/dtor are defined).

Patch Set #4, Line 36:

BASE_EXPORT static HRESULT WindowsCreateString(const base::char16* src,
                                                 uint32_t len,
                                                 HSTRING* out_hstr);

  BASE_EXPORT static HRESULT WindowsDeleteString(HSTRING hstr);

  BASE_EXPORT static HRESULT WindowsCreateStringReference(
      const base::char16* src,
      uint32_t len,
      HSTRING_HEADER* out_header,
      HSTRING* out_hstr);

  BASE_EXPORT static const base::char16* WindowsGetStringRawBuffer(
      HSTRING hstr,
      uint32_t* out_len);

These would be useful in a separate HSTRING helpers similar to HstringRef

I'm not sure if HStringRef is useful, but agreed that it would be better to move these to HStringRef, or ScopedHString if HStringRef is not needed.

Patch Set #4, Line 66:

  static bool PreloadRoGetActivationFactory();
  static bool PreloadRoActivateInstance();
  static bool PreloadWindowsCreateString();
  static bool PreloadWindowsDeleteString();
  static bool PreloadWindowsCreateStringReference();
  static bool PreloadWindowsGetStringRawBuffer();

Do you really need to make these method and EnsureLibraryLoaded() private static methods?
If these are not called from unit tests, I think these can be defined in anonymous namespace in .cc.

Patch Set #4, Line 73:

  static decltype(&::RoGetActivationFactory) get_factory_func_;
  static decltype(&::RoActivateInstance) activate_instance_func_;
  static decltype(&::WindowsCreateString) create_string_func_;
  static decltype(&::WindowsDeleteString) delete_string_func_;
  static decltype(&::WindowsCreateStringReference) create_string_ref_func_;
  static decltype(&::WindowsGetStringRawBuffer) get_string_raw_buffer_func_;

  static HMODULE combase_dll_;

these also can be in anonymous namespace in .cc.

File base/win/hstring_ref.h:
- Patch Set #4, Line 16: // A class for creating HStringReferences.
  Is this really useful?
  It looks better to merge this into ScopedHString, and use ScopedHString from existing callers to avoid handle leak.
File chrome/browser/notifications/notification_platform_bridge_win.cc:
- Patch Set #4, Line 46: ref_class_name
  is it ok to leak the HSTRING here?
  probably it should be a scoped object, or static const instance if it's performance critical?
  ditto for other HStringRef instance

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 6, 2017, 5:01:10 AM9/6/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

just drivin' by...

2 comments:

File base/win/com_base_util.h:
- Patch Set #4, Line 11: #include <vector>
  nit: blank line between c and c++ header blocks as per the example in https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes.

- Patch Set #4, Line 22: class

- optionally can be a namespace instead of class since all meaningful methods are static (and I believ […]
  while the style guide says to prefer a namespace over a class (https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions), i don't see a reason to go against it here. please switch.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 6, 2017, 10:25:23 AM9/6/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Greg Thompson, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

9 comments:

File base/win/com_base_util.h:
- Patch Set #4, Line 1: /
  base/win/win_util seems to have a code to load combase.dll too. […]
  Done

- Patch Set #4, Line 11:

- nit: blank line between c and c++ header blocks as per the example in https://google.github. […]
  Done
- Patch Set #4, Line 22: ies.
  while the style guide says to prefer a namespace over a class (https://google.github. […]
  Done

- Patch Set #4, Line 36:

SE_EXPORT HRESULT RoActivateInstance(HSTRING class_id,
                                       IInspectable** instance);

BASE_EXPORT HRESULT WindowsCreateString(const base::char16* src,
                                        uint32_t len,
                                        HSTRING* out_hstr);

BASE_EXPORT HRESULT WindowsDeleteString(HSTRING hstr);

BASE_EXPORT const base::char16* WindowsGetStringRawBuffer(HSTRING hstr,
                                                          uint32_t* out_len);

BASE_EXPORT HMODULE GetLibraryForTesting();

}  // namespace ComBaseUtil

I'm not sure if HStringRef is useful, but agreed that it would be better to move these to HStringRef […]

I'd like to request a clarification on where you guys are headed with this...

Are we talking about simply moving these functions into ScopedHString? (Note: HStringRef is gone now). That would mean doing a LoadLibrary there also, right? And the associated Preload functions.

Or are we talking about helper functions in ScopedHString that internally calls these functions?

- Patch Set #4, Line 66:

- Do you really need to make these method and EnsureLibraryLoaded() private static methods? […]
  Done

- Patch Set #4, Line 73:

- these also can be in anonymous namespace in .cc.

- Done
File base/win/com_base_util.cc:
- Patch Set #4, Line 88: const IID& iid
  Instead of the preload, it's sufficient (but not necessary) to have the caller request the library a […]
  I've removed this function entirely and exported each Preload function instead. This gets rid of callers specifying APIs by string. I don't think it is sufficient for callers to request just the library ahead of time, because (for example) the notifications code needs to know up front if any of the functions requested are not available, so it can fall back on non-native notifications.
File base/win/hstring_ref.h:

- Patch Set #4, Line 16:

- Is this really useful? […]
  Done
File chrome/browser/notifications/notification_platform_bridge_win.cc:

- Patch Set #4, Line 46:

- is it ok to leak the HSTRING here? […]
  Done

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Robert Liao (Gerrit)

unread,

Sep 6, 2017, 1:14:57 PM9/6/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Greg Thompson, Takashi Toyoshima, Peter Beverloo, Commit Bot, chromium...@chromium.org

1 comment:

File base/win/com_base_util.h:
- Patch Set #4, Line 36:

SE_EXPORT HRESULT RoActivateInstance(HSTRING class_id,
                                       IInspectable** instance);

BASE_EXPORT HRESULT WindowsCreateString(const base::char16* src,
                                        uint32_t len,
                                        HSTRING* out_hstr);

BASE_EXPORT HRESULT WindowsDeleteString(HSTRING hstr);

BASE_EXPORT const base::char16* WindowsGetStringRawBuffer(HSTRING hstr,
                                                          uint32_t* out_len);

BASE_EXPORT HMODULE GetLibraryForTesting();

}  // namespace ComBaseUtil

I'd like to request a clarification on where you guys are headed with this... […]

I'd like to group these APIs into logical groups.

We have things that deal with WinRT here (and can name this file winrt_util.h). RoGetActivationFactory and RoActivateInstance are WinRT supporting APIs.

HSTRINGs are a separate API and WinRT just happens to consume them, so having Windows*String and friends makes sense for this. Other APIs that aren't as closely related to WinRT (but are projected by the WinRT system) also consume HSTRINGs. A ScopedHString is the way to go for this.

I can see a world where we have

// Heavyweight HSTRING
class ScopedHString {
 public:
  // WindowsCreateString + WindowsDeleteString, and any additional useful variants
  static ScopedHString Create(const wchar_t* string);

  // Loads all required HSTRING functions. These tend to be consistent since Win8.
  static bool PreloadRequiredFunctions();

  // Returns an HSTRING for use by Windows APIs.
  HSTRING& GetHString() const;
};

// A way to quickly wrap preexisting strings for the purpose of passing them into APIs.
class ScopedHStringReference {
 public:
  // WindowsCreateStringReference
  ScopedHStringReference(const wchar_t* string);

  // Returns an HSTRING for use by Windows APIs.
  HSTRING& GetHString() const;

  // Loads all required HSTRING functions. These tend to be consistent since Win8.
  static bool PreloadRequiredFunctions();
}

I see you've used ScopedGeneric as a way to handle this, which seems fine.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 7, 2017, 2:33:12 AM9/7/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

1 comment:

File base/win/com_base_util.cc:

Patch Set #6, Line 10:


static HMODULE combase_dll_ = nullptr;

static decltype(&::RoGetActivationFactory) get_factory_func_ = nullptr;
static decltype(&::RoActivateInstance) activate_instance_func_ = nullptr;
static decltype(&::WindowsCreateString) create_string_func_ = nullptr;
static decltype(&::WindowsDeleteString) delete_string_func_ = nullptr;
static decltype(&::WindowsCreateStringReference) create_string_ref_func_ =
    nullptr;
static decltype(&::WindowsGetStringRawBuffer) get_string_raw_buffer_func_ =
    nullptr;

I suggested move them here, but now I'm afraid of race conditions.

E.g., MidiManagerWinrt runs mainly on the Chrome I/O thread, but some methods and callbacks run on other threads. Probably, other users may call them on the main thread. So thread-safety should be ensured inside this library to make this library usable.

Probably the easiest way to ensure the thread-safety is to use local static variables in each function. That works like base::LazyInstance does.

For instance,

HMODULE GetComBaseModuleHandle() {
  static combase_dll = ::LoadLibrary(L"combae.dll");
  return combase_dll;
}

Here, combase_dll is initialized only at the first call of GetComBaseModuleHandle in a thread-safe way, and will return the same initialized combase_dll always.

Other wrappers will be something like

HRESULT RoGetActivationFactory(HSTRING class_id,
                               const IID& iid,
                               void** out_factory) {
  static get_factory_func = ResolveRoGetActivationFactory();
  DCHECK(get_factory_func);
  return get_factory_func(class_id, iid, out_factory);
}

GetComBaseModuleHandle, ResolveRoGetActivationFactory, and so on can be functions in anonymous namespace. All callers have to do is just call RoGetActivationFactory without any preparation.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 7, 2017, 2:36:12 AM9/7/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

Ah, I forgot to declare type for each local static variable in the sample code. Of course, it should be "static HMODULE combase_dll" and so on.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 7, 2017, 11:30:29 AM9/7/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

PTAL

2 comments:

File base/win/com_base_util.h:

- Patch Set #4, Line 36:

- ```
 // namespace ComBaseUtil
}  // namespace win
}  // namespace base

#endif  // BASE_WIN_COM_BASE_UTIL_H_
```
  I'd like to group these APIs into logical groups. […]
  I think I met you half way there with the ScopedHString changes. See if this fits the bill.
File base/win/com_base_util.cc:

- Patch Set #6, Line 10:


template <typename signature>
bool GetProcAddress(HMODULE module,
                    signature* function_signature,
                    const std::string& function_name) {
  if (!module)
    return false;
  *function_signature = reinterpret_cast<signature>(
      ::GetProcAddress(module, function_name.c_str()));
  if (!*function_signature)
    return false;

I suggested move them here, but now I'm afraid of race conditions. […]

Thanks! Did something along those lines (still have to expose the PreloadFoo functions for callers that need to know upfront whether the functions are available).

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Robert Liao (Gerrit)

unread,

Sep 7, 2017, 9:01:09 PM9/7/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

Thanks for the changes. The overall structure looks more in line with what this should be.

18 comments:

File base/win/com_base_util.h:
- Patch Set #9, Line 8:
```
#include <Inspectable.h>
#include <Roapi.h>
```
  Lowercase these files. MSDN and VS have uppercase versions, which is incorrect.
  (And depending on the Preload* signature below, these can go away.)
  HRESULT can be grabbed from winnt.h
- Patch Set #9, Line 10:
```
#include <vector>

#include "base/gtest_prod_util.h"
#include "base/strings/string16.h"
```
  Nit: Unused headers.
- Patch Set #9, Line 24: BASE_EXPORT HMODULE GetComBaseModuleHandle();
  Do we expect callers to need the HMODULE?
  If not, this should be sufficient in the anonymous namespace.
- Patch Set #9, Line 26: decltype(&::RoGetActivationFactory)
  These should return bools. Callers should use the functions below directly.
- Patch Set #9, Line 29: HSTRING
  #include <hstring.h>
File base/win/com_base_util.cc:
- Patch Set #9, Line 18: ::GetProcAddress
  Include windows.h in this file to cover GetProcAddress and LoadLibrary.
- Patch Set #9, Line 19:
```
  if (!*function_signature)
    return false;
  return true;
```
  return !!*function_signature
  Can the return value not be the function signature or is it not nullable?
- Patch Set #9, Line 68: decltype(&::RoGetActivationFactory)
  Can this be 'auto'?
- Patch Set #9, Line 71: E_ACCESSDENIED
  E_ACCESSDENIED is used primarily for access checks. A regular E_FAIL will be good enough here.
File base/win/com_base_util_unittest.cc:
- Patch Set #9, Line 24: CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED);
  Use ScopedComInitializer
- Patch Set #9, Line 45:
```
decltype(&::RoGetActivationFactory) func1 = PreloadRoGetActivationFactory();
  EXPECT_TRUE(expectation() == (func1 != nullptr));
  FreeComBaseModule();

  decltype(&::RoActivateInstance) func2 = PreloadRoActivateInstance();
  EXPECT_TRUE(expectation() == (func2 != nullptr));
  FreeComBaseModule();
```
  I would be fine either removing these tests or turning these into sanity check tests (runs code to verify no crash occurs with no test expectations).
  We shouldn't be calling the APIs or verifying the existence of APIs for various Windows platforms as it's not clear what the dev should do if those tests fail.

File base/win/scoped_hstring.h:

Patch Set #9, Line 34:

BASE_EXPORT static HRESULT WindowsCreateString(const base::char16* src,
                                                 uint32_t len,
                                                 HSTRING* out_hstr);

  BASE_EXPORT static HRESULT WindowsDeleteString(HSTRING hstr);

  BASE_EXPORT static const base::char16* WindowsGetStringRawBuffer(
      HSTRING hstr,
      uint32_t* out_len);

- With ScopedHString, should folks should not need to call these functions directly. ScopedHString should provide equivalent projections.
File base/win/scoped_hstring.cc:
- Patch Set #9, Line 12:

template <typename signature>
bool GetProcAddress(HMODULE module,
                    signature* function_signature,
                    const std::string& function_name) {
  if (!module)
    return false;
  *function_signature = reinterpret_cast<signature>(
      ::GetProcAddress(module, function_name.c_str()));
  if (!*function_signature)
    return false;

- ```
  return true;
}
```
  This sounds like something that could go in a util
- Patch Set #9, Line 26: base::win::ComBaseUtil::GetComBaseModuleHandle();
  Have these directly load Combase here too instead of reaching into ComBaseUtil. Windows will just refcount the load.
  This makes it easier to change the library in the future should MS want to change the locations (and has happened between Windows versions when some stuff from ole32.dll went to combase.dll). Now, Windows generally reprojects those APIs when they do something like that, so it's generally not an issue.
- Patch Set #9, Line 86:
```
ScopedHString::ScopedHString(const base::char16* str) : ScopedGeneric(nullptr) {
  HSTRING hstr;
  HRESULT hr =
      WindowsCreateString(str, static_cast<uint32_t>(wcslen(str)), &hstr);
  if (FAILED(hr))
    VLOG(1) << "WindowsCreateString failed";
  else
    reset(hstr);
}
```
  Since there is a potential for the API to fail, this should go into a factory method and the constructor should be made private.
  Something like
  std::unique_ptr<ScopedHString> Create(const base::char16* str) {
  }
  should do the trick.
- Patch Set #9, Line 105:
```
decltype(&::WindowsCreateString) create_string_func_ =
      PreloadWindowsCreateString();
  if (!create_string_func_)
    return E_ACCESSDENIED;
```
  Carry forward the COM comments here.
- Patch Set #9, Line 126: L""
  This should return nullptr on failure.
File base/win/scoped_hstring_unittest.cc:
- Patch Set #9, Line 42: EXPECT_STREQ(kTestString1, buffer);
  Similarly, it's useful to have crash check tests, but verifying the APIs is beyond the scope of Chromium.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 8, 2017, 2:19:35 AM9/8/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

Robert's comments cover almost everything what I want on the patch set 9.
Here is one additional minor comment on media/midi.

Thank you for working on this!

1 comment:

File media/midi/midi_manager_winrt.cc:
- Patch Set #9, Line 863: GetLastError()
  probably, calling GetLastError() here does not make sense any more because the actual error is now behind the library, and what we want to show may not be the last error. It's just fine to remove this VLOG completely.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 8, 2017, 5:57:23 AM9/8/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

6 comments:

File base/win/scoped_hstring.h:
- Patch Set #9, Line 19: class
  nit: traits types like this are generally defined via "struct" rather than "class" (see doc comments in scoped_generic.h for an example). then you can get rid of "public:" below.
- Patch Set #9, Line 28: const base::char16* str
  use "base::StringPiece16 str" here to avoid the strlen in cases where the caller already knows it.
File base/win/scoped_hstring.cc:
- Patch Set #9, Line 76: // static
  these are sufficiently simple that i think they should be inlined in the .h. ScopedGeneric's doc comment even says that this one is assumed to be inline.
- Patch Set #9, Line 91: VLOG(1) << "WindowsCreateString failed";
```
what's your case for VLOG(1)? who will see these messages to diagnose them? if anything, how about:
    DLOG(ERROR) << "WindowsCreateString failed " << std::hex << hr;
so that it's a debug-only message with useful data?
```

Patch Set #9, Line 86:

ScopedHString::ScopedHString(const base::char16* str) : ScopedGeneric(nullptr) {
  HSTRING hstr;
  HRESULT hr =
      WindowsCreateString(str, static_cast<uint32_t>(wcslen(str)), &hstr);
  if (FAILED(hr))
    VLOG(1) << "WindowsCreateString failed";
  else
    reset(hstr);
}

- Since there is a potential for the API to fail, this should go into a factory method and the constru […]
```
ScopedGeneric is movable, so no need for unique_ptr:
...Create(...) {
  HSTRING hstr;
  HRESULT hr = ...;
  if (SUCCEDED(hr))
    return ScopedHString(hstr);
  DLOG(ERROR) << ...;
  return ScopedHString();
}
```
- Patch Set #9, Line 97: bool ScopedHString::PreloadRequiredFunctions() {
  apologies if i missed the discussion, but why do this rather than delayload? if it's because you want to know whether or not the dll/methods exist, you can do a one-time check via __HrLoadAllImportsForDll; see device/bluetooth/bluetooth_init_win.cc for an example. i think you can remove a whole lot of code if you let the loader do the work for you.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Robert Liao (Gerrit)

unread,

Sep 8, 2017, 3:24:13 PM9/8/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

2 comments:

File base/win/com_base_util.cc:
- Patch Set #9, Line 71: E_ACCESSDENIED

- E_ACCESSDENIED is used primarily for access checks. A regular E_FAIL will be good enough here.

- toyoshim@'s comment reminded me you can also use HRESULT_FROM_WIN32(GetLastError()) to have Windows explain what happened.
  IMPORTANT: Care must be taken to call GetLastError() immediately after the failing call, which means the Preloads should also provide a return value or set the last result right before returning if other Windows calls were made (and these are hard to spot).
File base/win/scoped_hstring.cc:
- Patch Set #9, Line 97: bool ScopedHString::PreloadRequiredFunctions() {
  apologies if i missed the discussion, but why do this rather than delayload? if it's because you wan […]
  There is a need for notifications to early fail if these functions don't exist, so finding them out at call time is too late.
  I'll defer to finnur@ on that.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 8, 2017, 5:40:52 PM9/8/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Takashi Toyoshima, Peter Beverloo, Commit Bot, chromium...@chromium.org

1 comment:

File base/win/scoped_hstring.cc:

- Patch Set #9, Line 97: bool ScopedHString::PreloadRequiredFunctions() {

- There is a need for notifications to early fail if these functions don't exist, so finding them out […]
  __HrLoadAllImportsForDll can do that, but only if all code that imports from combase.dll are okay with it being delayloaded, and if the imports from it are all-or-nothing.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Robert Liao (Gerrit)

unread,

Sep 8, 2017, 5:43:50 PM9/8/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

1 comment:

File base/win/scoped_hstring.cc:

- Patch Set #9, Line 97: bool ScopedHString::PreloadRequiredFunctions() {

- __HrLoadAllImportsForDll can do that, but only if all code that imports from combase. […]
  It's an interesting thought. Should this be the guidance for all delayloadable APIs going forward? That would save quite a few cases of this I'd imagine.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 8, 2017, 5:52:27 PM9/8/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Takashi Toyoshima, Peter Beverloo, Commit Bot, chromium...@chromium.org

1 comment:

File base/win/scoped_hstring.cc:

- Patch Set #9, Line 97: bool ScopedHString::PreloadRequiredFunctions() {

- It's an interesting thought. […]
  It's working well enough in the bluetooth code. In that case, the only sensible consumer of the dll is that one section of chromium code. If combase will have consumers sprinkled about (it's a grab-bag of APIs, no?), then maybe it's more difficult to take this pill. If a future build of Windows adds a new function, callers of that function will have to use the LoadLibrary approach, as relying on the delayload would cause this __HrLoadAllImportsForDll to start failing on systems where it previously worked. I can imagine this becoming a maintenance problem without regression tests running on the right OSes.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 11, 2017, 10:17:06 AM9/11/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

This CL includes only Greg's proposed changes for using __HrLoadAllImportsForDll (got it to work, wanted to see how it fares on the try bots and possibly facilitate further discussions on it).

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 11, 2017, 4:45:21 PM9/11/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Takashi Toyoshima, Peter Beverloo, Commit Bot, chromium...@chromium.org

a few comments.

7 comments:

File base/win/com_base_util.h:
- Patch Set #10, Line 22: namespace ComBaseUtil {
  nit: "Namespace names are all lower-case." (https://google.github.io/styleguide/cppguide.html#Namespace_Names)
File base/win/scoped_hstring.h:
- Patch Set #10, Line 21: BASE_EXPORT static HSTRING InvalidValue();
  i don't believe you need BASE_EXPORT on individual members of a class that is BASE_EXPORT.
- Patch Set #10, Line 25: BASE_EXPORT class ScopedHString
  consider a getter that returns a base::StringPiece16 to the raw string. seems useful.
File base/win/scoped_hstring.cc:
- Patch Set #10, Line 54: } has_loaded = LOAD_STATUS_UNKNOWN;
```
you can get rid of the UNKNOWN case and the comparison on line 56 and make this threadsafe(!) by using a lambda. something like:
  static const bool load_succeeded = []() {
    HRESULT hr = E_FAIL;
    __try {
      ...
    }
    return SUCCEEDED(hr);
  }();
  return load_succeeded;
```
  also, consider ThreadRestrictions::AssertIOAllowed() at the top of the function since it's expected to go to disk to load the DLL.
- Patch Set #10, Line 72: const base::char16* buffer = WindowsGetStringRawBuffer(hstr, nullptr);
  this should capture the length and use it below in the 3-arg WideToUTF8.
File base/win/scoped_hstring_unittest.cc:
- Patch Set #10, Line 57: EXPECT_TRUE(FreeLibrary(ComBaseUtil::GetComBaseModuleHandle()));
  this shouldn't be here, should it?
  (same comment below)
- Patch Set #10, Line 62: if (base::win::GetVersion() < base::win::VERSION_WIN10_RS1)
  it's supported in win8.1, no?

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 13, 2017, 2:00:31 AM9/13/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

4 comments:

File base/win/scoped_hstring.h:
- Patch Set #12, Line 34: HStringToString
  Having this static method here does not sound natural.
  How about adding a constructor that takes HSTRING, making this non-static method, and change the signature to GetString() or ToString()?
  Also, my caller side code in media/midi may have leaked HSTRING reference until today. See my comments in media/midi.
File media/midi/midi_manager_winrt.cc:
- Patch Set #12, Line 207:
  IIUC, we mistakenly leaked the obtained HSTRING reference here.
  (I can not find a clear document to know if get_Id increases the reference internally or not, but calling WindowsDeleteString at the last line seems to work fine.
  Probably, I want to rewrite the last line as follows;
```
  ScopedHString string(result);
  return string::GetString();
```
  You need to have another explicit constructor to take HSTRING and it's fine to call WindowsDeleteString for the HSTRING in the destructor.
- Patch Set #12, Line 218:
  ditto
- Patch Set #12, Line 228:
  ditto

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 13, 2017, 8:39:35 AM9/13/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

1 comment:

File base/win/scoped_hstring.h:
- Patch Set #13, Line 29: BASE_EXPORT static ScopedHString Create(const base::char16* str);
  I may miss other reviewers' comments on this, but do you have any reason to have static factory methods rather than to expose constructors?

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 13, 2017, 10:12:05 AM9/13/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

Phew! OK, I've addressed all comments, I think. I also dusted the cobwebs off my Win7 box and did some troubleshooting to see why this CL was failing there. I've gotten it to work on both my Win7 and Win10 box. Would appreciate another look at the CL.

There's one failure on the bot, which I can't repro, but think is not a red herring. It is failing due to:

FAILED: gen/net/http/transport_security_state_static.h
E:/b/depot_tools/win_tools-2_7_6_bin/python/bin/python.exe ../../build/gn_run_binary.py transport_security_state_generator.exe ../../net/http/transport_security_state_static.json ../../net/http/transport_security_state_static.pins ../../net/http/transport_security_state_static.template gen/net/http/transport_security_state_static.h
transport_security_state_generator.exe failed with exit code -1073741515

That error code is C0000135, or: "The program can't start because %hs is missing", which indicates another DLL needs to be delayloaded, but it doesn't say which and it doesn't repro on my machine so I'm not sure what the most productive way of addressing it is...

26 comments:

File base/win/com_base_util.h:

- Patch Set #10, Line 22:

- nit: "Namespace names are all lower-case." (https://google.github.io/styleguide/cppguide. […]
  Done
File base/win/com_base_util.h:
- Patch Set #9, Line 29: ING cla
  #include <hstring. […]
  Done
File base/win/com_base_util.cc:
- Patch Set #9, Line 18: : EXCEPTI

- Include windows.h in this file to cover GetProcAddress and LoadLibrary.

- Obsolete.

- Patch Set #9, Line 19:

- ```
}

}  // namespac
```
  return !!*function_signature […]
  Obsolete.

- Patch Set #9, Line 68:

- Can this be 'auto'?
  Obsolete, I believe.

- Patch Set #9, Line 71:

- toyoshim@'s comment reminded me you can also use HRESULT_FROM_WIN32(GetLastError()) to have Windows […]
  Obsolete, I believe.
File base/win/com_base_util_unittest.cc:
- Patch Set #9, Line 24: namespace win
  Use ScopedComInitializer
  Done
File base/win/scoped_hstring.h:

- Patch Set #13, Line 29: BASE_EXPORT static ScopedHString Create(const base::char16* str);

- I may miss other reviewers' comments on this, but do you have any reason to have static factory meth […]
  Robert made the argument that the API can fail and handling that in the constructor isn't ideal. Better to do so in a static Create method.
File base/win/scoped_hstring.h:
- Patch Set #12, Line 34: RequiredFunctio
  Having this static method here does not sound natural. […]
  Done
File base/win/scoped_hstring.h:
- Patch Set #10, Line 21: public:

- i don't believe you need BASE_EXPORT on individual members of a class that is BASE_EXPORT.

- Seems like the linker disagrees...

- Patch Set #10, Line 25:

- consider a getter that returns a base::StringPiece16 to the raw string. seems useful.
  Something like this? (see patch)

File base/win/scoped_hstring.h:

Patch Set #9, Line 34:

BASE_EXPORT static bool PreloadRequiredFunctions();

  BASE_EXPORT std::string GetString();
  BASE_EXPORT base::StringPiece16 GetString16();

 private:
  explicit ScopedHString(HSTRING hstr);
};

With ScopedHString, should folks should not need to call these functions directly. […]

Done

File base/win/scoped_hstring.cc:
- Patch Set #9, Line 26:
  Have these directly load Combase here too instead of reaching into ComBaseUtil. […]
  Obsolete, I believe.

- Patch Set #9, Line 76:

- these are sufficiently simple that i think they should be inlined in the .h. […]
  Done

- Patch Set #9, Line 91:

- what's your case for VLOG(1)? who will see these messages to diagnose them? if anything, how about: […]
  Done

- Patch Set #9, Line 86:

- ScopedGeneric is movable, so no need for unique_ptr: […]
  Done

- Patch Set #9, Line 97:

- It's working well enough in the bluetooth code. […]
  Done

- Patch Set #9, Line 105:

- Carry forward the COM comments here.

- Obsolete, I believe.

- Patch Set #9, Line 126:

- This should return nullptr on failure.
  Obsolete, I believe.
File base/win/scoped_hstring.cc:
- Patch Set #10, Line 54: __try {
  you can get rid of the UNKNOWN case and the comparison on line 56 and make this threadsafe(!) by usi […]
  Done
- Patch Set #10, Line 72: if (!base::WideToUTF8(buffer, length, &value))

- this should capture the length and use it below in the 3-arg WideToUTF8.

- Patch Set #10, Line 57:

- this shouldn't be here, should it? […]
  This was to ensure that a NoPreload test below would be valid (otherwise the DLL would have been loaded). But this is now obsolete.

- Patch Set #10, Line 62:

- it's supported in win8. […]
  Done. Looks like WIN8 and up to me.
File media/midi/midi_manager_winrt.cc:

- Patch Set #12, Line 207:

- IIUC, we mistakenly leaked the obtained HSTRING reference here. […]
  Done

- Patch Set #12, Line 218:
  ditto

- Done

- Patch Set #12, Line 228:
  ditto

- Done

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 13, 2017, 10:25:10 AM9/13/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

12 comments:

File base/BUILD.gn:
- Patch Set #13, Line 73: "/DELAYLOAD:api-ms-win-core-heap-l1-2-0.dll",
  i think it would be good to have a comment explaining why these are needed.
File base/win/com_base_util.h:
- Patch Set #13, Line 18: combase.dll
  is this comment correct? it's not combase but a bunch of other dlls that contain the symbols, no?
- Patch Set #13, Line 21: namespace com_base_util {
  on second thought, is this namespace even needed? how about changing the Preload function to something like ResolveCoreWinRTDelayload? the other two functions can go away altogether, can they not?
File base/win/com_base_util_unittest.cc:
- Patch Set #13, Line 16: if (base::win::GetVersion() < base::win::VERSION_WIN8)
  is there a reason not to test on win7 as well? are there cases where the preload would succeed on win7?
File base/win/scoped_hstring.h:
- Patch Set #13, Line 22: BASE_EXPORT
  nix BASE_EXPORT on the methods
- Patch Set #13, Line 36: BASE_EXPORT std::string GetString();
```
since the native format of an HString is a wide string, i think the API looks nice with:
  const base::StringPiece16 Get() const;
  std::string GetAsUTF8() const;
```
- Patch Set #13, Line 41: };
  DISALLOW_COPY_AND_ASSIGN?
File base/win/scoped_hstring.cc:
- Patch Set #13, Line 29: ScopedHString ScopedHString::Create(const base::char16* str) {
  i think it's cleaner for this to take a base::StringPiece16 so that wcslen isn't needed.
- Patch Set #13, Line 49: PreloadRequiredFunctions
  ?ResolveCoreWinRTStringDelayload?
- Patch Set #13, Line 68: const base::char16* buffer = WindowsGetStringRawBuffer(get(), &length);
  implement this in terms of GetString16() to avoid duplicating code?
- Patch Set #13, Line 70: return nullptr;
  return std::string() here and below
- Patch Set #13, Line 78: return WindowsGetStringRawBuffer(get(), nullptr);
  grab the length here and use it to construct the StringPiece16 to avoid another strlen

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 13, 2017, 11:19:40 AM9/13/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

Thanks for the comments. I haven't updated the code, just wanted to get a round of discussion in.

Also, do you have tips on how to tackle the transport_security_state_generator error?

2 comments:

File base/win/com_base_util.h:
- Patch Set #13, Line 21: namespace com_base_util {

- on second thought, is this namespace even needed?

- You mean ResolveCoreWinRTDelayload should just be part of base/win namespace?

- the other two functions can go away altogether, can they not?

- Well, they are there to contain the spreading of the mincore.lib+delayload dance to other modules. This way media/ and chrome/ can just call this through here.
File base/win/com_base_util_unittest.cc:

- Patch Set #13, Line 16: if (base::win::GetVersion() < base::win::VERSION_WIN8)

- is there a reason not to test on win7 as well? are there cases where the preload would succeed on wi […]
  The documentation for the two functions used (RoGetActivationFactory and RoActivateInstance) state a minimum on Win8, according to the documentation. Also, the DLL loaded is not present on my Win7 box (outside of an SDK/Web Tools directory).

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 14, 2017, 2:45:34 AM9/14/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

9 comments:

File base/win/com_base_util.h:
- Patch Set #13, Line 25: RoGetActivationFactory
  Wow, __HrLoadAllImportsForDll is fantastic!
  So, do we still need this kind of wrap functions?
  All we need seems to have "bool base::win::DelayLoad*" methods in base/win/delay_load?
File base/win/scoped_hstring.h:
- Patch Set #13, Line 26: BASE_EXPORT class ScopedHString
  Probably, you need explanations and some code examples here.
  Users may not notice that this class requires PreloadrequiredFunctions() before using.
- Patch Set #13, Line 29: BASE_EXPORT static ScopedHString Create(const base::char16* str);
  Robert made the argument that the API can fail and handling that in the constructor isn't ideal. […]
  Hum... current code just initializes ScopedHString with a nullptr for error cases. It does not do something special for errors. So, my personal opinion is this does not meat the bar to use another method for initialization. In both designs, what callers need is to call is_valid() after the instantiation. Also, when we avoid such errors in constructors, initialization method would return error information that constructors can not.
  But using Create to return a smart pointer is a pattern I often see. So, I'm neutral on this. So feel free to skip this comment.
- Patch Set #13, Line 30: Create
  Another choice is to use Create for char16* and constructor for HSTRING. But it's up to you.

- Patch Set #13, Line 36: BASE_EXPORT std::string GetString();

- since the native format of an HString is a wide string, i think the API looks nice with: […]
  +1
File base/win/scoped_hstring.cc:
- Patch Set #13, Line 44: ScopedHString::ScopedHString(HSTRING hstr) : ScopedGeneric(nullptr) {
  Could you have a DCHECK to ensure that Preload... was called beforehand?
  I'm afraid that calling unresolved delayed load dll methods would crash?
- Patch Set #13, Line 44: nullptr
  just ": ScopedGeneric(hstr) {}" does not work?
- Patch Set #13, Line 72: if (!base::WideToUTF8(buffer, length, &value))
  I guess we want to ignore this error in most cases rather than assuming it as empty string.
  This fails on invalid unicode characters, but conversion itself finishes with padding 0xFFFD for such invalid code places.
  How about just ignoring the error, or taking an argument "bool allow_invalid_unicode = true" so that callers can skip this error by default?
File base/win/scoped_hstring_unittest.cc:
- Patch Set #13, Line 48:
  may want to add a case to take invalid unicode string.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 14, 2017, 4:34:31 AM9/14/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

2 comments:

File base/win/com_base_util.h:
- Patch Set #13, Line 21: namespace com_base_util {

- on second thought, is this namespace even needed?
  You mean ResolveCoreWinRTDelayload should just be part of base/win namespace?

- Sure, why not? Namespaces are useful for either avoiding a naming collision or for making a logical grouping. In this case, there's so little to group and no risk of collision, so I don't think it adds much.

- the other two functions can go away altogether, can they not?
  Well, they are there to contain the spreading of the mincore.lib+delayload dance to other modules. This way media/ and chrome/ can just call this through here.

- Hmm. So you're suggesting that for component builds, all callers should go through the resolution that happens in base.dll, right? (For static builds this is a non-issue.) Seems sensible.
File base/win/com_base_util_unittest.cc:

- Patch Set #13, Line 16: if (base::win::GetVersion() < base::win::VERSION_WIN8)

- The documentation for the two functions used (RoGetActivationFactory and RoActivateInstance) state a […]
  Right, so PreloadRequiredFunctions() should return false (and not crash!) on Win7. Why not test this?

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Robert Liao (Gerrit)

unread,

Sep 14, 2017, 3:05:03 PM9/14/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

2 comments:

File base/win/core_winrt_util.cc:

Patch Set #15, Line 31:

__try {
      hr = __HrLoadAllImportsForDll("api-ms-win-core-winrt-l1-1-0.dll");
    } __except (FilterVisualCPPExceptions(::GetExceptionCode())) {
      hr = E_FAIL;
    }

Is handling exceptions worth the cost of using __HrLocalAllImportsForDll()?

File base/win/win_util.cc:
- Patch Set #15, Line 121: WindowsCreateString
  Should this used the ScopedHSTRING helper?

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 14, 2017, 7:51:14 PM9/14/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

21 comments:

File base/BUILD.gn:
- Patch Set #13, Line 73: # fail on Windows 7, for example).

- i think it would be good to have a comment explaining why these are needed.

- is this comment correct? it's not combase but a bunch of other dlls that contain the symbols, no?

- Done. Also renamed the .cc/.h file to reflect.

- Patch Set #13, Line 21:

- > > on second thought, is this namespace even needed? […]
  For these two issues:
  1) Done.
  2) Yes.

- Patch Set #13, Line 25:

- Wow, __HrLoadAllImportsForDll is fantastic! […]
  See my reply to Greg above.
File base/win/com_base_util_unittest.cc:

- Patch Set #13, Line 16:

- Right, so PreloadRequiredFunctions() should return false (and not crash!) on Win7. […]
  Done
File base/win/core_winrt_util.cc:

Patch Set #15, Line 31:

__try {
      hr = __HrLoadAllImportsForDll("api-ms-win-core-winrt-l1-1-0.dll");
    } __except (FilterVisualCPPExceptions(::GetExceptionCode())) {
      hr = E_FAIL;
    }

Is handling exceptions worth the cost of using __HrLocalAllImportsForDll()?

- That question feels somewhat above my pay grade. :)
  Greg?
File base/win/scoped_hstring.h:
- Patch Set #13, Line 22: BASE_EXPORT

- nix BASE_EXPORT on the methods

- When I do I start getting linker errors. For example:
  scoped_hstring_unittest.obj : error LNK2019: unresolved external symbol "public: static void __cdecl base::win::ScopedHStringTraits::Free(struct HSTRING__ *)" (?Free@ScopedHStringTraits@win@base@@SAXPAUHSTRING__@@@Z) referenced in function "private: void __thiscall base::ScopedGeneric<struct HSTRING__ *,class base::win::ScopedHStringTraits>::FreeIfNecessary(void)" (?FreeIfNecessary@?$ScopedGeneric@PAUHSTRING__@@VScopedHStringTraits@win@base@@@base@@AAEXXZ)
- Patch Set #13, Line 23:
  On second thoughts, I'm uncomfortable with inlining the WindowsDeleteString call here. I prefer to enclose all the Windows*String calls in the .cc file, if that's OK.
- Patch Set #13, Line 26: // functions that are only available on Windows 8 and later, and that these
  Probably, you need explanations and some code examples here. […]
  Done
- Patch Set #13, Line 30: Scope

- Another choice is to use Create for char16* and constructor for HSTRING. But it's up to you.

- Done
- Patch Set #13, Line 36: // // ScopeHString can be used.
  +1
  Done
- Patch Set #13, Line 41: // Example use:
  DISALLOW_COPY_AND_ASSIGN?
  I like the idea of providing a ctor with HSTRING, but using DISALLOW_COPY_AND_ASSIGN doesn't jive well with that, since the Create function then shows an error...
  error C2248: 'base::win::ScopedHString::ScopedHString': cannot access private member declared in class 'base::win::ScopedHString'
  ... when trying to do ...
```
   return ScopedHString(hstr);
```
  Scratching my head a bit over that...
File base/win/scoped_hstring.cc:
- Patch Set #13, Line 29: bool ScopedHString::load_succeeded = false;

- i think it's cleaner for this to take a base::StringPiece16 so that wcslen isn't needed.

- Done

- Patch Set #13, Line 44:

- just ": ScopedGeneric(hstr) {}" does not work?
  Done
- Patch Set #13, Line 44: return ScopedHString(nullptr);
  Could you have a DCHECK to ensure that Preload... was called beforehand? […]
  Done, sort of. If there's a better way, I'm open to suggestions.

- Patch Set #13, Line 49:

- ?ResolveCoreWinRTStringDelayload?
  Done

- Patch Set #13, Line 68: }

- implement this in terms of GetString16() to avoid duplicating code?
  Done
- Patch Set #13, Line 70: const base::StringPiece16 ScopedHString::Get() const {

- return std::string() here and below

- Done

- Patch Set #13, Line 72: }

- I guess we want to ignore this error in most cases rather than assuming it as empty string. […]
  Well, this has changed a lot since last CL. Take another look?

- Patch Set #13, Line 78:

- grab the length here and use it to construct the StringPiece16 to avoid another strlen
  Not sure what you mean... Is this obsolete due to recent changes?
File base/win/scoped_hstring_unittest.cc:
- Patch Set #13, Line 48: buffer = hstring.GetAsUTF8();

- may want to add a case to take invalid unicode string.

- What are good examples and what are the expectations in the test?

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 15, 2017, 7:04:49 AM9/15/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

Thank you for hard works on this.
Overall designs look good to me, but let me put some minor comments.

12 comments:

File base/win/com_base_util.h:
- Patch Set #13, Line 25:

- See my reply to Greg above.

- Ah, I think I understand, but let me check.
  When component build is enabled, base and midi are built as separate DLLs. Since delayload linker flag is specified only for base, the stubs for the runtime API can be visible only inside base even after preload was done, and media need to call base's exported function, this base::win::RoGetActivationFactory.
  That makes sense. Thanks!
File base/win/scoped_hstring.h:
- Patch Set #15, Line 17: // Scoped HSTRING class to maintain lifetime of HSTRINGs allocated with
  Traits can be inside internal namespace?
- Patch Set #15, Line 19: BASE_EXPORT
  BASE_EXPORT should be after "class" or "struct".
  This is the reason why you need to write BASE_EXPORT on each method unexpectedly.
- Patch Set #15, Line 19: class
  struct is enough?
- Patch Set #15, Line 21: BASE_EXPORT
  should be able to be removed by the fix I suggested.
- Patch Set #15, Line 22: BASE_EXPORT
  ditto
- Patch Set #15, Line 51: BASE_EXPORT
  same. should be after the "class".
  All BASE_EXPORT for methods can be removed.
- Patch Set #15, Line 52: base::
  base:: is not needed here?
File base/win/scoped_hstring.cc:
- Patch Set #13, Line 72: }
  Well, this has changed a lot since last CL. […]
  Ack
File base/win/scoped_hstring.cc:
- Patch Set #15, Line 67: WideToUTF8
  OK, now internal conversion errors are ignored behind WideToUTF8 implementation.
  That's fine with me, and we won't need a specific test for this.
File base/win/scoped_hstring_unittest.cc:

- Patch Set #13, Line 48: buffer = hstring.GetAsUTF8();

- What are good examples and what are the expectations in the test?
  since implementation was changed, I will take another look at the latest change, and ask in the latest patch set if it is still needed.
File media/midi/midi_manager_winrt.cc:
- Patch Set #15, Line 487: UTF8ToWide
  can we have a change to add one more constructor that takes const std::string& for this use case?

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 15, 2017, 9:18:10 AM9/15/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

16 comments:

File base/win/core_winrt_util.h:
- Patch Set #15, Line 10: #include <roapi.h>
  can this be moved to the .cc file?
File base/win/core_winrt_util.cc:

Patch Set #15, Line 31:

__try {
      hr = __HrLoadAllImportsForDll("api-ms-win-core-winrt-l1-1-0.dll");
    } __except (FilterVisualCPPExceptions(::GetExceptionCode())) {
      hr = E_FAIL;
    }

- That question feels somewhat above my pay grade. :) […]
  I don't understand the question. __HrLoadAllImportsForDll does a few things that I think are desirable:
  - allows us to use the loader's delayload mechanisms rather than writing our own.
  - allows us to force loading at a predicable point so we can be sure it happens on a thread that can do blocking I/O.
  The exception handling is one way to handle the case where the DLLs may or may not be present. If you know that you'll never call ResolveCoreWinRTDelayload on Win7, then you could remove the exception handling. If someone ever botches and calls functions when they shouldn't we'll see crashes and can fix them appropriately.
File base/win/core_winrt_util_unittest.cc:
- Patch Set #15, Line 7: #include "base/test/gtest_util.h"
  use "testing/gtest/include/gtest/gtest.h" instead of this
- Patch Set #15, Line 17: base::win::
  omit base::win::
File base/win/scoped_hstring.h:
- Patch Set #15, Line 19: class
  struct is enough?
  yes, please use struct and get rid of "public" on next line.
- Patch Set #15, Line 56: const base::StringPiece16&
  StringPiece should be passed by value as per comment in base/strings/string_piece.h
  (and omit base::)
- Patch Set #15, Line 62: base::
  omit base::
File base/win/scoped_hstring.cc:
- Patch Set #15, Line 29: bool ScopedHString::load_succeeded = false;
  nit:
  // static
- Patch Set #15, Line 47: ScopedHString::ScopedHString(HSTRING hstr) : ScopedGeneric(hstr) {}
  DCHECK(load_succeeded);
  in the body of the ctor
- Patch Set #15, Line 67: return WideToUTF8(Get().data());
```
please avoid extra string copies with:
  std::string result;
  const StringPiece16 wide_string = Get();
  WideToUTF8(wide_string.c_str(), wide_string.length(), &result);
  return result;
```
- Patch Set #15, Line 70: base::
  omit base:: here, too
- Patch Set #15, Line 71: return WindowsGetStringRawBuffer(get(), nullptr);
```
please avoid the extra strlen with:
  UINT32 length = 0;
  const wchar_t* buffer = ::WindowsGetStringRawBuffer(get(), &length);
  return StringPiece16(buffer, length);
```
File base/win/scoped_hstring_unittest.cc:
- Patch Set #15, Line 27: base::win::
  omit base::win::
File base/win/win_util.cc:
- Patch Set #15, Line 110: base::win::
  omit base::win::
  (i'll stop commenting on these, please fix 'em all up)
File media/midi/midi_manager_winrt.cc:
- Patch Set #15, Line 97: return id.GetAsUTF8();
```
nit: inline these
  return ScopedHString(result).GetAsUTF8();
```

- Patch Set #15, Line 487: UTF8ToWide
  can we have a change to add one more constructor that takes const std::string& for this use case?

- sure, but make it base::StringPiece rather than std::string.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 20, 2017, 9:03:28 AM9/20/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

this is looking really good. getting down to nuts-and-bolts.

10 comments:

File base/BUILD.gn:
- Patch Set #16, Line 87: # Please ignore the below delayloads, just testing to see if it makes
  can these be removed before landing?
File base/win/win_util.cc:
- Patch Set #16, Line 117: // This HSTRING is allocated on the heap and is leaked.
  why do we do this? i propose getting rid of this leak. i guess we did it before to avoid finding another function to free the string, but that's a non-issue now.
- Patch Set #16, Line 127: hr = ::RoGetActivationFactory(view_settings_guid,
  shouldn't this use the base::win:: variant?
File chrome/browser/notifications/notification_platform_bridge_win.cc:
- Patch Set #16, Line 41: unsigned int size
```
this is taking the size of the char array but isn't using it. you could construct the StringPiece16 explicitly to avoid the strlen:
  ScopedHString ref_class_name =
      ScopedHString::Create(base::StringPiece16(class_name, size - 1));
```
- Patch Set #16, Line 76: .c_str()
  omit .c_str() so that the implicit StringPiece16 from base::string16 ctor is used.
- Patch Set #16, Line 120: base::win::
  base::win:: not needed due to "using" declaration at top of file
- Patch Set #16, Line 147: .c_str()
  omit .c_str()
File media/midi/midi_manager_winrt.cc:
- Patch Set #16, Line 96: return ScopedHString(result).GetAsUTF8();
  this fixes HSTRING leaks in the old code! sweet!
- Patch Set #16, Line 484: .c_str()
  omit .c_str()
- Patch Set #16, Line 853: base::win::
  base::win:: not needed here, either.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 22, 2017, 4:47:15 AM9/22/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Greg Thompson, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

2 comments:

File base/win/scoped_hstring.h:
- Patch Set #16, Line 17:

// Scoped HSTRING class to maintain lifetime of HSTRINGs allocated with

// WindowsCreateString().
struct BASE_EXPORT ScopedHStringTraits {
  static HSTRING InvalidValue() { return nullptr; }
  static void Free(HSTRING hstr);
};

let me repeat.
can we make this traits in "namespace internal {}", as other similar classes in base do?
e.g, base/files/scoped_files

File media/midi/midi_manager_winrt.cc:
- Patch Set #16, Line 484: .c_str()
  omit . […]
  Can we just remove all conversion instead, e.g. ScopedHString::Create(dev_id) ?
  Since now Create can take StringPiece, passing std::string will work? I may miss something because I rarely use StringPiece.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Robert Liao (Gerrit)

unread,

Sep 22, 2017, 5:34:42 AM9/22/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

1 comment:

File base/win/core_winrt_util.cc:

- Patch Set #15, Line 31:

- ```
HRESULT hr = E_FAIL;
```


    __try {
      hr = __HrLoadAllImportsForDll("api-ms-win-core-winrt-l1-1-0.dll");
    } __except (FilterVisualCPPExceptions(::GetExceptionCode())) {

- I don't understand the question. […]
  The reason why I ask is this commits us to loading all functions including any future ones we use, potentially breaking downstream support.
  With the old-fashioned LoadLibrary+GetProcAddress, we could declare what we needed and if that subset was there, we would be good to go.
  If another file started using a new function unavailable on some versions of Windows, that would cause this to unnecessarily fail.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 25, 2017, 5:07:29 PM9/25/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

35 comments:

File base/BUILD.gn:

- Patch Set #16, Line 87: }

- can these be removed before landing?
  Gone for now. Although, I still need to figure out how to get transport_security_generator.exe to launch (it needs some delayload, I think, I'm just not sure which).
File base/win/core_winrt_util.h:
- Patch Set #15, Line 10: #include <windef.h>
  can this be moved to the . […]
  Done
File base/win/core_winrt_util.cc:

Patch Set #15, Line 31:

HRESULT hr = E_FAIL;
    __try {
      hr = __HrLoadAllImportsForDll("api-ms-win-core-winrt-l1-1-0.dll");
    } __except (FilterVisualCPPExceptions(::GetExceptionCode())) {

- The reason why I ask is this commits us to loading all functions including any future ones we use, p […]
  AFAIK, this is the only review question remaining on this CL. It would be good to get a resolution on this quickly, to avoid this dragging on too long. It's already been a fair amount of back and forth. :)
  My thinking on the subject is that we can cross that bridge when we get to it...?
File base/win/core_winrt_util_unittest.cc:
- Patch Set #15, Line 7: #include "base/win/scoped_com_initializer.h"
  use "testing/gtest/include/gtest/gtest. […]
  Done
- Patch Set #15, Line 17: GetVersion(
  omit base::win::
  Done
File base/win/scoped_hstring.h:

Patch Set #16, Line 17:


// Scoped HSTRING class to maintain lifetime of HSTRINGs allocated with
// WindowsCreateString().
struct BASE_EXPORT ScopedHStringTraits {
  static HSTRING InvalidValue() { return nullptr; }

- let me repeat. […]
  Sure! Done.
File base/win/scoped_hstring.h:
- Patch Set #15, Line 17:

- Traits can be inside internal namespace?

- I'm not sure I understand what you mean...
- Patch Set #15, Line 19: eateS

- yes, please use struct and get rid of "public" on next line.

- Done
- Patch Set #15, Line 19: // WindowsCr
  BASE_EXPORT should be after "class" or "struct". […]
  Bingo! That was it. Thanks!
- Patch Set #15, Line 21: static HSTRI

- should be able to be removed by the fix I suggested.

- Done
- Patch Set #15, Line 22: static void
  ditto
  Done
- Patch Set #15, Line 51: // HSTRING
  same. should be after the "class". […]
  Done
- Patch Set #15, Line 52: hr = W

- base:: is not needed here?

- Done
- Patch Set #15, Line 56: al::ScopedHStringTraits> {
  StringPiece should be passed by value as per comment in base/strings/string_piece.h […]
  Done

- Patch Set #15, Line 62:

- omit base::
  Done
File base/win/scoped_hstring.cc:

- Patch Set #15, Line 29:

- nit: […]
  Done
- Patch Set #15, Line 47: if (SUCCEEDED(hr))
  DCHECK(load_succeeded); […]
  Done
- Patch Set #15, Line 67: {
  OK, now internal conversion errors are ignored behind WideToUTF8 implementation. […]
  Ack
- Patch Set #15, Line 67: __try {
  please avoid extra string copies with: […]
  Done
- Patch Set #15, Line 70: hr = E
  omit base:: here, too
  Done

- Patch Set #15, Line 71: }

- please avoid the extra strlen with: […]
  Done
File base/win/scoped_hstring_unittest.cc:
- Patch Set #15, Line 27: GetVersion(
  omit base::win::
  Done
File base/win/win_util.cc:

- Patch Set #15, Line 110:

- omit base::win:: […]
  Done

- Patch Set #15, Line 121:

- Should this used the ScopedHSTRING helper?
  You mean like so?
File base/win/win_util.cc:
- Patch Set #16, Line 117: ScopedHString view_settings_guid = ScopedHString::Create(
  why do we do this? i propose getting rid of this leak. […]
  Done
- Patch Set #16, Line 127: // Avoid using GetForegroundWindow here and pass in the HWND of the window

- shouldn't this use the base::win:: variant?

- Patch Set #16, Line 41: unsigned int size

- this is taking the size of the char array but isn't using it. […]
  Done
- Patch Set #16, Line 76: ate(noti

- omit .c_str() so that the implicit StringPiece16 from base::string16 ctor is used.

- Done

- Patch Set #16, Line 120:
  base::win:: not needed due to "using" declaration at top of file

- Done

- Patch Set #16, Line 147:

- omit . […]
  Done
File media/midi/midi_manager_winrt.cc:

- Patch Set #15, Line 97: }

- nit: inline these […]
  Done
- Patch Set #15, Line 487: sync_op;

- sure, but make it base::StringPiece rather than std::string.

- Done
File media/midi/midi_manager_winrt.cc:
- Patch Set #16, Line 96: return ScopedHString(result).GetAsUTF8();

- this fixes HSTRING leaks in the old code! sweet!

- Ack

- Patch Set #16, Line 484:

- Can we just remove all conversion instead, e.g. ScopedHString::Create(dev_id) ? […]
  Done
- Patch Set #16, Line 853: OG(1) << "F

- base::win:: not needed here, either.

- Done

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 25, 2017, 5:09:07 PM9/25/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

1 comment:

File base/win/scoped_hstring.h:

- Patch Set #15, Line 17:

- I'm not sure I understand what you mean...
  Huh, that's confusing. Looks like part (all?) of my previous comments didn't go out last time I uploaded the CL. Anyway, this is done now.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 25, 2017, 8:04:34 PM9/25/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Takashi Toyoshima, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

1 comment:

File base/win/core_winrt_util.cc:

- Patch Set #15, Line 31:

- ```
decltype(&::RoGetActivationFactory) ResolveRoGetActivationFactory() {
  HMODULE combase_dll = GetComBaseModuleHandle();
  decltype(&::RoGetActivationFactory) get_factory_func = nullptr;
  if 
```
  AFAIK, this is the only review question remaining on this CL. […]
  OK, I've gone back to the LoadLibrary and GetProcAddr approach. Two reasons:
  1) I can't get the __HrLoadAllImportsForDll approach to work on Win7. It requires delayloading a handful of DLLs for Chrome, but what's worse is that transport_security_state_generator requires 27 (!) other DLLs to also be delayloaded (and I haven't even gotten to the telemetry test failures).
  It is hard to find out which DLLs are required (you need a Win7 machine where it repros) and it is really tedious to add a DLL to the list (build, repro, add one dll, build, repro, add one dll, etc). And once I was done with that, transport_security_state_generator crashed with "stack exhausted", while attempting to delayload an error handler dll in an endless loop.
  I worry this is brittle and will fail in spectacular ways in the future if MS decides to move functions around or add new versions of their DLLs. And the failures are going to be non-obvious (some exe failing to run during compile step with an error saying something is missing).
  2) There's also the forward compat problem Robert mentioned.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 26, 2017, 1:45:03 AM9/26/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

almost looks good, but there is one request.

2 comments:

File base/win/scoped_hstring.h:

Patch Set #19, Line 73:

  static bool load_succeeded;

  static HRESULT WindowsCreateString(const base::char16* src,
                                     uint32_t len,
                                     HSTRING* out_hstr);
  static HRESULT WindowsDeleteString(HSTRING hstr);
  static const base::char16* WindowsGetStringRawBuffer(HSTRING hstr,
                                                       uint32_t* out_len);

Would be better to declare them in anonymous namespace rather than private static. That would also allow you to remove friend struct of line 71.

File base/win/scoped_hstring.h:

- Patch Set #15, Line 17:

- Huh, that's confusing. […]
  Ack

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 26, 2017, 1:11:15 PM9/26/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

Takashi, I changed it as suggested (somewhat).

1 comment:

File base/win/scoped_hstring.h:

- Patch Set #19, Line 73:

- ```
}  // namespace win
}  // namespace base

#endif  // BASE_WIN_SCOPED_HSTRING_H_
```
  Would be better to declare them in anonymous namespace rather than private static. […]
  It was the first thing I tried but while it is in the anonymous namespace it clashes with the definition from the MS header. Would using the base::Internal namespace for this be ok?

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 26, 2017, 4:30:34 PM9/26/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

Please read all comments before acting on them -- I may have contradicted myself in later comments. :-)

19 comments:

File base/win/core_winrt_util.cc:
- Patch Set #19, Line 15: static HMODULE combase_dll = ::LoadLibrary(L"combase.dll");
```
nit: constipate all of these pieces of static data that are not meant to change. for example:
  static HMODULE const combase_dll = ...
```
- Patch Set #19, Line 21: signature* function_signature,
  a few comments:
  - "function_signature" doesn't really convey that this receives a pointer to the function. maybe "function_pointer" is more clear?
  - out params come after in params, so this should be the last param to the func
- Patch Set #19, Line 27: if (!*function_signature)
```
how about simply:
  return *function_pointer;
```
- Patch Set #19, Line 35: if (!GetProcAddress<decltype(&::RoGetActivationFactory)>(
  how about having GetProcAddress return the function pointer so that the extra local variable and conditional aren't needed?
- Patch Set #19, Line 55: HMODULE GetComBaseModuleHandle() {
  delete this one?
- Patch Set #19, Line 60: decltype(&::RoGetActivationFactory) PreloadRoGetActivationFactory() {
  these next two functions aren't declared in the .h, so move them into the unnamed namespace above.
- Patch Set #19, Line 83: static
  nit: remove "static" since PreloadRoActivateInstance does its own caching
- Patch Set #19, Line 90: bool ResolveCoreWinRTDelayload() {
  nit: move this above the two preceding functions to maintain the ordering in the .h
- Patch Set #19, Line 93: static const bool load_succeeded = []() {
  consider getting rid of the local static here since both of the functions called do their own caching. unless this is a super hot function, i don't think the cost of the local static is needed.

File base/win/core_winrt_util.cc:

Patch Set #20, Line 22: const std::string& function_name) {
use const char* to avoid needless std::string temporaries
Patch Set #20, Line 37: return nullptr;
nit: add braces for multi-line conditional

Patch Set #20, Line 60: decltype(&::RoGetActivationFactory) PreloadRoGetActivationFactory() {

are the Resolve* functions needed? it seems like less typing without:

void* LoadComBaseFunction(const char* function_name) {
  static HMODULE const handle = ::LoadLibrary(L"combase.dll");
  return handle ? ::GetProcAddress(handle, function_name) : nullptr;
}

decltype(&::RoActivateInstance) GetRoActivateInstanceFunction() {
  static decltype(&::RoActivateInstance) const function = []() {
    return reinterpret_cast<decltype(&::RoActivateInstance)>(
        LoadComBaseFunction("RoActivateInstance"));
  }();
  return function;
}

decltype(&::RoGetActivationFactory) GetRoGetActivationFactoryFunction() {
  static decltype(&::RoGetActivationFactory) const function = []() {
    return reinterpret_cast<decltype(&::RoGetActivationFactory)>(
        LoadComBaseFunction("RoGetActivationFactory"));
  }();
  return function;
}

then again, i don't really love the way there's a separate static for each function since each has its own overhead. maybe add a todo (or file a bug) to follow this CL with one that manages N loaded functions in a single static variable.

Patch Set #20, Line 76: Preload
i think GetFooBar makes more sense than PreloadFooBar since it returns the FooBar.

File base/win/scoped_hstring.h:
- Patch Set #20, Line 58: explicit ScopedHString(HSTRING hstr);
  maybe it should be obvious, but consider documenting that this takes ownership of |hstr|.
- Patch Set #20, Line 67: const
  now that i look over StringPiece again, i see that this returned instance doesn't need to be const -- methods on StringPiece can't modify the underlying string.
File base/win/scoped_hstring.cc:
- Patch Set #20, Line 86: HRESULT WindowsCreateString(const base::char16* src,
  these next three functions should be in an unnamed namespace since they are not used outside of this .cc file.
- Patch Set #20, Line 164: const StringPiece16 ScopedHString::Get() const {
  move this above GetAsUTF8 to match the ordering in the .h.
File base/win/win_util.cc:
- Patch Set #20, Line 105: HMODULE combase_dll = ::LoadLibrary(L"combase.dll");
  these four lines can be removed, no?
- Patch Set #20, Line 112: CHECK(false);
```
this doesn't make sense since CHECK will crash the process. do you mean:
  CHECK(preload_success);
in place of lines 111-114?
```

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 26, 2017, 8:38:47 PM9/26/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Greg Thompson, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

Greg, comments addressed/answered.

19 comments:

File base/win/core_winrt_util.cc:
- Patch Set #19, Line 15: static HMODULE const handle = ::LoadLibrary(L"combase.dll");
  nit: constipate all of these pieces of static data that are not meant to change. for example: […]
  Done
- Patch Set #19, Line 21: return reinterpret_cast<decltype(&::RoActivateInstance)>(
  a few comments: […]
  Done
- Patch Set #19, Line 27: decltype(&::RoGetActivationFactory) GetRoGetActivationFactoryFunction() {
  how about simply: […]
  Done
- Patch Set #19, Line 35: } // namespace
  how about having GetProcAddress return the function pointer so that the extra local variable and con […]
  Done

- Patch Set #19, Line 55:

- delete this one?
  Oh, hey... Yes!
- Patch Set #19, Line 60: return E_FAIL;

- these next two functions aren't declared in the .h, so move them into the unnamed namespace above.

- Obsolete.

- Patch Set #19, Line 83:

- nit: remove "static" since PreloadRoActivateInstance does its own caching
  Done

- Patch Set #19, Line 90:

- nit: move this above the two preceding functions to maintain the ordering in the . […]
  Done

- Patch Set #19, Line 93:

- consider getting rid of the local static here since both of the functions called do their own cachin […]
  Done
File base/win/core_winrt_util.cc:
- Patch Set #20, Line 22: LoadComBaseFunction("RoActivateInstance"));

- use const char* to avoid needless std::string temporaries

- Done
- Patch Set #20, Line 37: namespace base {

- nit: add braces for multi-line conditional

- Done
- Patch Set #20, Line 60: return E_FAIL;
  are the Resolve* functions needed? it seems like less typing without: […]
  I'm not sure what you mean by having a single static variable, but the rest is done.

- Patch Set #20, Line 76:

- i think GetFooBar makes more sense than PreloadFooBar since it returns the FooBar.
  Done
File base/win/scoped_hstring.h:
- Patch Set #20, Line 58: // Constructs a ScopedHString from an HSTRING, and takes ownership of |hstr|.

- maybe it should be obvious, but consider documenting that this takes ownership of |hstr|.

- Done

- Patch Set #20, Line 67:

- now that i look over StringPiece again, i see that this returned instance doesn't need to be const - […]
  Done
File base/win/scoped_hstring.cc:
- Patch Set #20, Line 86: // static
  these next three functions should be in an unnamed namespace since they are not used outside of this […]
  This was covered in a previous comment -- they conflict with the Microsoft definition (winstring.h file) if they are in the anonymous namespace.

- Patch Set #20, Line 164:

- move this above GetAsUTF8 to match the ordering in the .h.
  Done
File base/win/win_util.cc:

- Patch Set #20, Line 105: HMODULE combase_dll = ::LoadLibrary(L"combase.dll");
  these four lines can be removed, no?

- No, if you look at the old implementation it gracefully handles LoadLibrary failing by returning false and not CHECK()ing. I'm kind of leaning towards changing that behavior being outside the scope of this CL as I don't know what will happen if I do change it (well, I know some tests will fail on Win7 for starters). :)
  I think it is better to leave that kind of change out of this CL so that it won't suffer... :)

- Patch Set #20, Line 112: CHECK(false);

- this doesn't make sense since CHECK will crash the process. do you mean: […]
  Dunno. This is in line with the old implementation... Apparently someone really wanted to know if combase.dll could be loaded but the functions could not.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Robert Liao (Gerrit)

unread,

Sep 27, 2017, 12:14:39 AM9/27/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Greg Thompson, Takashi Toyoshima, Peter Beverloo, Commit Bot, chromium...@chromium.org

Thanks for performing this work. Approach looks good to me and I'm going OOO after this, so I'll lgtm this CL.

Patch set 21:Code-Review +1

8 comments:

File base/win/core_winrt_util.cc:
- Patch Set #21, Line 7: #include <delayimp.h>
  Nit: This doesn't look like it's needed anymore.
- Patch Set #21, Line 21:

reinterpret_cast<decltype(&::RoActivateInstance)>(
        LoadComBaseFunction("RoActivateInstance"));

- Since this is one statement, the lambda wrapping+invocation is no longer necessary and this expression can be directly assigned to |function|
File base/win/scoped_hstring.h:
- Patch Set #21, Line 33:
```
// This requires callers to use ResolveCoreWinRTStringDelayload() and checking
// its return value, *before* using ScopedHString.
```
  This might more direct:
  Callers MUST check the return value of ResolveCoreWinRTStringDelayLoad() before using ScopedHString.
File base/win/scoped_hstring.cc:
- Patch Set #21, Line 7: #include <delayimp.h>
  This looks like it can be removed.
- Patch Set #21, Line 22:
```
[]() {
    return reinterpret_cast<decltype(&::WindowsCreateString)>(
        LoadComBaseFunction("WindowsCreateString"));
  }();
```
  See earlier comment regarding one-statement lambdas+invocation.
- Patch Set #21, Line 112: load_succeeded
  Maybe rename this to local_load_succeeded to so it doesn't look like we aliased ScopedHString::load_succeeded.
File base/win/win_util.cc:
- Patch Set #21, Line 105:

  HMODULE combase_dll = ::LoadLibrary(L"combase.dll");

- ```
  if (!combase_dll)
    return false;
```
  This is no longer needed with the preloads below.

File media/midi/midi_manager_winrt.cc:

Patch Set #21, Line 853:

VLOG(1) << "Failed loading functions from combase.dll: "
            << PrintHr(HRESULT_FROM_WIN32(GetLastError()));

Update the string and remove the GetLastError() call.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 27, 2017, 2:23:05 AM9/27/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

3 comments:

File base/win/scoped_hstring.h:
- Patch Set #21, Line 72: load_succeeded
  this could be also in anonymous namespace in the implementation file.
File base/win/scoped_hstring.h:

Patch Set #19, Line 73:

};

}  // namespace win
}  // namespace base

#endif  // BASE_WIN_SCOPED_HSTRING_H_

- It was the first thing I tried but while it is in the anonymous namespace it clashes with the defini […]
  Did you try both top level anonymous, and base::(anonymous)? I guess, latter case could avoid the conflicts, or how about just using different function names that do not conflict.
File media/midi/midi_manager_winrt.cc:

Patch Set #21, Line 853:

VLOG(1) << "Failed loading functions from combase.dll: "
            << PrintHr(HRESULT_FROM_WIN32(GetLastError()));

Update the string and remove the GetLastError() call.

- +1. Since this message haven't given me useful information, I'm even fine with just removing this VLOG(1) completely.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 27, 2017, 1:47:20 PM9/27/17

to Daniel Cheng, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org

Finnur Thorarinsson has assigned a change to Daniel Cheng.

Move ComBase functions and HSTRING helpers to base/win.

Bug: 734095
Change-Id: Ib984cd7094cf4e075863d692acb59722114e3990
---
M base/BUILD.gn
A base/win/core_winrt_util.cc
A base/win/core_winrt_util.h
A base/win/core_winrt_util_unittest.cc
A base/win/scoped_hstring.cc
A base/win/scoped_hstring.h
A base/win/scoped_hstring_unittest.cc
M base/win/win_util.cc
M chrome/browser/notifications/notification_platform_bridge_win.cc
M media/midi/midi_manager_winrt.cc
10 files changed, 437 insertions(+), 317 deletions(-)

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 27, 2017, 1:47:41 PM9/27/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Daniel Cheng, Takashi Toyoshima, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

All done, I believe.

dcheng@: Mind doing an OWNERS check on BUILD.gn?

10 comments:

File base/win/core_winrt_util.cc:
- Patch Set #21, Line 7: #include <roapi.h>

- Nit: This doesn't look like it's needed anymore.

- Done

- Patch Set #21, Line 21:

- ```
oadComBaseFunction("RoActivateInstance"));
  return function;
```
  Since this is one statement, the lambda wrapping+invocation is no longer necessary and this expressi […]
  Done
File base/win/scoped_hstring.h:
- Patch Set #21, Line 33:
```
// Callers MUST check the return value of ResolveCoreWinRTStringDelayLoad()
// *before* using ScopedHString.
```
  This might more direct: […]
  Done
- Patch Set #21, Line 72: e win

- this could be also in anonymous namespace in the implementation file.

- Patch Set #19, Line 73:

- ```
}  // namespace base

#endif  // BASE_WIN_SCOPED_HSTRING_H_
```
  Did you try both top level anonymous, and base::(anonymous)? I guess, latter case could avoid the co […]
  Huh... Learn something every day (I didn't know you could have an anonymous namespace inside a named namespace). :)
  Seems to work fine!
File base/win/scoped_hstring.cc:
- Patch Set #21, Line 7: #include <winstring.h

- This looks like it can be removed.

- Done

- Patch Set #21, Line 22:


  static decltype(&::WindowsCreateString) const function =
      reinterpret_cast<decltype(&::WindowsCreateString)>(

- See earlier comment regarding one-statement lambdas+invocation.

- Done
- Patch Set #21, Line 112: etWindowsCreat
  Maybe rename this to local_load_succeeded to so it doesn't look like we aliased ScopedHString::load_ […]
  Obsolete, I suppose (the other variable changed name).
File base/win/win_util.cc:

Patch Set #21, Line 105:

  HMODULE combase_dll = ::LoadLibrary(L"combase.dll");
  if (!combase_dll)
    return false;

This is no longer needed with the preloads below.

- See my answer to Greg about the same thing (I'm just maintaining functionality with the old code, which gracefully handles combase.dll loading failures but hard CHECKs if the functions cannot be resolved).
File media/midi/midi_manager_winrt.cc:

- Patch Set #21, Line 853:

CompleteInitialization(Result::INITIALIZATION_ERROR);
    return;

+1. […]

Removed.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Daniel Cheng (Gerrit)

unread,

Sep 27, 2017, 3:32:07 PM9/27/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Robert Liao, Greg Thompson, Peter Beverloo, Commit Bot, chromium...@chromium.org

Patch set 22:Code-Review +1

1 comment:

File base/win/scoped_hstring_unittest.cc:
- Patch Set #22, Line 36: EXPECT_STREQ(kTestString1, base::UTF8ToWide(buffer).c_str());
  I think this can just be EXPECT_EQ if c_str() is dropped (here and elsewhere) Same comment applies to data() for the StringPiece comparison below.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 27, 2017, 4:44:31 PM9/27/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Daniel Cheng, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

11 comments:

File base/win/scoped_hstring.h:
- Patch Set #22, Line 11: #include "base/strings/string16.h"
  unused
- Patch Set #22, Line 64: consistent
  nit: consistent -> available?
File base/win/scoped_hstring.cc:
- Patch Set #22, Line 13: namespace {
  nit: is there a need for two unnamed namespaces? how about moving the contents of this one down into the one below?
- Patch Set #22, Line 52: _
  nit: no trailing underscore on local var
- Patch Set #22, Line 71: return nullptr;
```
add:
    *out_len = 0;
to this block
```
File base/win/scoped_hstring_unittest.cc:
- Patch Set #22, Line 10: #include "base/test/gtest_util.h"
  #include "testing/gtest/include/gtest/gtest.h" rather than this
- Patch Set #22, Line 19: static const wchar_t kTestString1[] = L"123";
  omit "static" on consts at namespace scope, and make this "constexpr" rather than "cost"
- Patch Set #22, Line 38: data
  fyi: data() is not guaranteed to return a pointer to a null-terminated string, so it shouldn't be used like this. c_str() would be correct here, but dcheng's comment above is better -- use EXPECT_EQ.

File base/win/win_util.cc:

Patch Set #22, Line 105: HMODULE combase_dll = ::LoadLibrary(L"combase.dll");
remove -- unused

Patch Set #22, Line 112: CHECK(false);

i think i commented on this already -- this isn't good. either:
  CHECK(preload_success);
if you want to crash the process, or remove the CHECK. since the old code returned false if combase.dll couldn't be found, i suspect you should return false in case of preload failure, so i'd just simplify all of this to:
  if (!ResolveCoreWinRTDelayload() ||
      !ScopedHString::ResolveCoreWinRTStringDelayload()) {
    return false;
  }

Patch Set #22, Line 116: HRESULT hr = E_FAIL;

remove this in favor of:
  HRESULT hr = base::win::...
on line 120

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 27, 2017, 6:14:59 PM9/27/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Greg Thompson, Daniel Cheng, Takashi Toyoshima, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

12 comments:

File base/win/scoped_hstring.h:
- Patch Set #22, Line 11: #include "base/strings/string_piece_forward.h"
  unused
  Done

- Patch Set #22, Line 64:

- nit: consistent -> available?
  Done
File base/win/scoped_hstring.cc:
- Patch Set #22, Line 13: namespace base {
  nit: is there a need for two unnamed namespaces? how about moving the contents of this one down into […]
  Done

- Patch Set #22, Line 52:

- nit: no trailing underscore on local var
  Done

- Patch Set #22, Line 71: }

- add: […]
  Done
File base/win/scoped_hstring_unittest.cc:
- Patch Set #22, Line 10: #include "base/win/core_winrt_util.h"
  #include "testing/gtest/include/gtest/gtest. […]
  Done
- Patch Set #22, Line 19: constexpr wchar_t kTestString1[] = L"123";

- omit "static" on consts at namespace scope, and make this "constexpr" rather than "cost"

- Done
- Patch Set #22, Line 36: EXPECT_EQ(kTestString1, base::UTF8ToWide(buffer));
  I think this can just be EXPECT_EQ if c_str() is dropped (here and elsewhere) Same comment applies t […]
  Done

- Patch Set #22, Line 38:

- fyi: data() is not guaranteed to return a pointer to a null-terminated string, so it shouldn't be us […]
  Done
File base/win/win_util.cc:
- Patch Set #22, Line 105: if (!ResolveCoreWinRTDelayload() ||
  remove -- unused
  Done
- Patch Set #22, Line 112: ScopedComPtr<IUIViewSettingsInterop> view_settings_interop;

- if you want to crash the process,

- Again, this isn't *my* code, so I don't really want anything here. :) I was just trying to stick to a cleanup that didn't change semantics. But whatever. I give up. :)
- Patch Set #22, Line 116: return false;
  remove this in favor of: […]
  Done

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Sep 28, 2017, 2:24:40 AM9/28/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Greg Thompson, Daniel Cheng, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

thank you for hard works!
all LGTM, and set +1 for media/midi

Patch set 24:Code-Review +1

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Greg Thompson (Gerrit)

unread,

Sep 28, 2017, 4:55:18 AM9/28/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Takashi Toyoshima, Daniel Cheng, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

LGTM! Thanks for the hard work here.

Patch set 24:Code-Review +1

1 comment:

File base/win/win_util.cc:
- Patch Set #22, Line 112: ScopedComPtr<IUIViewSettingsInterop> view_settings_interop;
  > if you want to crash the process, […]
  Apologies for the mis-attribution. :-) The original code didn't make sense, so may as well clean it up now. I think what you have in patch set 24 preserves the *intent* of the old code. I'm not sure that we want to crash the process if combase.dll or the required functions suddenly don't exist in a future version of Windows. It'd be unfortunate if MS pushed an update that caused all Chromen to crash at startup on Win10+.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 28, 2017, 1:51:58 PM9/28/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Greg Thompson, Takashi Toyoshima, Daniel Cheng, Robert Liao, Peter Beverloo, Commit Bot, chromium...@chromium.org

Patch set 24:Commit-Queue +2

1 comment:

File base/win/win_util.cc:

- Patch Set #22, Line 112: ScopedComPtr<IUIViewSettingsInterop> view_settings_interop;

- Apologies for the mis-attribution. […]
  No problem. And sorry if I sounded snarky yesterday -- I was a bit tired, but got better sleep last night so things are looking up. :)
  Looks like everyone has given green light, so I'll check in.
  Thanks all!

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Sep 28, 2017, 2:05:21 PM9/28/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Greg Thompson, Takashi Toyoshima, Daniel Cheng, Robert Liao, Peter Beverloo, chromium...@chromium.org

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/551359)

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Peter Beverloo (Gerrit)

unread,

Sep 28, 2017, 2:06:39 PM9/28/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Peter Beverloo, Greg Thompson, Takashi Toyoshima, Daniel Cheng, Robert Liao, Commit Bot, chromium...@chromium.org

I get to give another green light :)

Thanks for the very thorough reviews everyone, and thanks for the hard work on making this happen Finnur!

Patch set 24:Code-Review +1

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 28, 2017, 5:18:26 PM9/28/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Peter Beverloo, Greg Thompson, Takashi Toyoshima, Daniel Cheng, Robert Liao, Commit Bot, chromium...@chromium.org

I get to give another green light :)

Thanks Peter!

Patch set 24:Commit-Queue +2

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Sep 28, 2017, 5:31:46 PM9/28/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Peter Beverloo, Greg Thompson, Takashi Toyoshima, Daniel Cheng, Robert Liao, chromium...@chromium.org

Commit Bot merged this change.

Approvals:
  Daniel Cheng: Looks good to me
  Peter Beverloo: Looks good to me
  Takashi Toyoshima: Looks good to me
  Robert Liao: Looks good to me
  Greg Thompson: Looks good to me
  Finnur Thorarinsson: Commit

Move ComBase functions and HSTRING helpers to base/win.

Bug: 734095
Change-Id: Ib984cd7094cf4e075863d692acb59722114e3990

Reviewed-on: https://chromium-review.googlesource.com/649528
Reviewed-by: Takashi Toyoshima <toyo...@chromium.org>
Reviewed-by: Greg Thompson <g...@chromium.org>
Reviewed-by: Peter Beverloo <pe...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Robert Liao <rob...@chromium.org>
Commit-Queue: Finnur Thorarinsson <fin...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505163}


---
M base/BUILD.gn
A base/win/core_winrt_util.cc
A base/win/core_winrt_util.h
A base/win/core_winrt_util_unittest.cc
A base/win/scoped_hstring.cc
A base/win/scoped_hstring.h
A base/win/scoped_hstring_unittest.cc
M base/win/win_util.cc
M chrome/browser/notifications/notification_platform_bridge_win.cc
M media/midi/midi_manager_winrt.cc

10 files changed, 428 insertions(+), 319 deletions(-)

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Sigurður Ásgeirsson (Gerrit)

unread,

Sep 29, 2017, 9:52:01 AM9/29/17

to Finnur Thorarinsson, Commit Bot, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Peter Beverloo, Greg Thompson, Takashi Toyoshima, Daniel Cheng, Robert Liao, chromium...@chromium.org

This is DCHECKing due to IO restrictions on the main thread. See https://crbug.com/770193.

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Geoff Lang (Gerrit)

unread,

Sep 29, 2017, 12:01:03 PM9/29/17

to Finnur Thorarinsson, Commit Bot, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Sigurður Ásgeirsson, Peter Beverloo, Greg Thompson, Takashi Toyoshima, Daniel Cheng, Robert Liao, chromium...@chromium.org

This is also causing failures on the Windows 10 bots on the GPU FYI waterfall. Example: https://build.chromium.org/p/chromium.gpu.fyi/builders/Win10%20Release%20%28NVIDIA%20Quadro%20P400%29/builds/1746

To view, visit change 649528. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 29, 2017, 2:05:24 PM9/29/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, chromium...@chromium.org, Peter Beverloo

This change is ready for review.

Patch set 1:Commit-Queue +2

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Sep 29, 2017, 4:06:04 PM9/29/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, chromium...@chromium.org, Peter Beverloo

Try jobs failed on following builders:

  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 29, 2017, 4:07:41 PM9/29/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Commit Bot, chromium...@chromium.org, Peter Beverloo

Patch set 1:Commit-Queue +2

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Sep 29, 2017, 6:58:50 PM9/29/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, chromium...@chromium.org, Peter Beverloo

Try jobs failed on following builders:

  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 29, 2017, 7:29:17 PM9/29/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Commit Bot, chromium...@chromium.org, Peter Beverloo

Patch set 1:Commit-Queue +2

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Sep 29, 2017, 9:31:16 PM9/29/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, chromium...@chromium.org, Peter Beverloo

Try jobs failed on following builders:

  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 30, 2017, 1:49:59 PM9/30/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Commit Bot, chromium...@chromium.org, Peter Beverloo

Patch set 1:Commit-Queue +2

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Sep 30, 2017, 3:50:29 PM9/30/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, chromium...@chromium.org, Peter Beverloo

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))

  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Finnur Thorarinsson (Gerrit)

unread,

Sep 30, 2017, 6:06:18 PM9/30/17

to awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Commit Bot, chromium...@chromium.org, Peter Beverloo

Patch set 1:Commit-Queue +2

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Sep 30, 2017, 7:36:56 PM9/30/17

to Finnur Thorarinsson, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, chromium...@chromium.org, Peter Beverloo

Commit Bot merged this change.

Approvals:
  Finnur Thorarinsson: Looks good to me; Commit

Move ComBase functions and HSTRING helpers to base/win.

This is a reland of:
https://chromium-review.googlesource.com/c/chromium/src/+/649528/
... which failed because the CL exposed a caller using IO on the main thread.

I'm relanding without the asserts, because the underlying problem exists with
and without my CL (which is simply a cleanup job), so that problem should be
handled separately.

Bug: 734095
Change-Id: I79b809fede6097756e365f24f128db82977278a4
TBR: toyoshim, grt, peter, dcheng, robliao
Reviewed-on: https://chromium-review.googlesource.com/693094
Reviewed-by: Finnur Thorarinsson <fin...@chromium.org>
Commit-Queue: Finnur Thorarinsson <fin...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505478}


---
M base/BUILD.gn
A base/win/core_winrt_util.cc
A base/win/core_winrt_util.h
A base/win/core_winrt_util_unittest.cc
A base/win/scoped_hstring.cc
A base/win/scoped_hstring.h
A base/win/scoped_hstring_unittest.cc
M base/win/win_util.cc
M chrome/browser/notifications/notification_platform_bridge_win.cc
M media/midi/midi_manager_winrt.cc

10 files changed, 425 insertions(+), 319 deletions(-)

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Oct 2, 2017, 3:13:47 AM10/2/17

to Finnur Thorarinsson, Commit Bot, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, chromium...@chromium.org, Peter Beverloo

did you check which caller caused the issue?

Patch set 2:Code-Review +1

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Oct 2, 2017, 4:25:17 AM10/2/17

to Finnur Thorarinsson, Commit Bot, awdf+...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, mlamouri+watch...@chromium.org, toyosh...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, chromium...@chromium.org, Peter Beverloo

Aha, please ignore my previous question. I found it.

1 comment:

File base/win/core_winrt_util.cc:
- Patch Set #2, Line 36: 770193
  Ah, I see. The bug mentioned details about the problematic caller.

To view, visit change 693094. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu