Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2122303002:
Revive experiment to isolate shell operations. (Closed)
|
Created:
4 years, 5 months ago by Patrick Monette Modified:
3 years, 6 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIsolate shell operations to the utility process.
Shell operations can cause 3rd-party shell extensions to be
loaded into the caller's process. The utility process protects the browser process from potential instability.
Also convert the IPC mechanism to Mojo.
BUG=73098
Committed: https://crrev.com/b7faec8d94ef286d2b92144c2836b170c2cdea95
Cr-Commit-Position: refs/heads/master@{#405270}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Nits #
Total comments: 2
Patch Set 3 : HandleToUint32 #Patch Set 4 : Rebase #Patch Set 5 : Helper function #Patch Set 6 : Trying stuff #Patch Set 7 : Try 2 #Patch Set 8 : Try 3 #Patch Set 9 : try 4 #Patch Set 10 : Rebase #Patch Set 11 : Rebase #Messages
Total messages: 52 (20 generated)
Description was changed from ========== Fully commit to isolate shell operations to the utility process. Removes the experiment and convert the IPC mechanism to Mojo. BUG=TODO ========== to ========== Isolate shell operations to the utility process. Removes the experiment and convert the IPC mechanism to Mojo. BUG=TODO ==========
Description was changed from ========== Isolate shell operations to the utility process. Removes the experiment and convert the IPC mechanism to Mojo. BUG=TODO ========== to ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Removes the experiment and convert the IPC mechanism to Mojo. BUG=TODO ==========
Description was changed from ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Removes the experiment and convert the IPC mechanism to Mojo. BUG=TODO ========== to ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Also convert the IPC mechanism to Mojo. BUG=TODO ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Also convert the IPC mechanism to Mojo. BUG=TODO ========== to ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Also convert the IPC mechanism to Mojo. BUG=73098 ==========
pmonette@chromium.org changed reviewers: + sky@chromium.org
sky@ PTAL. I'm reviving an old experiment because I think it's worth avoiding the crashes from 3rd-party shell extensions but I don't have the data to back up this claim (The last owner moved on).
Just a handful of nits. You'll need a security review for the mojom changes. https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... File chrome/browser/win/chrome_select_file_dialog_factory.cc (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... chrome/browser/win/chrome_select_file_dialog_factory.cc:51: void OnGetOpenFileNameResult(const base::FilePath& directory, nit: more often than not we use past test, e.g. OnGotOptionFileNames, or OnDidGetOpenFileNames. https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... chrome/browser/win/chrome_select_file_dialog_factory.cc:71: DWORD one_based_filter_index_; Please initialize this. https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... File chrome/browser/win/chrome_select_file_dialog_factory.h (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... chrome/browser/win/chrome_select_file_dialog_factory.h:17: ChromeSelectFileDialogFactory(); nit: don't inline the destructor. https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... File chrome/common/shell_handler_win.mojom (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.mojom:16: // Instructs the utility process to invoke GetOpenFileName. |owner| is the document what owner is, e.g. an HWND. https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.mojom:17: // parent of the modal dialog, |flags| are OFN_* flags. |filter| constrains the nit: < 80 https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.mojom:19: // to be displayed and the file to be initially selected. Document what the result is if the user cancels the selection. https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.mojom:22: DoGetSaveFileName(uint64 owner, uint32 flags, FileExtensionFilters filters, uint32 one_based_filter_index, mojo.common.mojom.FilePath initial_directory, mojo.common.mojom.FilePath suggested_filename, mojo.common.mojom.FilePath default_extension) => (mojo.common.mojom.FilePath path, uint32 one_based_filter_index); Document what one_based_filter_index means. https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... File chrome/common/shell_handler_win.typemap (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.typemap:10: type_mappings = [ "mojom.FileExtensionFilters=std::vector<std::tuple<base::string16, base::string16>>" ] nit: < 80 https://codereview.chromium.org/2122303002/diff/20001/chrome/utility/shell_ha... File chrome/utility/shell_handler_impl_win.cc (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/utility/shell_ha... chrome/utility/shell_handler_impl_win.cc:244: if (filenames.size()) { nit: no {}, also !empty()
Thanks! I've fixed the nits. dcheng@ PTAL at chrome_utility_messages.h and shell_handler_win.mojom for security POV. https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... File chrome/browser/win/chrome_select_file_dialog_factory.cc (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... chrome/browser/win/chrome_select_file_dialog_factory.cc:51: void OnGetOpenFileNameResult(const base::FilePath& directory, On 2016/07/08 15:28:20, sky wrote: > nit: more often than not we use past test, e.g. OnGotOptionFileNames, or > OnDidGetOpenFileNames. Done. https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... chrome/browser/win/chrome_select_file_dialog_factory.cc:71: DWORD one_based_filter_index_; On 2016/07/08 15:28:20, sky wrote: > Please initialize this. Done. https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... File chrome/browser/win/chrome_select_file_dialog_factory.h (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/browser/win/chro... chrome/browser/win/chrome_select_file_dialog_factory.h:17: ChromeSelectFileDialogFactory(); On 2016/07/08 15:28:20, sky wrote: > nit: don't inline the destructor. Done. https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... File chrome/common/shell_handler_win.mojom (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.mojom:16: // Instructs the utility process to invoke GetOpenFileName. |owner| is the On 2016/07/08 15:28:20, sky wrote: > document what owner is, e.g. an HWND. Done. https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.mojom:17: // parent of the modal dialog, |flags| are OFN_* flags. |filter| constrains the On 2016/07/08 15:28:21, sky wrote: > nit: < 80 Done. Any suggestions on the formatting? Maybe the formatter should be able to handle mojom files :) https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.mojom:19: // to be displayed and the file to be initially selected. On 2016/07/08 15:28:21, sky wrote: > Document what the result is if the user cancels the selection. Done. https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.mojom:22: DoGetSaveFileName(uint64 owner, uint32 flags, FileExtensionFilters filters, uint32 one_based_filter_index, mojo.common.mojom.FilePath initial_directory, mojo.common.mojom.FilePath suggested_filename, mojo.common.mojom.FilePath default_extension) => (mojo.common.mojom.FilePath path, uint32 one_based_filter_index); On 2016/07/08 15:28:21, sky wrote: > Document what one_based_filter_index means. Done. https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... File chrome/common/shell_handler_win.typemap (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.typemap:10: type_mappings = [ "mojom.FileExtensionFilters=std::vector<std::tuple<base::string16, base::string16>>" ] On 2016/07/08 15:28:21, sky wrote: > nit: < 80 Done. https://codereview.chromium.org/2122303002/diff/20001/chrome/utility/shell_ha... File chrome/utility/shell_handler_impl_win.cc (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/utility/shell_ha... chrome/utility/shell_handler_impl_win.cc:244: if (filenames.size()) { On 2016/07/08 15:28:21, sky wrote: > nit: no {}, also !empty() Done.
pmonette@chromium.org changed reviewers: + dcheng@chromium.org
Forgot to add dcheng@...
LGTM https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... File chrome/common/shell_handler_win.mojom (right): https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... chrome/common/shell_handler_win.mojom:17: // parent of the modal dialog, |flags| are OFN_* flags. |filter| constrains the On 2016/07/08 19:36:10, Patrick Monette wrote: > On 2016/07/08 15:28:21, sky wrote: > > nit: < 80 > > Done. Any suggestions on the formatting? Maybe the formatter should be able to > handle mojom files :) I try to follow what I think git cl format would do. By that I mean a single argument per line, wrapping at 80...
On 2016/07/08 20:47:15, sky wrote: > LGTM > > https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... > File chrome/common/shell_handler_win.mojom (right): > > https://codereview.chromium.org/2122303002/diff/20001/chrome/common/shell_han... > chrome/common/shell_handler_win.mojom:17: // parent of the modal dialog, |flags| > are OFN_* flags. |filter| constrains the > On 2016/07/08 19:36:10, Patrick Monette wrote: > > On 2016/07/08 15:28:21, sky wrote: > > > nit: < 80 > > > > Done. Any suggestions on the formatting? Maybe the formatter should be able to > > handle mojom files :) > > I try to follow what I think git cl format would do. By that I mean a single > argument per line, wrapping at 80... Yeah that's what I did but I wasn't sure about the callback syntax formatting.
dcheng@chromium.org changed reviewers: + wfh@chromium.org
The changes seem reasonable to me, but I have a Windows question for +wfh: The reinterpret_casts between uint64_t and HWND are a bit sad: the old code looked a bit nicer in this regard. I wonder if it'd be better if it was typemapped and using the appropriate conversion functions (HandleToLong, LogToHandle) instead. https://codereview.chromium.org/2122303002/diff/40001/chrome/browser/win/chro... File chrome/browser/win/chrome_select_file_dialog_factory.cc (right): https://codereview.chromium.org/2122303002/diff/40001/chrome/browser/win/chro... chrome/browser/win/chrome_select_file_dialog_factory.cc:27: "IsolateShellOperations", base::FEATURE_DISABLED_BY_DEFAULT}; Per https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a..., this should be ("IsolateShellOperations", base::FEATURE_DISABLED_BY_DEFAULT)
https://codereview.chromium.org/2122303002/diff/40001/chrome/browser/win/chro... File chrome/browser/win/chrome_select_file_dialog_factory.cc (right): https://codereview.chromium.org/2122303002/diff/40001/chrome/browser/win/chro... chrome/browser/win/chrome_select_file_dialog_factory.cc:27: "IsolateShellOperations", base::FEATURE_DISABLED_BY_DEFAULT}; On 2016/07/11 09:00:46, dcheng wrote: > Per > https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a..., > this should be ("IsolateShellOperations", base::FEATURE_DISABLED_BY_DEFAULT) Doesn't work as base::Feature doesn't have a constructor defined. I used {} as this is what feature_list_unittest.cc uses.
On 2016/07/11 09:00:46, dcheng wrote: > The changes seem reasonable to me, but I have a Windows question for +wfh: > > The reinterpret_casts between uint64_t and HWND are a bit sad: the old code > looked a bit nicer in this regard. I wonder if it'd be better if it was > typemapped and using the appropriate conversion functions (HandleToLong, > LogToHandle) instead. HWND like HANDLE is guaranteed to fit in a 32-bit value https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203.aspx So you should be using base::win::HandleToUint32 here and using a 32-bit mojo type.
Done. I doesn't seem like there is an equivalent Uint32ToHandle() to cast back tho.
This all lgtm from a win perspective. On 2016/07/12 19:38:21, Patrick Monette wrote: > Done. I doesn't seem like there is an equivalent Uint32ToHandle() to cast back > tho. ...yup it looks like there's code in ppapi/shared_impl/platform_file.h which returns a base::PlatformHandle which is HANDLE, which is HWND... I'm wondering if the static_cast<intptr_t> is needed or not though. I think the reinterpret_cast as you have it should be fine as long as clang doesn't complain.
On 2016/07/12 19:50:06, Will Harris wrote: > This all lgtm from a win perspective. > > On 2016/07/12 19:38:21, Patrick Monette wrote: > > Done. I doesn't seem like there is an equivalent Uint32ToHandle() to cast back > > tho. > > ...yup it looks like there's code in ppapi/shared_impl/platform_file.h which > returns a base::PlatformHandle which is HANDLE, which is HWND... I'm wondering > if the static_cast<intptr_t> is needed or not though. I think the > reinterpret_cast as you have it should be fine as long as clang doesn't > complain. 1) Why don't we use the MS helpers for this? It looks like that's what ParamTraits<HANDLE> uses: https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1143 and https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1152 2) I'd prefer it if we just created a small mojo struct to wrap HANDLE and typemap it to/from HANDLE. Is that possible / reasonable here?
On 2016/07/13 02:08:36, dcheng wrote: > On 2016/07/12 19:50:06, Will Harris wrote: > > This all lgtm from a win perspective. > > > > On 2016/07/12 19:38:21, Patrick Monette wrote: > > > Done. I doesn't seem like there is an equivalent Uint32ToHandle() to cast > back > > > tho. > > > > ...yup it looks like there's code in ppapi/shared_impl/platform_file.h which > > returns a base::PlatformHandle which is HANDLE, which is HWND... I'm wondering > > if the static_cast<intptr_t> is needed or not though. I think the > > reinterpret_cast as you have it should be fine as long as clang doesn't > > complain. > > 1) Why don't we use the MS helpers for this? It looks like that's what > ParamTraits<HANDLE> uses: > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1143 and > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1152 > > 2) I'd prefer it if we just created a small mojo struct to wrap HANDLE and > typemap it to/from HANDLE. Is that possible / reasonable here? it's unusual to transfer HANDLE over mojo because they are normally per-process - it's just HWND that are global (per winsta anyway) so I don't think there is too much utility (and perhaps there is danger) in exposing this more widely in mojo. Normally HANDLE would be carried cross process using the handle attachment broker (is there a handle attachment broker for mojo, I have no idea). https://msdn.microsoft.com/en-us/library/windows/desktop/aa384267.aspx is all the funcs, HandleToLong and LongToHandle look pretty much like the ones already in use... TL;DR; I don't mind what is used here.
On 2016/07/13 16:00:28, Will Harris wrote: > On 2016/07/13 02:08:36, dcheng wrote: > > On 2016/07/12 19:50:06, Will Harris wrote: > > > This all lgtm from a win perspective. > > > > > > On 2016/07/12 19:38:21, Patrick Monette wrote: > > > > Done. I doesn't seem like there is an equivalent Uint32ToHandle() to cast > > back > > > > tho. > > > > > > ...yup it looks like there's code in ppapi/shared_impl/platform_file.h which > > > returns a base::PlatformHandle which is HANDLE, which is HWND... I'm > wondering > > > if the static_cast<intptr_t> is needed or not though. I think the > > > reinterpret_cast as you have it should be fine as long as clang doesn't > > > complain. > > > > 1) Why don't we use the MS helpers for this? It looks like that's what > > ParamTraits<HANDLE> uses: > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1143 and > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1152 > > > > 2) I'd prefer it if we just created a small mojo struct to wrap HANDLE and > > typemap it to/from HANDLE. Is that possible / reasonable here? > > it's unusual to transfer HANDLE over mojo because they are normally per-process > - it's just HWND that are global (per winsta anyway) so I don't think there is > too much utility (and perhaps there is danger) in exposing this more widely in > mojo. Normally HANDLE would be carried cross process using the handle attachment > broker (is there a handle attachment broker for mojo, I have no idea). Ah, OK. So you see reinterpret_cast as a feature, not a problem here by raising the bar. That seems like a decent reason to me. > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa384267.aspx is all > the funcs, HandleToLong and LongToHandle look pretty much like the ones already > in use... > > TL;DR; I don't mind what is used here. Mostly this is just motivated by a desire to see fewer reinterpret_casts. Mojo does have a WrappedPlatformHandle thingie: I guess it uses DuplicateHandle(). Is it an error to use DuplicateHandle() on a HWND, given their global-ish nature?
On 2016/07/13 16:19:48, dcheng wrote: > On 2016/07/13 16:00:28, Will Harris wrote: > > On 2016/07/13 02:08:36, dcheng wrote: > > > On 2016/07/12 19:50:06, Will Harris wrote: > > > > This all lgtm from a win perspective. > > > > > > > > On 2016/07/12 19:38:21, Patrick Monette wrote: > > > > > Done. I doesn't seem like there is an equivalent Uint32ToHandle() to > cast > > > back > > > > > tho. > > > > > > > > ...yup it looks like there's code in ppapi/shared_impl/platform_file.h > which > > > > returns a base::PlatformHandle which is HANDLE, which is HWND... I'm > > wondering > > > > if the static_cast<intptr_t> is needed or not though. I think the > > > > reinterpret_cast as you have it should be fine as long as clang doesn't > > > > complain. > > > > > > 1) Why don't we use the MS helpers for this? It looks like that's what > > > ParamTraits<HANDLE> uses: > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1143 > and > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1152 > > > > > > 2) I'd prefer it if we just created a small mojo struct to wrap HANDLE and > > > typemap it to/from HANDLE. Is that possible / reasonable here? > > > > it's unusual to transfer HANDLE over mojo because they are normally > per-process > > - it's just HWND that are global (per winsta anyway) so I don't think there is > > too much utility (and perhaps there is danger) in exposing this more widely in > > mojo. Normally HANDLE would be carried cross process using the handle > attachment > > broker (is there a handle attachment broker for mojo, I have no idea). > > Ah, OK. So you see reinterpret_cast as a feature, not a problem here by raising > the bar. That seems like a decent reason to me. > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa384267.aspx is all > > the funcs, HandleToLong and LongToHandle look pretty much like the ones > already > > in use... > > > > TL;DR; I don't mind what is used here. > > Mostly this is just motivated by a desire to see fewer reinterpret_casts. Mojo > does have a WrappedPlatformHandle thingie: I guess it uses DuplicateHandle(). Is > it an error to use DuplicateHandle() on a HWND, given their global-ish nature? Yes we normally wouldn't duplicatehandle on an HWND because all of Chrome runs in the same window station so it's effectively global. That's not the same for HANDLE where it's typically per-process so needs to be brokered via DuplicateHandle to the target process. It does look like from looking at the mojo WrappedPlatformHandle code, it does the same job as the old IPC attachmentbroker. Either way, it's not needed here for an HWND so I think using the reinterpret_cast/static_cast (or the ATL functions, but I'd rather not, they are C code and it seems to make sense to use C++ casts here). Hope I'm making sense and not rambling here.
On 2016/07/13 16:19:48, dcheng wrote: > On 2016/07/13 16:00:28, Will Harris wrote: > > On 2016/07/13 02:08:36, dcheng wrote: > > > On 2016/07/12 19:50:06, Will Harris wrote: > > > > This all lgtm from a win perspective. > > > > > > > > On 2016/07/12 19:38:21, Patrick Monette wrote: > > > > > Done. I doesn't seem like there is an equivalent Uint32ToHandle() to > cast > > > back > > > > > tho. > > > > > > > > ...yup it looks like there's code in ppapi/shared_impl/platform_file.h > which > > > > returns a base::PlatformHandle which is HANDLE, which is HWND... I'm > > wondering > > > > if the static_cast<intptr_t> is needed or not though. I think the > > > > reinterpret_cast as you have it should be fine as long as clang doesn't > > > > complain. > > > > > > 1) Why don't we use the MS helpers for this? It looks like that's what > > > ParamTraits<HANDLE> uses: > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1143 > and > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1152 > > > > > > 2) I'd prefer it if we just created a small mojo struct to wrap HANDLE and > > > typemap it to/from HANDLE. Is that possible / reasonable here? > > > > it's unusual to transfer HANDLE over mojo because they are normally > per-process > > - it's just HWND that are global (per winsta anyway) so I don't think there is > > too much utility (and perhaps there is danger) in exposing this more widely in > > mojo. Normally HANDLE would be carried cross process using the handle > attachment > > broker (is there a handle attachment broker for mojo, I have no idea). > > Ah, OK. So you see reinterpret_cast as a feature, not a problem here by raising > the bar. That seems like a decent reason to me. > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa384267.aspx is all > > the funcs, HandleToLong and LongToHandle look pretty much like the ones > already > > in use... > > > > TL;DR; I don't mind what is used here. > > Mostly this is just motivated by a desire to see fewer reinterpret_casts. Mojo > does have a WrappedPlatformHandle thingie: I guess it uses DuplicateHandle(). Is > it an error to use DuplicateHandle() on a HWND, given their global-ish nature? It is a problem. HWND should really be seen as a different type from HANDLE (case in point, using LongToHandle() instead of reinterpret_cast gives the error c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): error C2664: 'ui::win::OpenFileName::OpenFileName(const ui::win::OpenFileName &)': cannot convert argument 1 from 'void *' to 'HWND' c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): note: Conversion from 'void*' to pointer to non-'void' requires an explicit cast
On 2016/07/13 16:28:04, Patrick Monette wrote: > On 2016/07/13 16:19:48, dcheng wrote: > > On 2016/07/13 16:00:28, Will Harris wrote: > > > On 2016/07/13 02:08:36, dcheng wrote: > > > > On 2016/07/12 19:50:06, Will Harris wrote: > > > > > This all lgtm from a win perspective. > > > > > > > > > > On 2016/07/12 19:38:21, Patrick Monette wrote: > > > > > > Done. I doesn't seem like there is an equivalent Uint32ToHandle() to > > cast > > > > back > > > > > > tho. > > > > > > > > > > ...yup it looks like there's code in ppapi/shared_impl/platform_file.h > > which > > > > > returns a base::PlatformHandle which is HANDLE, which is HWND... I'm > > > wondering > > > > > if the static_cast<intptr_t> is needed or not though. I think the > > > > > reinterpret_cast as you have it should be fine as long as clang doesn't > > > > > complain. > > > > > > > > 1) Why don't we use the MS helpers for this? It looks like that's what > > > > ParamTraits<HANDLE> uses: > > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1143 > > and > > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1152 > > > > > > > > 2) I'd prefer it if we just created a small mojo struct to wrap HANDLE and > > > > typemap it to/from HANDLE. Is that possible / reasonable here? > > > > > > it's unusual to transfer HANDLE over mojo because they are normally > > per-process > > > - it's just HWND that are global (per winsta anyway) so I don't think there > is > > > too much utility (and perhaps there is danger) in exposing this more widely > in > > > mojo. Normally HANDLE would be carried cross process using the handle > > attachment > > > broker (is there a handle attachment broker for mojo, I have no idea). > > > > Ah, OK. So you see reinterpret_cast as a feature, not a problem here by > raising > > the bar. That seems like a decent reason to me. > > > > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa384267.aspx is > all > > > the funcs, HandleToLong and LongToHandle look pretty much like the ones > > already > > > in use... > > > > > > TL;DR; I don't mind what is used here. > > > > Mostly this is just motivated by a desire to see fewer reinterpret_casts. Mojo > > does have a WrappedPlatformHandle thingie: I guess it uses DuplicateHandle(). > Is > > it an error to use DuplicateHandle() on a HWND, given their global-ish nature? > > It is a problem. HWND should really be seen as a different type from HANDLE > (case in point, using LongToHandle() instead of reinterpret_cast gives the error > > c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): error C2664: > 'ui::win::OpenFileName::OpenFileName(const ui::win::OpenFileName &)': cannot > convert argument 1 from 'void *' to 'HWND' > c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): note: > Conversion from 'void*' to pointer to non-'void' requires an explicit cast LGTM based on wfh's lgtm. That being said, I'm a bit surprised at the last point: I'm pretty sure we're using ParamTraits<HANDLE> for HWNDs, and https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx claims HWND is typedef'ed to HANDLE, so I would have expected it to Just Work. Very mysterious...
On 2016/07/13 16:32:33, dcheng wrote: > On 2016/07/13 16:28:04, Patrick Monette wrote: > > On 2016/07/13 16:19:48, dcheng wrote: > > > On 2016/07/13 16:00:28, Will Harris wrote: > > > > On 2016/07/13 02:08:36, dcheng wrote: > > > > > On 2016/07/12 19:50:06, Will Harris wrote: > > > > > > This all lgtm from a win perspective. > > > > > > > > > > > > On 2016/07/12 19:38:21, Patrick Monette wrote: > > > > > > > Done. I doesn't seem like there is an equivalent Uint32ToHandle() to > > > cast > > > > > back > > > > > > > tho. > > > > > > > > > > > > ...yup it looks like there's code in ppapi/shared_impl/platform_file.h > > > which > > > > > > returns a base::PlatformHandle which is HANDLE, which is HWND... I'm > > > > wondering > > > > > > if the static_cast<intptr_t> is needed or not though. I think the > > > > > > reinterpret_cast as you have it should be fine as long as clang > doesn't > > > > > > complain. > > > > > > > > > > 1) Why don't we use the MS helpers for this? It looks like that's what > > > > > ParamTraits<HANDLE> uses: > > > > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1143 > > > and > > > > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1152 > > > > > > > > > > 2) I'd prefer it if we just created a small mojo struct to wrap HANDLE > and > > > > > typemap it to/from HANDLE. Is that possible / reasonable here? > > > > > > > > it's unusual to transfer HANDLE over mojo because they are normally > > > per-process > > > > - it's just HWND that are global (per winsta anyway) so I don't think > there > > is > > > > too much utility (and perhaps there is danger) in exposing this more > widely > > in > > > > mojo. Normally HANDLE would be carried cross process using the handle > > > attachment > > > > broker (is there a handle attachment broker for mojo, I have no idea). > > > > > > Ah, OK. So you see reinterpret_cast as a feature, not a problem here by > > raising > > > the bar. That seems like a decent reason to me. > > > > > > > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa384267.aspx is > > all > > > > the funcs, HandleToLong and LongToHandle look pretty much like the ones > > > already > > > > in use... > > > > > > > > TL;DR; I don't mind what is used here. > > > > > > Mostly this is just motivated by a desire to see fewer reinterpret_casts. > Mojo > > > does have a WrappedPlatformHandle thingie: I guess it uses > DuplicateHandle(). > > Is > > > it an error to use DuplicateHandle() on a HWND, given their global-ish > nature? > > > > It is a problem. HWND should really be seen as a different type from HANDLE > > (case in point, using LongToHandle() instead of reinterpret_cast gives the > error > > > > c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): error C2664: > > 'ui::win::OpenFileName::OpenFileName(const ui::win::OpenFileName &)': cannot > > convert argument 1 from 'void *' to 'HWND' > > c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): note: > > Conversion from 'void*' to pointer to non-'void' requires an explicit cast > > LGTM based on wfh's lgtm. > > That being said, I'm a bit surprised at the last point: I'm pretty sure we're > using ParamTraits<HANDLE> for HWNDs, and > https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx > claims HWND is typedef'ed to HANDLE, so I would have expected it to Just Work. > Very mysterious... That must mean we are compiling Chrome with STRICT mode enabled. https://msdn.microsoft.com/en-us/library/windows/desktop/aa383681(v=vs.85).aspx This make sure that HANDLE and HWND are 2 distinct types.
On 2016/07/13 16:43:58, Patrick Monette wrote: > On 2016/07/13 16:32:33, dcheng wrote: > > On 2016/07/13 16:28:04, Patrick Monette wrote: > > > On 2016/07/13 16:19:48, dcheng wrote: > > > > On 2016/07/13 16:00:28, Will Harris wrote: > > > > > On 2016/07/13 02:08:36, dcheng wrote: > > > > > > On 2016/07/12 19:50:06, Will Harris wrote: > > > > > > > This all lgtm from a win perspective. > > > > > > > > > > > > > > On 2016/07/12 19:38:21, Patrick Monette wrote: > > > > > > > > Done. I doesn't seem like there is an equivalent Uint32ToHandle() > to > > > > cast > > > > > > back > > > > > > > > tho. > > > > > > > > > > > > > > ...yup it looks like there's code in > ppapi/shared_impl/platform_file.h > > > > which > > > > > > > returns a base::PlatformHandle which is HANDLE, which is HWND... I'm > > > > > wondering > > > > > > > if the static_cast<intptr_t> is needed or not though. I think the > > > > > > > reinterpret_cast as you have it should be fine as long as clang > > doesn't > > > > > > > complain. > > > > > > > > > > > > 1) Why don't we use the MS helpers for this? It looks like that's what > > > > > > ParamTraits<HANDLE> uses: > > > > > > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1143 > > > > and > > > > > > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1152 > > > > > > > > > > > > 2) I'd prefer it if we just created a small mojo struct to wrap HANDLE > > and > > > > > > typemap it to/from HANDLE. Is that possible / reasonable here? > > > > > > > > > > it's unusual to transfer HANDLE over mojo because they are normally > > > > per-process > > > > > - it's just HWND that are global (per winsta anyway) so I don't think > > there > > > is > > > > > too much utility (and perhaps there is danger) in exposing this more > > widely > > > in > > > > > mojo. Normally HANDLE would be carried cross process using the handle > > > > attachment > > > > > broker (is there a handle attachment broker for mojo, I have no idea). > > > > > > > > Ah, OK. So you see reinterpret_cast as a feature, not a problem here by > > > raising > > > > the bar. That seems like a decent reason to me. > > > > > > > > > > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa384267.aspx > is > > > all > > > > > the funcs, HandleToLong and LongToHandle look pretty much like the ones > > > > already > > > > > in use... > > > > > > > > > > TL;DR; I don't mind what is used here. > > > > > > > > Mostly this is just motivated by a desire to see fewer reinterpret_casts. > > Mojo > > > > does have a WrappedPlatformHandle thingie: I guess it uses > > DuplicateHandle(). > > > Is > > > > it an error to use DuplicateHandle() on a HWND, given their global-ish > > nature? > > > > > > It is a problem. HWND should really be seen as a different type from HANDLE > > > (case in point, using LongToHandle() instead of reinterpret_cast gives the > > error > > > > > > c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): error > C2664: > > > 'ui::win::OpenFileName::OpenFileName(const ui::win::OpenFileName &)': cannot > > > convert argument 1 from 'void *' to 'HWND' > > > c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): note: > > > Conversion from 'void*' to pointer to non-'void' requires an explicit cast > > > > LGTM based on wfh's lgtm. > > > > That being said, I'm a bit surprised at the last point: I'm pretty sure we're > > using ParamTraits<HANDLE> for HWNDs, and > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx > > claims HWND is typedef'ed to HANDLE, so I would have expected it to Just Work. > > Very mysterious... > > That must mean we are compiling Chrome with STRICT mode enabled. > https://msdn.microsoft.com/en-us/library/windows/desktop/aa383681(v=vs.85).aspx > > This make sure that HANDLE and HWND are 2 distinct types. a bit of grepping finds https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.h?rcl=0&l=103 which instructs IPC to do the appropriate C++ magic to allow HWND to use the HANDLE traits. So this is why it works in IPC, but not in your code. Either way, I still think using the reinterpret_cast/static_cast is best route here. It makes sense for them to be symmetric though - like in https://cs.chromium.org/chromium/src/ppapi/shared_impl/platform_file.cc?sq=pa...
On 2016/07/13 16:43:58, Patrick Monette wrote: > On 2016/07/13 16:32:33, dcheng wrote: > > On 2016/07/13 16:28:04, Patrick Monette wrote: > > > On 2016/07/13 16:19:48, dcheng wrote: > > > > On 2016/07/13 16:00:28, Will Harris wrote: > > > > > On 2016/07/13 02:08:36, dcheng wrote: > > > > > > On 2016/07/12 19:50:06, Will Harris wrote: > > > > > > > This all lgtm from a win perspective. > > > > > > > > > > > > > > On 2016/07/12 19:38:21, Patrick Monette wrote: > > > > > > > > Done. I doesn't seem like there is an equivalent Uint32ToHandle() > to > > > > cast > > > > > > back > > > > > > > > tho. > > > > > > > > > > > > > > ...yup it looks like there's code in > ppapi/shared_impl/platform_file.h > > > > which > > > > > > > returns a base::PlatformHandle which is HANDLE, which is HWND... I'm > > > > > wondering > > > > > > > if the static_cast<intptr_t> is needed or not though. I think the > > > > > > > reinterpret_cast as you have it should be fine as long as clang > > doesn't > > > > > > > complain. > > > > > > > > > > > > 1) Why don't we use the MS helpers for this? It looks like that's what > > > > > > ParamTraits<HANDLE> uses: > > > > > > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1143 > > > > and > > > > > > > > https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?rcl=0&l=1152 > > > > > > > > > > > > 2) I'd prefer it if we just created a small mojo struct to wrap HANDLE > > and > > > > > > typemap it to/from HANDLE. Is that possible / reasonable here? > > > > > > > > > > it's unusual to transfer HANDLE over mojo because they are normally > > > > per-process > > > > > - it's just HWND that are global (per winsta anyway) so I don't think > > there > > > is > > > > > too much utility (and perhaps there is danger) in exposing this more > > widely > > > in > > > > > mojo. Normally HANDLE would be carried cross process using the handle > > > > attachment > > > > > broker (is there a handle attachment broker for mojo, I have no idea). > > > > > > > > Ah, OK. So you see reinterpret_cast as a feature, not a problem here by > > > raising > > > > the bar. That seems like a decent reason to me. > > > > > > > > > > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa384267.aspx > is > > > all > > > > > the funcs, HandleToLong and LongToHandle look pretty much like the ones > > > > already > > > > > in use... > > > > > > > > > > TL;DR; I don't mind what is used here. > > > > > > > > Mostly this is just motivated by a desire to see fewer reinterpret_casts. > > Mojo > > > > does have a WrappedPlatformHandle thingie: I guess it uses > > DuplicateHandle(). > > > Is > > > > it an error to use DuplicateHandle() on a HWND, given their global-ish > > nature? > > > > > > It is a problem. HWND should really be seen as a different type from HANDLE > > > (case in point, using LongToHandle() instead of reinterpret_cast gives the > > error > > > > > > c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): error > C2664: > > > 'ui::win::OpenFileName::OpenFileName(const ui::win::OpenFileName &)': cannot > > > convert argument 1 from 'void *' to 'HWND' > > > c:\src\chrome\src\chrome\utility\shell_handler_impl_win.cc(234): note: > > > Conversion from 'void*' to pointer to non-'void' requires an explicit cast > > > > LGTM based on wfh's lgtm. > > > > That being said, I'm a bit surprised at the last point: I'm pretty sure we're > > using ParamTraits<HANDLE> for HWNDs, and > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx > > claims HWND is typedef'ed to HANDLE, so I would have expected it to Just Work. > > Very mysterious... > > That must mean we are compiling Chrome with STRICT mode enabled. > https://msdn.microsoft.com/en-us/library/windows/desktop/aa383681(v=vs.85).aspx > > This make sure that HANDLE and HWND are 2 distinct types. wfh@ found the template trait that makes this work in Chromium: https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.h?sq=package:chrom... Anyway, I'm fine with the CL as-is. Sorry for the random musings!
Thanks! Committing.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, dcheng@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2122303002/#ps100001 (title: "Helper function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by pmonette@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Also convert the IPC mechanism to Mojo. BUG=73098 ========== to ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Also convert the IPC mechanism to Mojo. BUG=73098 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Also convert the IPC mechanism to Mojo. BUG=73098 ========== to ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Also convert the IPC mechanism to Mojo. BUG=73098 Committed: https://crrev.com/b7faec8d94ef286d2b92144c2836b170c2cdea95 Cr-Commit-Position: refs/heads/master@{#405270} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b7faec8d94ef286d2b92144c2836b170c2cdea95 Cr-Commit-Position: refs/heads/master@{#405270}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/2151643002/ by nektar@chromium.org. The reason for reverting is: Possibly broke compile on"Win8 GYP (dbg)".
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/2141093005/ by nektar@chromium.org. The reason for reverting is: Possibly broke compile on"Win8 GYP (dbg)".
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/2147763003/ by dbeam@chromium.org. The reason for reverting is: Broke Win8 GYP compile: https://build.chromium.org/p/chromium.win/builders/Win8%20GYP https://build.chromium.org/p/chromium.win/builders/Win8%20GYP%20(dbg).
Message was sent while issue was closed.
wow three reverts, must have been very broken! :)
Message was sent while issue was closed.
Description was changed from ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Also convert the IPC mechanism to Mojo. BUG=73098 Committed: https://crrev.com/b7faec8d94ef286d2b92144c2836b170c2cdea95 Cr-Commit-Position: refs/heads/master@{#405270} ========== to ========== Isolate shell operations to the utility process. Shell operations can cause 3rd-party shell extensions to be loaded into the caller's process. The utility process protects the browser process from potential instability. Also convert the IPC mechanism to Mojo. BUG=73098 Committed: https://crrev.com/b7faec8d94ef286d2b92144c2836b170c2cdea95 Cr-Commit-Position: refs/heads/master@{#405270} ==========
Copy / pasting one of the compile logs here FYI: c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.h(27): error C3668: 'ShellHandlerImpl::DoGetOpenFileName': method with override specifier 'override' did not override any base class methods c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.h(35): error C3668: 'ShellHandlerImpl::DoGetSaveFileName': method with override specifier 'override' did not override any base class methods c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(216): error C2259: 'ShellHandlerImpl': cannot instantiate abstract class c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(216): note: due to following members: c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(216): note: 'void mojom::ShellHandler::DoGetOpenFileName(uint32_t,uint32_t,mojom::FileExtensionFiltersPtr,mojo::common::mojom::FilePathPtr,mojo::common::mojom::FilePathPtr,const mojom::ShellHandler::DoGetOpenFileNameCallback &)': is abstract c:\b\build\slave\win_x64\build\src\out\release_x64\gen\chrome\common\shell_handler_win.mojom.h(83): note: see declaration of 'mojom::ShellHandler::DoGetOpenFileName' c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(216): note: 'void mojom::ShellHandler::DoGetSaveFileName(uint32_t,uint32_t,mojom::FileExtensionFiltersPtr,uint32_t,mojo::common::mojom::FilePathPtr,mojo::common::mojom::FilePathPtr,mojo::common::mojom::FilePathPtr,const mojom::ShellHandler::DoGetSaveFileNameCallback &)': is abstract c:\b\build\slave\win_x64\build\src\out\release_x64\gen\chrome\common\shell_handler_win.mojom.h(87): note: see declaration of 'mojom::ShellHandler::DoGetSaveFileName' c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(249): error C2664: 'void base::Callback<void (mojo::common::mojom::FilePathPtr,mojo::Array<mojo::common::mojom::FilePathPtr>),base::internal::CopyMode::Copyable>::Run(mojo::common::mojom::FilePathPtr,mojo::Array<mojo::common::mojom::FilePathPtr>) const': cannot convert argument 1 from 'base::FilePath' to 'mojo::common::mojom::FilePathPtr' c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(249): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(251): error C2664: 'void base::Callback<void (mojo::common::mojom::FilePathPtr,mojo::Array<mojo::common::mojom::FilePathPtr>),base::internal::CopyMode::Copyable>::Run(mojo::common::mojom::FilePathPtr,mojo::Array<mojo::common::mojom::FilePathPtr>) const': cannot convert argument 1 from 'base::FilePath' to 'mojo::common::mojom::FilePathPtr' c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(251): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(272): error C2664: 'void base::Callback<void (mojo::common::mojom::FilePathPtr,uint32_t),base::internal::CopyMode::Copyable>::Run(mojo::common::mojom::FilePathPtr,uint32_t) const': cannot convert argument 1 from 'base::FilePath' to 'mojo::common::mojom::FilePathPtr' c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(272): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(281): error C2664: 'void base::Callback<void (mojo::common::mojom::FilePathPtr,uint32_t),base::internal::CopyMode::Copyable>::Run(mojo::common::mojom::FilePathPtr,uint32_t) const': cannot convert argument 1 from 'base::FilePath' to 'mojo::common::mojom::FilePathPtr' c:\b\build\slave\win_x64\build\src\chrome\utility\shell_handler_impl_win.cc(281): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by pmonette@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run. |
