Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2745653008:
[Media Router] Parse device description xml in utility process (Closed)
|
Created:
3 years, 9 months ago by zhaobin Modified:
3 years, 9 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, media-router+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Parse device description xml in utility process
- Add dial_device_description_parser.mojom to support communication between browser and utility process
- Add DialDeviceDescriptionParser in chrome/utility to handle xml parsing and scrubbing
- Add SafeDialDeviceDescriptionParser in chrome/browser to start parsing in utility process
BUG=687375
Review-Url: https://codereview.chromium.org/2745653008
Cr-Commit-Position: refs/heads/master@{#459902}
Committed: https://chromium.googlesource.com/chromium/src/+/687cddcf3340fb4e163ec55462dd2a66b9167774
Patch Set 1 #Patch Set 2 : update common/media_router/OWNERS #Patch Set 3 : use mojo instead of IPC #
Total comments: 38
Patch Set 4 : rebase with master #Patch Set 5 : resolve code review comments from Mark #
Total comments: 20
Patch Set 6 : resolve code review comments from Mark #
Total comments: 21
Patch Set 7 : resolve code review comments from Mark #
Total comments: 26
Patch Set 8 : resolve code review comments from Derek #
Total comments: 16
Patch Set 9 : resolve code review comments from dcheng #Patch Set 10 : resolve code review comments from jam #
Total comments: 4
Patch Set 11 : resolve code review comments from dcheng #Patch Set 12 : rebase with master #Patch Set 13 : try fix build errors #Patch Set 14 : fix android build error #Messages
Total messages: 55 (23 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
Bin, could you look into using Mojo for talking to the utility process? Instead of defining IPCs, you would define interfaces / structs in mojom. https://cs.chromium.org/chromium/src/content/public/browser/utility_process_m...
Patchset #3 (id:40001) has been deleted
On 2017/03/15 00:51:33, imcheng wrote: > Bin, could you look into using Mojo for talking to the utility process? Instead > of defining IPCs, you would define interfaces / structs in mojom. > > https://cs.chromium.org/chromium/src/content/public/browser/utility_process_m... Done.
This looks really good overall and most of my comments are minor. The one more significant thing is where the mojo service should live -from the design doc it wasn't clear that it needed to be linked to the extensions system. I'm a little nervous about the XSLT code. Could you find a reviewer who has written similar code before to help review it? https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:39: &SafeDialDeviceDescriptionParser::ParseDialDeviceDescriptionFailed, Conventionally, methods used only as callbacks start with "On", so suggest OnParseDeviceDescriptionFailed https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:41: utility_process_mojo_client_->Start(); // Start the utility process I assume this starts the process in the background, and the call below is queued up until the Mojo message pipe has been established to the service running in the utility process. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:45: &SafeDialDeviceDescriptionParser::ParseDialDeviceDescriptionDone, OnParseDeviceDescriptionComplete https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:53: DCHECK_CURRENTLY_ON(BrowserThread::IO); Slightly prefer base::ThreadChecker. I suppose that DCHECK_CURRENTLY_ON saves allocating a class member, but makes the class slightly harder to test. Not sure why we have both ways of checking thread safety. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:20: : public base::RefCountedThreadSafe<SafeDialDeviceDescriptionParser> { If this is used *only* on the IO thread I think this can be base::RefCounted<>. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:22: using DeviceDescriptionCallback = Document the parameters here (especially the first and third) https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:39: // device description XML. Runs on IO thread. Please document the meanings of |result| and |logging_xml| https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:45: void ParseDialDeviceDescriptionFailed(); Can this take an enum describing the reason why it failed? We will probably want to add an UMA histogram tracking failure reasons. (Fine to add as a TODO() for later). https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:55: Add a base::ThreadChecker to enforce thread safety. https://codereview.chromium.org/2745653008/diff/60001/chrome/common/extension... File chrome/common/extensions/dial_device_description_parser.mojom (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/common/extension... chrome/common/extensions/dial_device_description_parser.mojom:8: module extensions.mojom; Is this going to be used only from extensions? It seems like it could go in media_router.mojom. I'm not make sure it makes sense to add a chrome.dial API for this service, since the DOM XML parsing is sufficient, and the incremental security benefit of moving the parsing out of the extension process would be short lived. https://codereview.chromium.org/2745653008/diff/60001/chrome/common/extension... chrome/common/extensions/dial_device_description_parser.mojom:10: struct DialDeviceDescription { Can you document the meanings of these fields, maybe copying from the dial.idl documentation or the extension documentation? https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/extensio... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/extensio... chrome/utility/extensions/extensions_handler.cc:240: registry->AddInterface(base::Bind(&DialDeviceDescriptionParserImpl::Create)); It isn't clear that this needs to be created from extension processes - from a quick glance at the design doc, it looks like it will be created by MediaRouterMojoImpl (or a DialSinkDiscovery class owned by it), so only from the browser process. https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... File chrome/utility/media_router/dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:89: << "Fixed device description: created friendlyName from modelName."; In my opinion, it will be pretty clear from the friendly name that this is what happened. Consider removing this log statement. https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:106: if (doc == NULL) { s/NULL/nullptr/ assuming doc is a pointer type. https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:112: DCHECK(xslt_doc); Assuming xmlDocPtr is a pointer type, it would be safer to CHECK here to avoid a null deref later (although xslt_text is under our control). https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:119: xsltSaveResultToString(&output, &size, res, style_ptr); I assume this allocates output to be sufficiently sized for the result? https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:121: std::string logging_xml = std::string(reinterpret_cast<char*>(output)); I'm not an expert in XML APIs to know how safe this is. Is output guaranteed to be valid UTF-8 and null-terminated? https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:127: xmlCleanupParser(); Who frees output? https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... File chrome/utility/media_router/dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.h:30: const media_router::DialDeviceDescription& device_description() { This design means the caller has to keep the parser alive since it owns the device description. Also it means that calling this before parse returns a default-constructed description which is not useful. A more common pattern for parsing APIs is to pass a non-const pointer as an output parameter; it's slightly less safe, but allows the caller to clearly own the output. It also means this parser doesn't have to be a class at all, saving an allocation.
[I'll review the unit test in a final pass.]
On 2017/03/17 at 19:06:05, mark a. foltz wrote: > [I'll review the unit test in a final pass.] P.S. Please update the patch description to match the current design :)
Description was changed from ========== [Media Router] Parse device description xml in utility process - Add IPC message ChromeUtilityMsg_ParseDialDeviceDescriptionXml and ChromeUtilityHostMsg_GotDialDeviceDescription - Add DialDeviceDescriptionParser in chrome/utility to handle xml parsing and scrubbing - Add SafeDialDeviceDescriptionParser in chrome/browser to start parsing in utility process BUG=687375 ========== to ========== [Media Router] Parse device description xml in utility process - Add dial_device_description_parser.mojom to support communication between browser and utility process - Add DialDeviceDescriptionParser in chrome/utility to handle xml parsing and scrubbing - Add SafeDialDeviceDescriptionParser in chrome/browser to start parsing in utility process BUG=687375 ==========
> > The one more significant thing is where the mojo service should live -from the > design doc it wasn't clear that it needed to be linked to the extensions system. > Mojo service does not need to be linked to the extensions system. But it seems that it needs to stay in chrome/common for utility process to access it. > I'm a little nervous about the XSLT code. Could you find a reviewer who has > written similar code before to help review it? > Did not find any reference of libxslt in code base...Shall we simply use string replacement to get rid of the xslt code? (E.g. find '<serialNumber>', '</serialNumber>' tags and replace them with <serialNumber>****</serialNumber>...) https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:39: &SafeDialDeviceDescriptionParser::ParseDialDeviceDescriptionFailed, On 2017/03/17 19:05:46, mark a. foltz wrote: > Conventionally, methods used only as callbacks start with "On", so suggest > OnParseDeviceDescriptionFailed Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:41: utility_process_mojo_client_->Start(); // Start the utility process On 2017/03/17 19:05:46, mark a. foltz wrote: > I assume this starts the process in the background, and the call below is queued > up until the Mojo message pipe has been established to the service running in > the utility process. Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:45: &SafeDialDeviceDescriptionParser::ParseDialDeviceDescriptionDone, On 2017/03/17 19:05:46, mark a. foltz wrote: > OnParseDeviceDescriptionComplete Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:53: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/03/17 19:05:46, mark a. foltz wrote: > Slightly prefer base::ThreadChecker. I suppose that DCHECK_CURRENTLY_ON saves > allocating a class member, but makes the class slightly harder to test. Not > sure why we have both ways of checking thread safety. Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:20: : public base::RefCountedThreadSafe<SafeDialDeviceDescriptionParser> { On 2017/03/17 19:05:46, mark a. foltz wrote: > If this is used *only* on the IO thread I think this can be base::RefCounted<>. Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:22: using DeviceDescriptionCallback = On 2017/03/17 19:05:46, mark a. foltz wrote: > Document the parameters here (especially the first and third) Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:39: // device description XML. Runs on IO thread. On 2017/03/17 19:05:46, mark a. foltz wrote: > Please document the meanings of |result| and |logging_xml| Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:45: void ParseDialDeviceDescriptionFailed(); On 2017/03/17 19:05:46, mark a. foltz wrote: > Can this take an enum describing the reason why it failed? We will probably > want to add an UMA histogram tracking failure reasons. > > (Fine to add as a TODO() for later). Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:55: On 2017/03/17 19:05:46, mark a. foltz wrote: > Add a base::ThreadChecker to enforce thread safety. Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/common/extension... File chrome/common/extensions/dial_device_description_parser.mojom (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/common/extension... chrome/common/extensions/dial_device_description_parser.mojom:8: module extensions.mojom; On 2017/03/17 19:05:47, mark a. foltz wrote: > Is this going to be used only from extensions? It seems like it could go in > media_router.mojom. > > I'm not make sure it makes sense to add a chrome.dial API for this service, > since the DOM XML parsing is sufficient, and the incremental security benefit of > moving the parsing out of the extension process would be short lived. Moved mojo related files to common/media_router/. Cannot add it to browser/media/router/mojo/media_router.mojom file since chrome/utility does not have access to chrome/browser headers. Other .mojom files in chrome/common use "module chrome.mojom". Keep that instead of changing it to "module media_router.mojom"... https://codereview.chromium.org/2745653008/diff/60001/chrome/common/extension... chrome/common/extensions/dial_device_description_parser.mojom:10: struct DialDeviceDescription { On 2017/03/17 19:05:47, mark a. foltz wrote: > Can you document the meanings of these fields, maybe copying from the dial.idl > documentation or the extension documentation? Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/extensio... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/extensio... chrome/utility/extensions/extensions_handler.cc:240: registry->AddInterface(base::Bind(&DialDeviceDescriptionParserImpl::Create)); On 2017/03/17 19:05:47, mark a. foltz wrote: > It isn't clear that this needs to be created from extension processes - from a > quick glance at the design doc, it looks like it will be created by > MediaRouterMojoImpl (or a DialSinkDiscovery class owned by it), so only from the > browser process. Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... File chrome/utility/media_router/dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:89: << "Fixed device description: created friendlyName from modelName."; On 2017/03/17 19:05:47, mark a. foltz wrote: > In my opinion, it will be pretty clear from the friendly name that this is what > happened. Consider removing this log statement. Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:106: if (doc == NULL) { On 2017/03/17 19:05:47, mark a. foltz wrote: > s/NULL/nullptr/ assuming doc is a pointer type. Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:112: DCHECK(xslt_doc); On 2017/03/17 19:05:47, mark a. foltz wrote: > Assuming xmlDocPtr is a pointer type, it would be safer to CHECK here to avoid a > null deref later (although xslt_text is under our control). > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:119: xsltSaveResultToString(&output, &size, res, style_ptr); On 2017/03/17 19:05:47, mark a. foltz wrote: > I assume this allocates output to be sufficiently sized for the result? Seems so. output is allocated inside xsltSaveResultToString(). https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:121: std::string logging_xml = std::string(reinterpret_cast<char*>(output)); On 2017/03/17 19:05:47, mark a. foltz wrote: > I'm not an expert in XML APIs to know how safe this is. Is output guaranteed to > be valid UTF-8 and null-terminated? xmlChar is unsigned char, and seems that output is not necessary null-terminated, so reinterpret_cast is not safe... https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.cc:127: xmlCleanupParser(); On 2017/03/17 19:05:47, mark a. foltz wrote: > Who frees output? Done. https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... File chrome/utility/media_router/dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/60001/chrome/utility/media_ro... chrome/utility/media_router/dial_device_description_parser.h:30: const media_router::DialDeviceDescription& device_description() { On 2017/03/17 19:05:47, mark a. foltz wrote: > This design means the caller has to keep the parser alive since it owns the > device description. > > Also it means that calling this before parse returns a default-constructed > description which is not useful. > > A more common pattern for parsing APIs is to pass a non-const pointer as an > output parameter; it's slightly less safe, but allows the caller to clearly own > the output. > > It also means this parser doesn't have to be a class at all, saving an > allocation. Done.
Looks good with remaining comments addressed. https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description.h (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... chrome/common/media_router/dial_device_description.h:23: // Model name Nit: Period at end https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... chrome/common/media_router/dial_device_description.h:26: // The reported device type. E.g. urn:dial-multiscreen-org:device:dial:1 Nit: type, e.g. https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description_parser.mojom (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:17: // Model name Period at end https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:20: // The reported device type. E.g. urn:dial-multiscreen-org:device:dial:1 type, e.g. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser.cc:55: XmlReader xml_reader; DCHECK(out) https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser.cc:81: auto model_name = out->model_name; Slight preference to factor out a function like ComputeFriendlyName(out) to take care of this. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser.cc:108: xmlDocPtr xslt_doc = xmlParseMemory(xslt_text, sizeof(xslt_text)); I'm leaning towards using string substitution for scrubbing. It's less code, doesn't require parsing the document twice, and doesn't use a library I'm not familiar with the security risk of and no one else seems to be using either. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser.h:15: class DialDeviceDescriptionParser { Does this need to be a class? There are no longer any data members. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.h (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.h:13: class DialDeviceDescriptionParserImpl Please document the thread safety of this class. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_unittest.cc (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_unittest.cc:68: std::string& Replace(std::string& input, This doesn't need to be part of a class; you can declare it in an anonymous namespace, remove DeviceDescriptionParserTest, and just use TEST() instead of TEST_F below.
https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description.h (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... chrome/common/media_router/dial_device_description.h:23: // Model name On 2017/03/18 18:56:57, mark a. foltz wrote: > Nit: Period at end Done. https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... chrome/common/media_router/dial_device_description.h:26: // The reported device type. E.g. urn:dial-multiscreen-org:device:dial:1 On 2017/03/18 18:56:57, mark a. foltz wrote: > Nit: type, e.g. Done. https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description_parser.mojom (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:17: // Model name On 2017/03/18 18:56:57, mark a. foltz wrote: > Period at end Done. https://codereview.chromium.org/2745653008/diff/100001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:20: // The reported device type. E.g. urn:dial-multiscreen-org:device:dial:1 On 2017/03/18 18:56:57, mark a. foltz wrote: > type, e.g. Done. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser.cc:55: XmlReader xml_reader; On 2017/03/18 18:56:57, mark a. foltz wrote: > DCHECK(out) Done. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser.cc:81: auto model_name = out->model_name; On 2017/03/18 18:56:57, mark a. foltz wrote: > Slight preference to factor out a function like ComputeFriendlyName(out) to take > care of this. Done. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser.cc:108: xmlDocPtr xslt_doc = xmlParseMemory(xslt_text, sizeof(xslt_text)); On 2017/03/18 18:56:57, mark a. foltz wrote: > I'm leaning towards using string substitution for scrubbing. It's less code, > doesn't require parsing the document twice, and doesn't use a library I'm not > familiar with the security risk of and no one else seems to be using either. Done. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser.h:15: class DialDeviceDescriptionParser { On 2017/03/18 18:56:57, mark a. foltz wrote: > Does this need to be a class? There are no longer any data members. Done. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.h (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.h:13: class DialDeviceDescriptionParserImpl On 2017/03/18 18:56:57, mark a. foltz wrote: > Please document the thread safety of this class. Done. https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_unittest.cc (right): https://codereview.chromium.org/2745653008/diff/100001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_unittest.cc:68: std::string& Replace(std::string& input, On 2017/03/18 18:56:57, mark a. foltz wrote: > This doesn't need to be part of a class; you can declare it in an anonymous > namespace, remove DeviceDescriptionParserTest, and just use TEST() instead of > TEST_F below. Done.
LGTM with a few minor comments - you'll need to get SECURITY_OWNERS review as usual to add a new mojom. My comments about the API using an optional return value are mostly about trying to use consistent patterns across the several Mojo APIs we now maintain vs. anything particularly important about this API. https://codereview.chromium.org/2745653008/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:27: // |logging_xml|: xml file for logging without unique ID and serial number. Nit: s/xml file/serialized xml/ https://codereview.chromium.org/2745653008/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:46: // |result|: returns false if parsing fails. You could refer to the documentation for DeviceDescriptionCallback here to avoid repetition. https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description_parser.mojom (right): https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:29: // |logging_xml|: xml file for logging without unique ID and serial number. Is this empty if result == false? https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:30: ParseDialDeviceDescription(string device_description_xml_data) Another API would be: => (bool success, DialDeviceDescription? device_description) with the plan being that |success| is replaced by an enum indicating success or the failure reason in the future if need be. This way the service doesn't have to pass an empty (and invalid) description back on failure. It's also more consistent with other APIs we maintain that use optional return values with an error status, like PresentationService::StartPresentation(). It adds some complexity to the C++ API and that might outweigh any perceived benefit. Let me know what you think. https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:31: => (bool parse_result, s/parse_result/success/ ? https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:33: string logging_xml); Could logging_xml be a field in DialDeviceDescription? Or is it important to pass it separately? https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description_parser.typemap (right): https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.typemap:6: Super nit: I think other typemaps don't have empty lines between sections. https://codereview.chromium.org/2745653008/diff/120001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.cc (right): https://codereview.chromium.org/2745653008/diff/120001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:31: if (!out->friendly_name.empty()) For clarity I would move this check back out to the Parse method, as there's nothing to compute unless the friendly_name is already empty. https://codereview.chromium.org/2745653008/diff/120001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:39: model_name + "[" + unique_id.substr(unique_id.length() - 4) + "]"; Nit: Can you add a space before [ https://codereview.chromium.org/2745653008/diff/120001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:50: if (pos != std::string::npos && end_pos != std::string::npos) && end_pos > start_pos
https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description_parser.mojom (right): https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:33: string logging_xml); On 2017/03/21 at 01:39:21, mark a. foltz wrote: > Could logging_xml be a field in DialDeviceDescription? Or is it important to pass it separately? I realize that passing separately means we can log it in the caller, even if parsing fails and we don't return a DialDeviceDescription. So let's keep it separate :)
zhaobin@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng@ to review mojo related changes. https://codereview.chromium.org/2745653008/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:27: // |logging_xml|: xml file for logging without unique ID and serial number. On 2017/03/21 01:39:21, mark a. foltz wrote: > Nit: s/xml file/serialized xml/ Done. https://codereview.chromium.org/2745653008/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:46: // |result|: returns false if parsing fails. On 2017/03/21 01:39:21, mark a. foltz wrote: > You could refer to the documentation for DeviceDescriptionCallback here to avoid > repetition. Done. https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description_parser.mojom (right): https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:29: // |logging_xml|: xml file for logging without unique ID and serial number. On 2017/03/21 01:39:21, mark a. foltz wrote: > Is this empty if result == false? Removed :) https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:30: ParseDialDeviceDescription(string device_description_xml_data) On 2017/03/21 01:39:21, mark a. foltz wrote: > Another API would be: > > => (bool success, DialDeviceDescription? device_description) > > with the plan being that |success| is replaced by an enum indicating success or > the failure reason in the future if need be. > > This way the service doesn't have to pass an empty (and invalid) description > back on failure. It's also more consistent with other APIs we maintain that > use optional return values with an error status, like > PresentationService::StartPresentation(). > > It adds some complexity to the C++ API and that might outweigh any perceived > benefit. Let me know what you think. Done. https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:31: => (bool parse_result, On 2017/03/21 01:39:21, mark a. foltz wrote: > s/parse_result/success/ ? Done. https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:33: string logging_xml); On 2017/03/21 01:41:10, mark a. foltz wrote: > On 2017/03/21 at 01:39:21, mark a. foltz wrote: > > Could logging_xml be a field in DialDeviceDescription? Or is it important to > pass it separately? > > I realize that passing separately means we can log it in the caller, even if > parsing fails and we don't return a DialDeviceDescription. So let's keep it > separate :) Removed logging related code in this patch. Will add them to DialMediaSink patch (in future), which uses SafeDialDeviceDescriptionParser. https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description_parser.typemap (right): https://codereview.chromium.org/2745653008/diff/120001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.typemap:6: On 2017/03/21 01:39:21, mark a. foltz wrote: > Super nit: I think other typemaps don't have empty lines between sections. Done. https://codereview.chromium.org/2745653008/diff/120001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.cc (right): https://codereview.chromium.org/2745653008/diff/120001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:31: if (!out->friendly_name.empty()) On 2017/03/21 01:39:21, mark a. foltz wrote: > For clarity I would move this check back out to the Parse method, as there's > nothing to compute unless the friendly_name is already empty. Done. https://codereview.chromium.org/2745653008/diff/120001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:39: model_name + "[" + unique_id.substr(unique_id.length() - 4) + "]"; On 2017/03/21 01:39:21, mark a. foltz wrote: > Nit: Can you add a space before [ Done. https://codereview.chromium.org/2745653008/diff/120001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:50: if (pos != std::string::npos && end_pos != std::string::npos) On 2017/03/21 01:39:22, mark a. foltz wrote: > && end_pos > start_pos Done.
https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:7: #include <utility> Is this used? https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:9: #include "chrome/common/extensions/chrome_utility_extensions_messages.h" Not used? https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:14: using content::BrowserThread; This doesn't seem to be used - remove? https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:50: this)); Unless you have ref counting for other purposes, can this be base::Unretained(this)? It looks like the callback won't get invoked after this class is destroyed. https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. Seems like this file should be in c/b/m/r/discovery/dial to be consistent with https://codereview.chromium.org/2756483007 https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:8: #include "base/callback.h" #include <memory> #include <string> https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:22: : public base::RefCounted<SafeDialDeviceDescriptionParser> { why does this need to be ref counted? https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:26: // |result|: returns false if parsing fails. s/result/success https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:46: bool result, s/result/success https://codereview.chromium.org/2745653008/diff/140001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description.h (right): https://codereview.chromium.org/2745653008/diff/140001/chrome/common/media_ro... chrome/common/media_router/dial_device_description.h:4: #ifndef CHROME_COMMON_MEDIA_ROUTER_DIAL_DEVICE_DESCRIPTION_H_ Insert empty line above https://codereview.chromium.org/2745653008/diff/140001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.cc (right): https://codereview.chromium.org/2745653008/diff/140001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:17: static std::string Validate( This does not need the static keyword. https://codereview.chromium.org/2745653008/diff/140001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:36: model_name + " [" + unique_id.substr(unique_id.length() - 4) + "]"; Consider using base::StringPrintf here. https://codereview.chromium.org/2745653008/diff/140001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:63: bool result = Parse(device_description_xml_data, &device_description); If Parse returned false we shouldn't be sending back the partially constructed device description.
https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:7: #include <utility> On 2017/03/22 00:31:46, imcheng wrote: > Is this used? Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:9: #include "chrome/common/extensions/chrome_utility_extensions_messages.h" On 2017/03/22 00:31:46, imcheng wrote: > Not used? Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:14: using content::BrowserThread; On 2017/03/22 00:31:46, imcheng wrote: > This doesn't seem to be used - remove? Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.cc:50: this)); On 2017/03/22 00:31:46, imcheng wrote: > Unless you have ref counting for other purposes, can this be > base::Unretained(this)? It looks like the callback won't get invoked after this > class is destroyed. Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/safe_dial_device_description_parser.h (right): https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/03/22 00:31:46, imcheng wrote: > Seems like this file should be in c/b/m/r/discovery/dial to be consistent with > https://codereview.chromium.org/2756483007 Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:8: #include "base/callback.h" On 2017/03/22 00:31:46, imcheng wrote: > #include <memory> > #include <string> Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:22: : public base::RefCounted<SafeDialDeviceDescriptionParser> { On 2017/03/22 00:31:46, imcheng wrote: > why does this need to be ref counted? Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:26: // |result|: returns false if parsing fails. On 2017/03/22 00:31:47, imcheng wrote: > s/result/success Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/safe_dial_device_description_parser.h:46: bool result, On 2017/03/22 00:31:46, imcheng wrote: > s/result/success Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description.h (right): https://codereview.chromium.org/2745653008/diff/140001/chrome/common/media_ro... chrome/common/media_router/dial_device_description.h:4: #ifndef CHROME_COMMON_MEDIA_ROUTER_DIAL_DEVICE_DESCRIPTION_H_ On 2017/03/22 00:31:47, imcheng wrote: > Insert empty line above Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.cc (right): https://codereview.chromium.org/2745653008/diff/140001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:17: static std::string Validate( On 2017/03/22 00:31:47, imcheng wrote: > This does not need the static keyword. Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:36: model_name + " [" + unique_id.substr(unique_id.length() - 4) + "]"; On 2017/03/22 00:31:47, imcheng wrote: > Consider using base::StringPrintf here. Done. https://codereview.chromium.org/2745653008/diff/140001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:63: bool result = Parse(device_description_xml_data, &device_description); On 2017/03/22 00:31:47, imcheng wrote: > If Parse returned false we shouldn't be sending back the partially constructed > device description. Done.
Sorry for the slow review. https://codereview.chromium.org/2745653008/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:58: DCHECK(!device_description_callback_.is_null()); Nit: be consistent about using is_null() or omitting it in DCHECKs (line 24 omits, while line 58 and 67 both use is_null) https://codereview.chromium.org/2745653008/diff/160001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description_parser.mojom (right): https://codereview.chromium.org/2745653008/diff/160001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:28: // |device_description|: device description object. Empty if parsing fails. Is it possible to link to some sort of spec for the device description? It'd be useful to know what it's supposed to look like. https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.cc (right): https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:24: return ""; Nit: return std::string() https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:30: auto model_name = out->model_name; nit: const auto& to avoid a copy (same below) https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:53: mojo::MakeStrongBinding(base::MakeUnique<DialDeviceDescriptionParserImpl>(), #include "base/memory/ptr_util.h" for MakeUnique https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:54: std::move(request)); Nit: #include <utility> https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:104: std::string error = Validate(*out); Will this string eventually be returned to the user? If not, I'd suggest using an enum (the string itself probably won't be optimized out) https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.h (right): https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.h:34: TestParseWithoutFriendlyName); FWIW, it might be easier to just add a test fixture, add a helper to call Parse on the test fixture, and just friend the one test fixture. Otherwise, please #include "base/gtest_prod_util.h" for this macro.
https://codereview.chromium.org/2745653008/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:58: DCHECK(!device_description_callback_.is_null()); On 2017/03/22 09:53:02, dcheng wrote: > Nit: be consistent about using is_null() or omitting it in DCHECKs (line 24 > omits, while line 58 and 67 both use is_null) Done. https://codereview.chromium.org/2745653008/diff/160001/chrome/common/media_ro... File chrome/common/media_router/dial_device_description_parser.mojom (right): https://codereview.chromium.org/2745653008/diff/160001/chrome/common/media_ro... chrome/common/media_router/dial_device_description_parser.mojom:28: // |device_description|: device description object. Empty if parsing fails. On 2017/03/22 09:53:02, dcheng wrote: > Is it possible to link to some sort of spec for the device description? It'd be > useful to know what it's supposed to look like. Done. https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.cc (right): https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:24: return ""; On 2017/03/22 09:53:02, dcheng wrote: > Nit: return std::string() Done. https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:30: auto model_name = out->model_name; On 2017/03/22 09:53:02, dcheng wrote: > nit: const auto& to avoid a copy (same below) Done. https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:53: mojo::MakeStrongBinding(base::MakeUnique<DialDeviceDescriptionParserImpl>(), On 2017/03/22 09:53:02, dcheng wrote: > #include "base/memory/ptr_util.h" for MakeUnique Done. https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:54: std::move(request)); On 2017/03/22 09:53:02, dcheng wrote: > Nit: #include <utility> Done. https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:104: std::string error = Validate(*out); On 2017/03/22 09:53:02, dcheng wrote: > Will this string eventually be returned to the user? If not, I'd suggest using > an enum (the string itself probably won't be optimized out) Done. https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.h (right): https://codereview.chromium.org/2745653008/diff/160001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.h:34: TestParseWithoutFriendlyName); On 2017/03/22 09:53:02, dcheng wrote: > FWIW, it might be easier to just add a test fixture, add a helper to call Parse > on the test fixture, and just friend the one test fixture. > > Otherwise, please #include "base/gtest_prod_util.h" for this macro. Done.
lgtm
zhaobin@chromium.org changed reviewers: + jam@chromium.org
+jam@ to review following files: chrome/utility/BUILD.gn chrome/utility/chrome_content_utility_client.cc mojo/public/tools/bindings/chromium_bindings_configuration.gni
On 2017/03/22 21:18:41, zhaobin wrote: > +jam@ to review following files: > > chrome/utility/BUILD.gn > chrome/utility/chrome_content_utility_client.cc > mojo/public/tools/bindings/chromium_bindings_configuration.gni I couldn't find a doc or mo
On 2017/03/22 21:18:41, zhaobin wrote: > +jam@ to review following files: > > chrome/utility/BUILD.gn > chrome/utility/chrome_content_utility_client.cc > mojo/public/tools/bindings/chromium_bindings_configuration.gni I couldn't find a doc or mo
On 2017/03/22 21:18:41, zhaobin wrote: > +jam@ to review following files: > > chrome/utility/BUILD.gn > chrome/utility/chrome_content_utility_client.cc > mojo/public/tools/bindings/chromium_bindings_configuration.gni I couldn't find a doc or mo
sorry hit enter accidentally. I was saying I couldn't find background for why this has to be done in a utility process. What is the source of the data, untrusted content? Does it have to be XML or can it be JSON? AFAIK we trust the base JSON parser to run it in the browser. Also, in response to what user action does this operation run?
On 2017/03/23 00:41:20, jam wrote: > sorry hit enter accidentally. > > I was saying I couldn't find background for why this has to be done in a utility > process. What is the source of the data, untrusted content? Does it have to be > XML or can it be JSON? AFAIK we trust the base JSON parser to run it in the > browser. Also, in response to what user action does this operation run? We are trying to move device discovery from component extension to browser (Design doc https://docs.google.com/document/d/1_ccuW8mU1Nky6oprVLiKvWZ91JxGl45l-LzrOvviy...). Code involves parsing device description XML and security team suggests that we should do it in utility process instead of browser process (crbug.com/699306). The device description XML comes from devices such as Samsung TV and is untrusted. It has to be XML. We will start device discovery (utility process) when media router component extension is registered with browser process, and periordically (>= 2 mins interval) if device list has been updated.
sorry for delay, I missed this. So this is fine, but I have one question: why do we need typemapping? The code in the browser could just use the mojom struct and avoid the need for the duplicate mojom definition and typemapping.
On 2017/03/24 17:44:15, jam wrote: > sorry for delay, I missed this. > > So this is fine, but I have one question: why do we need typemapping? The code > in the browser could just use the mojom struct and avoid the need for the > duplicate mojom definition and typemapping. Talked with zhaobin@ offline. It's fine to use the Mojo struct directly here. However, we do want to be a bit cautious about where we use Mojo structs directly until https://crbug.com/651711 is addressed. It is at least somewhat common for struct traits to express additional validation logic (for example, that a GURL is < a certain length). With pure Mojo structs, that is currently impossible to express. Concrete example from this CL: I wasn't sure if media router needed to validate the UUID field looked like a UUID (or used it in ways that would depend on a correct structure). But it seems like media router simply cares if this value is empty or not--so a string is fine for now.
On 2017/03/24 17:44:15, jam wrote: > sorry for delay, I missed this. > > So this is fine, but I have one question: why do we need typemapping? The code > in the browser could just use the mojom struct and avoid the need for the > duplicate mojom definition and typemapping. Removed typemapping and use mojo types directly.
The CQ bit was checked by zhaobin@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...
https://codereview.chromium.org/2745653008/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:68: chrome::mojom::DialDeviceDescriptionPtr()); Note: it's legal to just pass nullptr here https://codereview.chromium.org/2745653008/diff/200001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.cc (right): https://codereview.chromium.org/2745653008/diff/200001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:84: chrome::mojom::DialDeviceDescriptionPtr& out) { Nit: mutable ref arguments aren't permitted in Chrome. One option is to just have this function return the struct directly (which should obviate the need to have a result parameter in the mojom at all: we can just check that we have a result)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 zhaobin@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm
The CQ bit was checked by zhaobin@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...
https://codereview.chromium.org/2745653008/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2745653008/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:68: chrome::mojom::DialDeviceDescriptionPtr()); On 2017/03/25 00:59:38, dcheng wrote: > Note: it's legal to just pass nullptr here Done. https://codereview.chromium.org/2745653008/diff/200001/chrome/utility/media_r... File chrome/utility/media_router/dial_device_description_parser_impl.cc (right): https://codereview.chromium.org/2745653008/diff/200001/chrome/utility/media_r... chrome/utility/media_router/dial_device_description_parser_impl.cc:84: chrome::mojom::DialDeviceDescriptionPtr& out) { On 2017/03/25 00:59:38, dcheng wrote: > Nit: mutable ref arguments aren't permitted in Chrome. > > One option is to just have this function return the struct directly (which > should obviate the need to have a result parameter in the mojom at all: we can > just check that we have a result) Done.
The CQ bit was checked by zhaobin@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.
still lgtm
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2745653008/#ps280001 (title: "fix android build error")
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": 280001, "attempt_start_ts": 1490652326845880,
"parent_rev": "0bebf0ea569d282c3db14f067b5e8fdebf023c34", "commit_rev":
"687cddcf3340fb4e163ec55462dd2a66b9167774"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Parse device description xml in utility process - Add dial_device_description_parser.mojom to support communication between browser and utility process - Add DialDeviceDescriptionParser in chrome/utility to handle xml parsing and scrubbing - Add SafeDialDeviceDescriptionParser in chrome/browser to start parsing in utility process BUG=687375 ========== to ========== [Media Router] Parse device description xml in utility process - Add dial_device_description_parser.mojom to support communication between browser and utility process - Add DialDeviceDescriptionParser in chrome/utility to handle xml parsing and scrubbing - Add SafeDialDeviceDescriptionParser in chrome/browser to start parsing in utility process BUG=687375 Review-Url: https://codereview.chromium.org/2745653008 Cr-Commit-Position: refs/heads/master@{#459902} Committed: https://chromium.googlesource.com/chromium/src/+/687cddcf3340fb4e163ec55462dd... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001) as https://chromium.googlesource.com/chromium/src/+/687cddcf3340fb4e163ec55462dd... |
