Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2615423002:
Convert utility process ParseMediaMetadata IPC to mojo (Closed)
|
Created:
3 years, 11 months ago by Noel Gordon Modified:
3 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, kinuko+fileapi, nhiroki, qsr+mojo_chromium.org, Lei Zhang, tommycli, tzik, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert utility process ParseMediaMetadata IPC to mojo
Convert IPC media parse messages ChromeUtilityMsg_ParseMediaMetadata
and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo.
Add media_parser.mojom, add StructTraits for metadata::AttachedImage
and use mojo common traits for base::DictionaryValue metadata.
Resolve a ChromeContentUtilityClient TODO: if the utility process is
running with elevated privileges, we only add elevated Mojo services
to the service_manager::InterfaceRegistry. Add ExtensionsHandler API
ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose
its elevated Mojo services if the process is running elevated.
Manually tested using the MGA media gallery sample app [1]. There is
a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that
runs on chrome mac.
[1] http://bit.ly/2i8qPSN
BUG=680928
Review-Url: https://codereview.chromium.org/2615423002
Cr-Commit-Position: refs/heads/master@{#443579}
Committed: https://chromium.googlesource.com/chromium/src/+/3b491f68e9be76a22f54208d6c1a3531eb4d8386
Patch Set 1 #Patch Set 2 : Build fix. #Patch Set 3 : Patch for review. #
Total comments: 10
Patch Set 4 : Respond to review comments. #Patch Set 5 : Use ::metadata::AttachedImage uniformly in the struct traits. #
Total comments: 16
Patch Set 6 : More review comments. #
Total comments: 16
Patch Set 7 : Security review comments. #Messages
Total messages: 85 (61 generated)
The CQ bit was checked by noel@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by noel@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...
Patchset #1 (id:1) has been deleted
Teted with https://github.com/GoogleChrome/chrome-app-samples/tree/master/samples/media-... sample, also available in the chrome webstore at https://chrome.google.com/webstore/detail/media-gallery-sample/lidepfgfmameop...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by noel@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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by noel@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 checked by noel@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...
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by noel@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.
The CQ bit was checked by noel@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.
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo BUG= ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo BUG=597124 ==========
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo BUG=597124 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata, and ChromeUtilityHostMsg_ParseMediaMetadata_Finished, to mojo. Add struct traits for serializing metadata::AttachedImage, and reuse mojo common traits for serializing base::DictionaryValue metadata in the parsed media metadata mojo callback response. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ==========
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata, and ChromeUtilityHostMsg_ParseMediaMetadata_Finished, to mojo. Add struct traits for serializing metadata::AttachedImage, and reuse mojo common traits for serializing base::DictionaryValue metadata in the parsed media metadata mojo callback response. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add struct traits for serializing metadata::AttachedImage, and reuse mojo common traits for serializing base::DictionaryValue metadata in the parsed media metadata mojo callback response. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ==========
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add struct traits for serializing metadata::AttachedImage, and reuse mojo common traits for serializing base::DictionaryValue metadata in the parsed media metadata mojo callback response. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add struct traits for serializing metadata::AttachedImage, and mojo common traits for serializing base::DictionaryValue metadata, in the parsed media metadata mojo callback response. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ==========
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add struct traits for serializing metadata::AttachedImage, and mojo common traits for serializing base::DictionaryValue metadata, in the parsed media metadata mojo callback response. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add struct traits for serializing metadata::AttachedImage, and mojo common traits for serializing base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ==========
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add struct traits for serializing metadata::AttachedImage, and mojo common traits for serializing base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add struct traits for serializing metadata::AttachedImage, use mojo common traits for serializing base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ==========
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add struct traits for serializing metadata::AttachedImage, use mojo common traits for serializing base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add media_parser.mojom, add StructTraits for metadata::AttachedImage and use mojo common traits for base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ==========
noel@chromium.org changed reviewers: + sammc@chromium.org, tibell@chromium.org
My first attempt at using mojo struct traits, lemme know if I'm doing it wrong. I also used basic mojo on the calling side, rather than mojo utility client, because calling code has other IPCs that rely on the UtilityProcessHostClient message loop.
https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... File chrome/common/extensions/media_parser.mojom (right): https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... chrome/common/extensions/media_parser.mojom:13: string type; Is this a MIME type? Please add a comment and perhaps rename to mime_type for clarity. https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... chrome/common/extensions/media_parser.mojom:14: string data; If this is binary data use array<uint8>. string will be string in C++, but a Unicode thing in Java. https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... File chrome/common/extensions/media_parser_struct_traits.h (right): https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... chrome/common/extensions/media_parser_struct_traits.h:15: using AttachedImageData = extensions::mojom::AttachedImageDataView; Do we allow using in .h files? Doesn't it pollute the mojo namespace? https://codereview.chromium.org/2615423002/diff/100001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2615423002/diff/100001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:195: const bool running_elevated = Can you explain why this was changed? Could perhaps use a clarifying comment. https://codereview.chromium.org/2615423002/diff/100001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2615423002/diff/100001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:60: mojo::MakeStrongBinding(base::MakeUnique<MediaParserImpl>(client), Do we know |client| will outlive this impl? We only delete this object when the connection is closed, which could be very late. Should |client| be ref-counted?
The CQ bit was checked by noel@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...
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo Converts IPC media messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add media_parser.mojom, add StructTraits for metadata::AttachedImage and use mojo common traits for base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Convert IPC media parse messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add media_parser.mojom, add StructTraits for metadata::AttachedImage and use mojo common traits for base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ==========
New patch uploaded ... https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... File chrome/common/extensions/media_parser.mojom (right): https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... chrome/common/extensions/media_parser.mojom:13: string type; On 2017/01/09 03:55:51, tibell wrote: > Is this a MIME type? Please add a comment and perhaps rename to mime_type for > clarity. Not sure it's a mime type: no documentation in ::media::AttachedImage about it. https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... chrome/common/extensions/media_parser.mojom:14: string data; On 2017/01/09 03:55:51, tibell wrote: > If this is binary data use array<uint8>. string will be string in C++, but a > Unicode thing in Java. Looks like binary data, so good point; use an array<uint8>. I used a ConstCArray<uint8_t> to put the std::string on to the wire as an array<uint8>, and a mojo data view of the structure's .data field to read it off the wire back into a std::string. Re-tested, seems fine. Though the code now has reintepret_cast() in a few places, array<uint8> is a much better type to use here, well-spotted. https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... File chrome/common/extensions/media_parser_struct_traits.h (right): https://codereview.chromium.org/2615423002/diff/100001/chrome/common/extensio... chrome/common/extensions/media_parser_struct_traits.h:15: using AttachedImageData = extensions::mojom::AttachedImageDataView; On 2017/01/09 03:55:52, tibell wrote: > Do we allow using in .h files? Doesn't it pollute the mojo namespace? Done. https://codereview.chromium.org/2615423002/diff/100001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2615423002/diff/100001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:195: const bool running_elevated = On 2017/01/09 03:55:52, tibell wrote: > Can you explain why this was changed? I did mention in the change description (you mighta missed it) > Could perhaps use a clarifying comment. and I added a comment about it at line 203 below :) https://codereview.chromium.org/2615423002/diff/100001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2615423002/diff/100001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:60: mojo::MakeStrongBinding(base::MakeUnique<MediaParserImpl>(client), On 2017/01/09 03:55:52, tibell wrote: > Do we know |client| will outlive this impl? We only delete this object when the > connection is closed, which could be very late. Yes |client| outlives the impl (I retained the same semantics about |client| lieftime that were used before this change). The |client| lifetime is controlled by the mojo callback and it calls ReleaseProcessIfNecessary() (see line 91 below) which kills |client|. > Should |client| be ref-counted? Nope (see above). Good questions about the lifetime issue btw, so thx for that. Aside: if in future we changed the caller to use mojo's utility host client, then lifetime of |client| for this mojo-ed API would probably become an issue.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@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.
lgtm
lgtm https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:112: base::Passed(metadata_dictionary->CreateDeepCopy()), base::Passed(&metadata_dictionary) https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:67: // Mojo callbcack if the utility process or metadata parse requests fail. callback https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:71: // Mojo callbcack from utility process when it finishes parsing metadata. callback https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:107: extensions::mojom::MediaParserPtr interface_; Maybe mention which thread this lives on. https://codereview.chromium.org/2615423002/diff/140001/chrome/common/extensio... File chrome/common/extensions/media_parser.typemap (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/common/extensio... chrome/common/extensions/media_parser.typemap:12: "//mojo/public/cpp/bindings", This shouldn't be necessary. https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:203: // Mojo services to the service_manager::InterfaceRegistry. I don't think the service_manager:: prefix is needed. https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:77: new metadata::IPCDataSource(total_size)); This appears to be the source of data to be parsed. https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.h (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.h:39: static void ExposeInterfacesToBrowser( I feel like we might want to put this in the UtilityMessageHandler interface. A TODO seems fine until the next UtilityMessageHandler is converted to mojo.
The CQ bit was checked by noel@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.
https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:112: base::Passed(metadata_dictionary->CreateDeepCopy()), On 2017/01/10 06:55:14, Sam McNally wrote: > base::Passed(&metadata_dictionary) Done. https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:67: // Mojo callbcack if the utility process or metadata parse requests fail. On 2017/01/10 06:55:14, Sam McNally wrote: > callback Done. https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:71: // Mojo callbcack from utility process when it finishes parsing metadata. On 2017/01/10 06:55:14, Sam McNally wrote: > callback Done. https://codereview.chromium.org/2615423002/diff/140001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:107: extensions::mojom::MediaParserPtr interface_; On 2017/01/10 06:55:14, Sam McNally wrote: > Maybe mention which thread this lives on. Hmmm, there's a comment introducing the member variables above, "// All member variables are only accessed on the IO thread". Maybe good enough? https://codereview.chromium.org/2615423002/diff/140001/chrome/common/extensio... File chrome/common/extensions/media_parser.typemap (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/common/extensio... chrome/common/extensions/media_parser.typemap:12: "//mojo/public/cpp/bindings", On 2017/01/10 06:55:14, Sam McNally wrote: > This shouldn't be necessary. Removed. https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:203: // Mojo services to the service_manager::InterfaceRegistry. On 2017/01/10 06:55:14, Sam McNally wrote: > I don't think the service_manager:: prefix is needed. Done. "service_manager::InterfaceRegistry." -> "interface registry." here, and in the other place I used this comment. https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:77: new metadata::IPCDataSource(total_size)); On 2017/01/10 06:55:14, Sam McNally wrote: > This appears to be the source of data to be parsed. Ack. https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.h (right): https://codereview.chromium.org/2615423002/diff/140001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.h:39: static void ExposeInterfacesToBrowser( On 2017/01/10 06:55:14, Sam McNally wrote: > I feel like we might want to put this in the UtilityMessageHandler interface. A > TODO seems fine until the next UtilityMessageHandler is converted to mojo. OK, added // TODO(noel): consider moving this API to the UtilityMessageHandler interface. I also just spotted https://codereview.chromium.org/1773233002 ... fascinating.
noel@chromium.org changed reviewers: + dcheng@chromium.org, tommycli@chromium.org
+tommycli for media galleries. +dcheng for IPC security.
roughly lgtm. I know very little about mojo. But since the browser tests and manual testing succeed, and nothing looks unusual / wrong from the media galleries perspective, lgtm
On 2017/01/11 00:42:03, tommycli wrote: > roughly lgtm. > > I know very little about mojo. But since the browser tests and manual testing > succeed, and nothing looks unusual / wrong from the media galleries perspective, > lgtm Thanks Tommy. Reading https://codereview.chromium.org/1773233002 seems the plan is to delete some (all?) Media Gallery features? Not sure we need to bother mojo-ing the related IPC's if they are gonna be deleted.
That particular piece is now used by ChromeOS File Manager. The work you have done is well worth it. On Jan 10, 2017 4:48 PM, <noel@chromium.org> wrote: > On 2017/01/11 00:42:03, tommycli wrote: > > roughly lgtm. > > > > I know very little about mojo. But since the browser tests and manual > testing > > succeed, and nothing looks unusual / wrong from the media galleries > perspective, > > lgtm > > Thanks Tommy. Reading https://codereview.chromium.org/1773233002 seems > the plan > is to delete some (all?) Media Gallery features? Not sure we need to bother > mojo-ing the related IPC's if they are gonna be deleted. > > https://codereview.chromium.org/2615423002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2615423002/diff/160001/chrome/browser/media_g... File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:102: interface_.reset(); I have mixed feelings about this sort of code... it feels like overly explicit memory management, but I understand the purpose. https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... File chrome/common/extensions/chrome_utility_extensions_messages.h (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... chrome/common/extensions/chrome_utility_extensions_messages.h:61: IPC_STRUCT_TRAITS_END() I think this can be removed as well. https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... File chrome/common/extensions/media_parser_struct_traits.h (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... chrome/common/extensions/media_parser_struct_traits.h:26: image.data.size(), reinterpret_cast<const uint8_t*>(image.data.data())); Mind adding a TODO to pass the actual image data around as a std::vector of uint8_t? You could TODO(dcheng) it =) https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... chrome/common/extensions/media_parser_struct_traits.h:30: ::metadata::AttachedImage* out) { Nit: please out-of-line this into a .cc file https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:205: if (running_elevated) Since this was changed, I wonder why we don't filter messages if kMessageWhitelistSize == 0 and the process is elevated. Is this a bug? Or do we assume that we never start up a elevated utility process that's filtering all messages? https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:76: std::unique_ptr<metadata::IPCDataSource> source( Prefer auto source = base::MakeUnique<IPCDataSource>(total_size) https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:122: // Mojo services to the interface registry. It doesn't seem like this needs the elevated bit at all. Should we even call this in the non-elevated case and add this parameter in if it's needed? Or do you envision this being a general method somewhere?
The CQ bit was checked by noel@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.
https://codereview.chromium.org/2615423002/diff/160001/chrome/browser/media_g... File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/browser/media_g... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:102: interface_.reset(); On 2017/01/11 09:09:59, dcheng wrote: > I have mixed feelings about this sort of code... it feels like overly explicit > memory management, but I understand the purpose. Acknowledged. https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... File chrome/common/extensions/chrome_utility_extensions_messages.h (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... chrome/common/extensions/chrome_utility_extensions_messages.h:61: IPC_STRUCT_TRAITS_END() On 2017/01/11 09:09:59, dcheng wrote: > I think this can be removed as well. Done. https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... File chrome/common/extensions/media_parser_struct_traits.h (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... chrome/common/extensions/media_parser_struct_traits.h:26: image.data.size(), reinterpret_cast<const uint8_t*>(image.data.data())); On 2017/01/11 09:09:59, dcheng wrote: > Mind adding a TODO to pass the actual image data around as a std::vector of > uint8_t? You could TODO(dcheng) it =) Good idea, done. https://codereview.chromium.org/2615423002/diff/160001/chrome/common/extensio... chrome/common/extensions/media_parser_struct_traits.h:30: ::metadata::AttachedImage* out) { On 2017/01/11 09:09:59, dcheng wrote: > Nit: please out-of-line this into a .cc file Done. Added chrome/common/extensions/media_parser_struct_traits.cc, add that file to sources = [] in the media_parser.typemap file. https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:205: if (running_elevated) On 2017/01/11 09:09:59, dcheng wrote: > Since this was changed, I wonder why we don't filter messages if > kMessageWhitelistSize == 0 and the process is elevated. Is this a bug? Great question. Looks like a potential bug to me. kMessageWhitelistSize == 0 and elevated looks to be possible by building chrome without the EXTENSIONS build flag (that would make kMessageWhitelistSize = 0). Dunno if we do that, but an elevated utility process in that case would expose non-elevated IPC, and (prior to my change) non-elevated mojos too. https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:76: std::unique_ptr<metadata::IPCDataSource> source( On 2017/01/11 09:09:59, dcheng wrote: > Prefer auto source = base::MakeUnique<IPCDataSource>(total_size) Done. https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:122: // Mojo services to the interface registry. On 2017/01/11 09:09:59, dcheng wrote: > It doesn't seem like this needs the elevated bit at all. Should we even call > this in the non-elevated case and add this parameter in if it's needed? Or do > you envision this being a general method somewhere? The current mojo is not elevated, and should not be added if elevation was requested. In an upcoming patch [1] I will add an elevated mojo herein. So yes, I envision this as a general method call to either add evalated mojos and bail, or else add non-elevated mojos. [1] https://codereview.chromium.org/2610953003
https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:205: if (running_elevated) On 2017/01/12 14:11:20, noel gordon wrote: > On 2017/01/11 09:09:59, dcheng wrote: > > Since this was changed, I wonder why we don't filter messages if > > kMessageWhitelistSize == 0 and the process is elevated. Is this a bug? > > Great question. Looks like a potential bug to me. kMessageWhitelistSize == 0 > and elevated looks to be possible by building chrome without the EXTENSIONS > build flag (that would make kMessageWhitelistSize = 0). Dunno if we do that, > but an elevated utility process in that case would expose non-elevated IPC, and > (prior to my change) non-elevated mojos too. https://cs.chromium.org/search/?q=ElevatePrivileges&sq=package:chromium tells me that utility process elevation is only used on WIN, and that its two users are in the EXTENSIONS system. Each of them have IPC whitelist entries installed in the utility process. So no bug here: an elevated utility process always has a whitelist to filter on.
mojo lgtm https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:205: if (running_elevated) On 2017/01/12 23:19:04, noel gordon wrote: > On 2017/01/12 14:11:20, noel gordon wrote: > > On 2017/01/11 09:09:59, dcheng wrote: > > > Since this was changed, I wonder why we don't filter messages if > > > kMessageWhitelistSize == 0 and the process is elevated. Is this a bug? > > > > Great question. Looks like a potential bug to me. kMessageWhitelistSize == 0 > > and elevated looks to be possible by building chrome without the EXTENSIONS > > build flag (that would make kMessageWhitelistSize = 0). Dunno if we do that, > > but an elevated utility process in that case would expose non-elevated IPC, > and > > (prior to my change) non-elevated mojos too. > > https://cs.chromium.org/search/?q=ElevatePrivileges&sq=package:chromium tells me > that utility process elevation is only used on WIN, and that its two users are > in the EXTENSIONS system. Each of them have IPC whitelist entries installed in > the utility process. So no bug here: an elevated utility process always has a > whitelist to filter on. OK. I hope we can clean up the filter_messages_ thing once this is all Mojo.
On 2017/01/13 09:21:04, dcheng wrote: > mojo lgtm > > https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... > File chrome/utility/chrome_content_utility_client.cc (right): > > https://codereview.chromium.org/2615423002/diff/160001/chrome/utility/chrome_... > chrome/utility/chrome_content_utility_client.cc:205: if (running_elevated) > On 2017/01/12 23:19:04, noel gordon wrote: > > https://cs.chromium.org/search/?q=ElevatePrivileges&sq=package:chromium tells > me > > that utility process elevation is only used on WIN, and that its two users are > > in the EXTENSIONS system. Each of them have IPC whitelist entries installed > in > > the utility process. So no bug here: an elevated utility process always has a > > whitelist to filter on. > > OK. I hope we can clean up the filter_messages_ thing once this is all Mojo. Agree. If we can get to all mojo, the hope is we can rm -rf "the filter_messages_ thing". Thanks for the careful review. Let's try landing.
noel@chromium.org changed reviewers: + jochen@chromium.org
+jochen for OWNERS.
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo Convert IPC media parse messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add media_parser.mojom, add StructTraits for metadata::AttachedImage and use mojo common traits for base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Convert IPC media parse messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add media_parser.mojom, add StructTraits for metadata::AttachedImage and use mojo common traits for base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124, 680928 ==========
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo Convert IPC media parse messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add media_parser.mojom, add StructTraits for metadata::AttachedImage and use mojo common traits for base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=597124, 680928 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Convert IPC media parse messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add media_parser.mojom, add StructTraits for metadata::AttachedImage and use mojo common traits for base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=680928 ==========
On 2017/01/11 00:48:35, noel gordon wrote: > On 2017/01/11 00:42:03, tommycli wrote: > > roughly lgtm. > > > > I know very little about mojo. But since the browser tests and manual testing > > succeed, and nothing looks unusual / wrong from the media galleries > perspective, > > lgtm > > Thanks Tommy. Reading https://codereview.chromium.org/1773233002 seems the plan > is to delete some (all?) Media Gallery features? Not sure we need to bother > mojo-ing the related IPC's if they are gonna be deleted. For the record, resolved this on http://crbug.com/542912 #19-#21, not worth mojo-ing the IPCs listed there ( http://crbug.com/542912#19 ).
On 2017/01/13 10:32:54, noel gordon wrote: > On 2017/01/11 00:48:35, noel gordon wrote: > > On 2017/01/11 00:42:03, tommycli wrote: > > > roughly lgtm. > > > > > > I know very little about mojo. But since the browser tests and manual > testing > > > succeed, and nothing looks unusual / wrong from the media galleries > > perspective, > > > lgtm > > > > Thanks Tommy. Reading https://codereview.chromium.org/1773233002 seems the > plan > > is to delete some (all?) Media Gallery features? Not sure we need to bother > > mojo-ing the related IPC's if they are gonna be deleted. > > For the record, resolved this on http://crbug.com/542912 #19-#21, not worth > mojo-ing the IPCs listed there ( http://crbug.com/542912#19 ). Well, http://crbug.com/542912#c19
lgtm
The CQ bit was checked by noel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, tibell@chromium.org, tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2615423002/#ps180001 (title: "Security review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484321714048070,
"parent_rev": "5688a4a82f90d9c27ab3e39d0dd682ca31f5876a", "commit_rev":
"3b491f68e9be76a22f54208d6c1a3531eb4d8386"}
Message was sent while issue was closed.
Description was changed from ========== Convert utility process ParseMediaMetadata IPC to mojo Convert IPC media parse messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add media_parser.mojom, add StructTraits for metadata::AttachedImage and use mojo common traits for base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=680928 ========== to ========== Convert utility process ParseMediaMetadata IPC to mojo Convert IPC media parse messages ChromeUtilityMsg_ParseMediaMetadata and ChromeUtilityHostMsg_ParseMediaMetadata_Finished to mojo. Add media_parser.mojom, add StructTraits for metadata::AttachedImage and use mojo common traits for base::DictionaryValue metadata. Resolve a ChromeContentUtilityClient TODO: if the utility process is running with elevated privileges, we only add elevated Mojo services to the service_manager::InterfaceRegistry. Add ExtensionsHandler API ExtensionsHandler::ExposeInterfacesToBrowser, and again, only expose its elevated Mojo services if the process is running elevated. Manually tested using the MGA media gallery sample app [1]. There is a browser test MediaGalleriesPlatformAppBrowserTest.GetMetadata that runs on chrome mac. [1] http://bit.ly/2i8qPSN BUG=680928 Review-Url: https://codereview.chromium.org/2615423002 Cr-Commit-Position: refs/heads/master@{#443579} Committed: https://chromium.googlesource.com/chromium/src/+/3b491f68e9be76a22f54208d6c1a... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/3b491f68e9be76a22f54208d6c1a...
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + yzshen@chromium.org
Message was sent while issue was closed.
Hallo yzshen@chromium.org! Due to a depot_tools patch which mistakenly removed the OWNERS check for non-source files (see crbug.com/684270), the following files landed in this CL and need a retrospective rubber-stampy review from you: mojo/public/tools/bindings/chromium_bindings_configuration.gni Thanks, Wez
Message was sent while issue was closed.
chromium_bindings_configuration.gni LGTM |
