Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
343053002:
Credential passing for WifiManager in Windows (Closed)
|
Created:
6 years, 6 months ago by Noam Samuel Modified:
6 years, 3 months ago Reviewers:
stevenjb, Lei Zhang, jschuh, Jorge Lucangeli Obes, Vitaly Buka (NO REVIEWS), not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPass credentials for WifiManager in Windows.
BUG=370071
Committed: https://crrev.com/8383a9849bd233d0c1792375e13d5243434826bc
Cr-Commit-Position: refs/heads/master@{#292700}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 18
Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Total comments: 3
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Messages
Total messages: 57 (1 generated)
lgtm https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... File chrome/browser/local_discovery/wifi/credential_getter_win.h (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/credential_getter_win.h:28: const scoped_refptr<base::MessageLoopProxy>& callback_runner); base::TaskRunner* callback_runner https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/credential_getter_win.h:44: scoped_refptr<base::MessageLoopProxy> callback_runner_; scoped_refptr<base::TaskRunner>
https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:14: using extensions::NetworkingPrivateCredentialsGetter; Unnecessary? (Code is in extensions namespace) https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:16: using extensions::NetworkingPrivateServiceClient; Ditto https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:42: callback.Run(base64_encoded_ciphertext, ""); nit: the above might be a little more clear if 'error' was a named variable and there was only one callback.Run invocation, i.e. callback.Run(base64_encoded_ciphertext, error). https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:63: base::WeakPtrFactory<NetworkingPrivateCredentialsGetterWin> weak_factory_; Unused? https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... File chrome/browser/local_discovery/wifi/credential_getter_win.cc (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/credential_getter_win.cc:55: PostCallback(false, ""); nit: comment args, e.g. PostCallback(false /* success */, "" /* no key data */) https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... File chrome/browser/local_discovery/wifi/credential_getter_win.h (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/credential_getter_win.h:28: const scoped_refptr<base::MessageLoopProxy>& callback_runner); On 2014/06/21 00:33:39, Vitaly Buka wrote: > base::TaskRunner* callback_runner Why use TaskRunner here, instead of MesageLoopProxy, which can safely outlive the message loop? Also, shouldn't const scoped_refptr<>& be preferable to a raw *? https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:441: #if defined(OS_WIN) nit: WS
https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... File chrome/browser/local_discovery/wifi/credential_getter_win.h (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/credential_getter_win.h:28: const scoped_refptr<base::MessageLoopProxy>& callback_runner); Usually Chrome code use TaskRunner or SequencedTaskRunner for similar purpose, and I believe it's correct. MesageLoopProxy is TaskRunner, it's subclass of SequencedTaskRunner and TaskRunner, and TaskRunner is enough here. It should be up to caller to choose which implementation of TaskRunner to pass here. Most callers will pass MesageLoopProxy into this method, but decision about lifecycle should be made by caller, not here. I had no strong opinion regarding scoped_refptr<> vs pure ptr. My impression is that pure ptr is usual way to pass RefCounted objects in functions and function should to create scoped_refptr only if it's going to store pointer after return. On 2014/06/23 17:12:20, stevenjb wrote: > On 2014/06/21 00:33:39, Vitaly Buka wrote: > > base::TaskRunner* callback_runner > Why use TaskRunner here, instead of MesageLoopProxy, which can safely outlive > the message loop? Also, shouldn't const scoped_refptr<>& be preferable to a raw > *?
https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:14: using extensions::NetworkingPrivateCredentialsGetter; On 2014/06/23 17:12:20, stevenjb wrote: > Unnecessary? (Code is in extensions namespace) Done. https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:16: using extensions::NetworkingPrivateServiceClient; On 2014/06/23 17:12:20, stevenjb wrote: > Ditto Done. https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:42: callback.Run(base64_encoded_ciphertext, ""); On 2014/06/23 17:12:20, stevenjb wrote: > nit: the above might be a little more clear if 'error' was a named variable and > there was only one callback.Run invocation, i.e. > callback.Run(base64_encoded_ciphertext, error). Done. https://codereview.chromium.org/343053002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:63: base::WeakPtrFactory<NetworkingPrivateCredentialsGetterWin> weak_factory_; On 2014/06/23 17:12:20, stevenjb wrote: > Unused? Done. https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... File chrome/browser/local_discovery/wifi/credential_getter_win.cc (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/credential_getter_win.cc:55: PostCallback(false, ""); On 2014/06/23 17:12:20, stevenjb wrote: > nit: comment args, e.g. PostCallback(false /* success */, "" /* no key data */) Done. https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... File chrome/browser/local_discovery/wifi/credential_getter_win.h (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/credential_getter_win.h:28: const scoped_refptr<base::MessageLoopProxy>& callback_runner); On 2014/06/23 17:46:27, Vitaly Buka wrote: > Usually Chrome code use TaskRunner or SequencedTaskRunner for similar purpose, > and I believe it's correct. > MesageLoopProxy is TaskRunner, it's subclass of SequencedTaskRunner and > TaskRunner, and TaskRunner is enough here. > It should be up to caller to choose which implementation of TaskRunner to pass > here. Most callers will pass MesageLoopProxy into this method, but decision > about lifecycle should be made by caller, not here. > > I had no strong opinion regarding scoped_refptr<> vs pure ptr. My impression is > that pure ptr is usual way to pass RefCounted objects in functions and function > should to create scoped_refptr only if it's going to store pointer after return. > > > On 2014/06/23 17:12:20, stevenjb wrote: > > On 2014/06/21 00:33:39, Vitaly Buka wrote: > > > base::TaskRunner* callback_runner > > Why use TaskRunner here, instead of MesageLoopProxy, which can safely outlive > > the message loop? Also, shouldn't const scoped_refptr<>& be preferable to a > raw > > *? > used const scoped_refptr<base::TaskRunner>& https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/credential_getter_win.h:44: scoped_refptr<base::MessageLoopProxy> callback_runner_; On 2014/06/21 00:33:39, Vitaly Buka wrote: > scoped_refptr<base::TaskRunner> Done. https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/343053002/diff/60001/chrome/browser/local_dis... chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:441: #if defined(OS_WIN) On 2014/06/23 17:12:20, stevenjb wrote: > nit: WS Done.
BUG id?
+thestig@ for chrome_content_utility_client review
https://codereview.chromium.org/343053002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/343053002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:42: callback.Run(base64_encoded_ciphertext, ""); , error
https://codereview.chromium.org/343053002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/343053002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:42: callback.Run(base64_encoded_ciphertext, ""); On 2014/06/23 18:18:29, stevenjb wrote: > , error Done.
On 2014/06/23 17:58:09, Noam Samuel wrote: > +thestig@ for chrome_content_utility_client review lgtm
lgtm
Is there anything specific that you want me to review, or should I take a look at the whole CL?
On 2014/06/23 22:24:00, jorgelo (OOO till July 11th) wrote: > Is there anything specific that you want me to review, or should I take a look > at the whole CL? Specifically IPC security, and ensuring that this approach is secure.
I see a lot of code moved around but I don't see the utility side of the IPC. Where's the UKEY implementation? Has it been done already and you're just using that code? IPC looks sane but just wanted to double-check. https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right): https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_ipc_whitelist.cc:11: #ifdef OS_WIN #if defined(OS_WIN) Don't even know why we have it the other way.
On 2014/07/22 17:20:05, Jorge Lucangeli Obes wrote: > I see a lot of code moved around but I don't see the utility side of the IPC. > Where's the UKEY implementation? Has it been done already and you're just using > that code? IPC looks sane but just wanted to double-check. > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right): > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > chrome/utility/chrome_content_utility_ipc_whitelist.cc:11: #ifdef OS_WIN > #if defined(OS_WIN) > > Don't even know why we have it the other way. I guess we can hold this patch.
On 2014/07/22 17:38:57, Vitaly Buka wrote: > On 2014/07/22 17:20:05, Jorge Lucangeli Obes wrote: > > I see a lot of code moved around but I don't see the utility side of the IPC. > > Where's the UKEY implementation? Has it been done already and you're just > using > > that code? IPC looks sane but just wanted to double-check. > > > > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > > File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right): > > > > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > > chrome/utility/chrome_content_utility_ipc_whitelist.cc:11: #ifdef OS_WIN > > #if defined(OS_WIN) > > > > Don't even know why we have it the other way. > > I guess we can hold this patch. What do you mean?
On 2014/07/22 18:00:27, Jorge Lucangeli Obes wrote: > On 2014/07/22 17:38:57, Vitaly Buka wrote: > > On 2014/07/22 17:20:05, Jorge Lucangeli Obes wrote: > > > I see a lot of code moved around but I don't see the utility side of the > IPC. > > > Where's the UKEY implementation? Has it been done already and you're just > > using > > > that code? IPC looks sane but just wanted to double-check. > > > > > > > > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > > > File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right): > > > > > > > > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > > > chrome/utility/chrome_content_utility_ipc_whitelist.cc:11: #ifdef OS_WIN > > > #if defined(OS_WIN) > > > > > > Don't even know why we have it the other way. > > > > I guess we can hold this patch. > > What do you mean? Sorry, confused with different patch. Patch description is confusing, this one has nothing UKEY related.
On 2014/07/22 18:06:00, Vitaly Buka wrote: > On 2014/07/22 18:00:27, Jorge Lucangeli Obes wrote: > > On 2014/07/22 17:38:57, Vitaly Buka wrote: > > > On 2014/07/22 17:20:05, Jorge Lucangeli Obes wrote: > > > > I see a lot of code moved around but I don't see the utility side of the > > IPC. > > > > Where's the UKEY implementation? Has it been done already and you're just > > > using > > > > that code? IPC looks sane but just wanted to double-check. > > > > > > > > > > > > > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > > > > File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > > > > chrome/utility/chrome_content_utility_ipc_whitelist.cc:11: #ifdef OS_WIN > > > > #if defined(OS_WIN) > > > > > > > > Don't even know why we have it the other way. > > > > > > I guess we can hold this patch. > > > > What do you mean? > > Sorry, confused with different patch. > > Patch description is confusing, this one has nothing UKEY related. Noam, can you update the description for this patch?
On 2014/07/22 18:08:15, Jorge Lucangeli Obes wrote: > On 2014/07/22 18:06:00, Vitaly Buka wrote: > > On 2014/07/22 18:00:27, Jorge Lucangeli Obes wrote: > > > On 2014/07/22 17:38:57, Vitaly Buka wrote: > > > > On 2014/07/22 17:20:05, Jorge Lucangeli Obes wrote: > > > > > I see a lot of code moved around but I don't see the utility side of the > > > IPC. > > > > > Where's the UKEY implementation? Has it been done already and you're > just > > > > using > > > > > that code? IPC looks sane but just wanted to double-check. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > > > > > File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/343053002/diff/120001/chrome/utility/chrome_c... > > > > > chrome/utility/chrome_content_utility_ipc_whitelist.cc:11: #ifdef OS_WIN > > > > > #if defined(OS_WIN) > > > > > > > > > > Don't even know why we have it the other way. > > > > > > > > I guess we can hold this patch. > > > > > > What do you mean? > > > > Sorry, confused with different patch. > > > > Patch description is confusing, this one has nothing UKEY related. > > Noam, can you update the description for this patch? Updated
https://codereview.chromium.org/343053002/diff/120001/chrome/common/chrome_ut... File chrome/common/chrome_utility_messages.h (right): https://codereview.chromium.org/343053002/diff/120001/chrome/common/chrome_ut... chrome/common/chrome_utility_messages.h:521: IPC_MESSAGE_CONTROL1(ChromeUtilityHostMsg_GetWiFiCredentials, So this is not encrypted anymore? The comment change is confusing.
https://codereview.chromium.org/343053002/diff/120001/chrome/common/chrome_ut... File chrome/common/chrome_utility_messages.h (right): https://codereview.chromium.org/343053002/diff/120001/chrome/common/chrome_ut... chrome/common/chrome_utility_messages.h:521: IPC_MESSAGE_CONTROL1(ChromeUtilityHostMsg_GetWiFiCredentials, On 2014/07/22 18:18:49, Jorge Lucangeli Obes wrote: > So this is not encrypted anymore? The comment change is confusing. Correct. Communication between the utility process and the browser process is not encrypted in this change. The browser process is responsible for encrypting the password for use in the renderer process or over the network.
On 2014/07/23 17:19:11, Noam Samuel wrote: > https://codereview.chromium.org/343053002/diff/120001/chrome/common/chrome_ut... > File chrome/common/chrome_utility_messages.h (right): > > https://codereview.chromium.org/343053002/diff/120001/chrome/common/chrome_ut... > chrome/common/chrome_utility_messages.h:521: > IPC_MESSAGE_CONTROL1(ChromeUtilityHostMsg_GetWiFiCredentials, > On 2014/07/22 18:18:49, Jorge Lucangeli Obes wrote: > > So this is not encrypted anymore? The comment change is confusing. > > Correct. Communication between the utility process and the browser process is > not encrypted in this change. The browser process is responsible for encrypting > the password for use in the renderer process or over the network. I see. So is that a separate CL? Has that code already landed?
The encryption is still being worked on. If you're more comfortable with it, we can return to this CL once the encryption is working. On Wed, Jul 23, 2014 at 10:34 AM, <jorgelo@chromium.org> wrote: > On 2014/07/23 17:19:11, Noam Samuel wrote: > > https://codereview.chromium.org/343053002/diff/120001/ > chrome/common/chrome_utility_messages.h > >> File chrome/common/chrome_utility_messages.h (right): >> > > > https://codereview.chromium.org/343053002/diff/120001/ > chrome/common/chrome_utility_messages.h#newcode521 > >> chrome/common/chrome_utility_messages.h:521: >> IPC_MESSAGE_CONTROL1(ChromeUtilityHostMsg_GetWiFiCredentials, >> On 2014/07/22 18:18:49, Jorge Lucangeli Obes wrote: >> > So this is not encrypted anymore? The comment change is confusing. >> > > Correct. Communication between the utility process and the browser >> process is >> not encrypted in this change. The browser process is responsible for >> > encrypting > >> the password for use in the renderer process or over the network. >> > > I see. So is that a separate CL? Has that code already landed? > > https://codereview.chromium.org/343053002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/23 17:40:04, chromium-reviews wrote: > The encryption is still being worked on. If you're more comfortable with > it, we can return to this CL once the encryption is working. Well, that would mean that between the time this CL lands and that CL lands, credentials would be in the clear? Are you guys just hoping that there's no dev channel releases during that time? > > On Wed, Jul 23, 2014 at 10:34 AM, <mailto:jorgelo@chromium.org> wrote: > > > On 2014/07/23 17:19:11, Noam Samuel wrote: > > > > https://codereview.chromium.org/343053002/diff/120001/ > > chrome/common/chrome_utility_messages.h > > > >> File chrome/common/chrome_utility_messages.h (right): > >> > > > > > > https://codereview.chromium.org/343053002/diff/120001/ > > chrome/common/chrome_utility_messages.h#newcode521 > > > >> chrome/common/chrome_utility_messages.h:521: > >> IPC_MESSAGE_CONTROL1(ChromeUtilityHostMsg_GetWiFiCredentials, > >> On 2014/07/22 18:18:49, Jorge Lucangeli Obes wrote: > >> > So this is not encrypted anymore? The comment change is confusing. > >> > > > > Correct. Communication between the utility process and the browser > >> process is > >> not encrypted in this change. The browser process is responsible for > >> > > encrypting > > > >> the password for use in the renderer process or over the network. > >> > > > > I see. So is that a separate CL? Has that code already landed? > > > > https://codereview.chromium.org/343053002/
On 2014/07/23 20:01:28, Jorge Lucangeli Obes wrote: > On 2014/07/23 17:40:04, chromium-reviews wrote: > > The encryption is still being worked on. If you're more comfortable with > > it, we can return to this CL once the encryption is working. > > Well, that would mean that between the time this CL lands and that CL lands, > credentials would be in the clear? Are you guys just hoping that there's no dev > channel releases during that time? > > > > > On Wed, Jul 23, 2014 at 10:34 AM, <mailto:jorgelo@chromium.org> wrote: > > > > > On 2014/07/23 17:19:11, Noam Samuel wrote: > > > > > > https://codereview.chromium.org/343053002/diff/120001/ > > > chrome/common/chrome_utility_messages.h > > > > > >> File chrome/common/chrome_utility_messages.h (right): > > >> > > > > > > > > > https://codereview.chromium.org/343053002/diff/120001/ > > > chrome/common/chrome_utility_messages.h#newcode521 > > > > > >> chrome/common/chrome_utility_messages.h:521: > > >> IPC_MESSAGE_CONTROL1(ChromeUtilityHostMsg_GetWiFiCredentials, > > >> On 2014/07/22 18:18:49, Jorge Lucangeli Obes wrote: > > >> > So this is not encrypted anymore? The comment change is confusing. > > >> > > > > > > Correct. Communication between the utility process and the browser > > >> process is > > >> not encrypted in this change. The browser process is responsible for > > >> > > > encrypting > > > > > >> the password for use in the renderer process or over the network. > > >> > > > > > > I see. So is that a separate CL? Has that code already landed? > > > > > > https://codereview.chromium.org/343053002/ This is used in gcdPrivate, a trunk-only API that will soon have a whitelisted extension once it reaches non-trunk.
On 2014/07/23 21:01:05, Noam Samuel wrote: > On 2014/07/23 20:01:28, Jorge Lucangeli Obes wrote: > > On 2014/07/23 17:40:04, chromium-reviews wrote: > > > The encryption is still being worked on. If you're more comfortable with > > > it, we can return to this CL once the encryption is working. > > > > Well, that would mean that between the time this CL lands and that CL lands, > > credentials would be in the clear? Are you guys just hoping that there's no > dev > > channel releases during that time? > > > > > > > > On Wed, Jul 23, 2014 at 10:34 AM, <mailto:jorgelo@chromium.org> wrote: > > > > > > > On 2014/07/23 17:19:11, Noam Samuel wrote: > > > > > > > > https://codereview.chromium.org/343053002/diff/120001/ > > > > chrome/common/chrome_utility_messages.h > > > > > > > >> File chrome/common/chrome_utility_messages.h (right): > > > >> > > > > > > > > > > > > https://codereview.chromium.org/343053002/diff/120001/ > > > > chrome/common/chrome_utility_messages.h#newcode521 > > > > > > > >> chrome/common/chrome_utility_messages.h:521: > > > >> IPC_MESSAGE_CONTROL1(ChromeUtilityHostMsg_GetWiFiCredentials, > > > >> On 2014/07/22 18:18:49, Jorge Lucangeli Obes wrote: > > > >> > So this is not encrypted anymore? The comment change is confusing. > > > >> > > > > > > > > Correct. Communication between the utility process and the browser > > > >> process is > > > >> not encrypted in this change. The browser process is responsible for > > > >> > > > > encrypting > > > > > > > >> the password for use in the renderer process or over the network. > > > >> > > > > > > > > I see. So is that a separate CL? Has that code already landed? > > > > > > > > https://codereview.chromium.org/343053002/ > > This is used in gcdPrivate, a trunk-only API that will soon have a whitelisted > extension once it reaches non-trunk. In that case feel free to land in the order that makes things easier for you. IPC lgtm.
The CQ bit was checked by noamsml@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/343053002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/32391) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/37291)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/32439)
The CQ bit was checked by noamsml@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/343053002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/47709) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/53050) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by noamsml@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/343053002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/47809) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
noamsml@chromium.org changed reviewers: + kalman@chromium.org
+kalman for chrome_utility_extension_messages
On 2014/08/28 23:13:03, Noam Samuel wrote: > +kalman for chrome_utility_extension_messages No, you need a security reviewer but for some reason, jorgelo is in most messages OWNERS files but not this one.
You need an IPC owner not an extension owner.
thestig@chromium.org changed reviewers: + jschuh@chromium.org
+jschuh ^ I'll review and possibly sync up the OWNERS files.
On 2014/08/28 23:16:41, Lei Zhang wrote: > +jschuh ^ > > I'll review and possibly sync up the OWNERS files. lgtm - rubberstamping since jorgelo already reviewed for ipc security.
The CQ bit was checked by vitalybuka@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/343053002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/47913) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by noamsml@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/343053002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as f611114d8f2ef08eb6f374fc1ff20300ac60811a
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/8383a9849bd233d0c1792375e13d5243434826bc Cr-Commit-Position: refs/heads/master@{#292700} |
