Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1040773002:
Limit number of service instances passed to onServiceList listeners. (Closed)
|
Created:
5 years, 9 months ago by Red Daly Modified:
5 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLimit number of service instances passed to onServiceList listeners.
BUG=451527
R=mfoltz@chromium.org,kalman@chromium.org
Committed: https://crrev.com/76db6018031f3eb7dd1ae26c92fd94a353078b0f
Cr-Commit-Position: refs/heads/master@{#324345}
Patch Set 1 #Patch Set 2 : Service instance limit and unit test against HEAD of master #
Total comments: 13
Patch Set 3 : Add new mdns.getMaxServiceInstancesPerEvent() function and update unit tests #
Total comments: 5
Patch Set 4 : Sync to HEAD after submission of const support in API .idl files #Patch Set 5 : Define const maxServiceInstancesPerEvent and send console.warning message when limit is hit #
Total comments: 5
Patch Set 6 : Change mdns.maxServiceInstancesPerEvent to mdns.MAX_SERVICE_INSTANCES_PER_EVENT, use StringPrintf #
Total comments: 4
Patch Set 7 : Share code between two MDnsAPI methods that need a list of valid listeners, and send console messag… #Patch Set 8 : Fix typo that prevented error console messages. #Patch Set 9 : Tidy up warning message #Patch Set 10 : Add missing semicolon #
Total comments: 12
Patch Set 11 : Change GetValidOnServiceListListeners() signature and mdns browser test #
Total comments: 8
Patch Set 12 : Modify GetValidOnServiceListListeners logic #
Total comments: 8
Patch Set 13 : Style fixes and extra explanatory comment #Patch Set 14 : Rebase changes onto HEAD and compress into a single commit #Patch Set 15 : Add testing:: prefix before MakeMatcher to fix windows build #
Messages
Total messages: 33 (5 generated)
Please hold off on review for now as many of the changes here are redundant. Once change 668983003 is submitted, I'll rebase and this change will be fairly trivial.
On 2015/03/27 19:19:02, Red Daly wrote: > Please hold off on review for now as many of the changes here are redundant. > Once change 668983003 is submitted, I'll rebase and this change will be fairly > trivial. This is now ready for review (Patch Set 2).
https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:27: const uint8 kMaxServicesPerOnServiceListEvent = 64; You can, and should, declare this constant in the schema file. You can access that from here, and - it'll be automatically generated as part of the API documentation. - be available to developers programmatically. https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:146: it != services.end() && args.size() < kMaxServicesPerOnServiceListEvent; It would be nice to warn the developer somehow if this will fail. Silently limiting could be hard to debug. https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api_unittest.cc (right): https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:303: for (int i=0; i < 80; ++i) { when it's a shared constant, use that rather than "80". https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:310: .Times(1); I dunno - a mock here seems pretty complex and overkill. Can you test it in a more principled way, like add the services, then get them back and assert that they're limited? another thing to check, and think about, is how eviction should work. Should earlier registrations be evicted, vs later registrations ignored?
https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:146: it != services.end() && args.size() < kMaxServicesPerOnServiceListEvent; On 2015/03/30 17:08:50, kalman wrote: > It would be nice to warn the developer somehow if this will fail. Silently > limiting could be hard to debug. That would depend on the number of devices advertising on the LAN, which is hard to tell in advance. https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api_unittest.cc (right): https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:276: return service_count_matcher_.MatchAndExplain(services->GetSize(), It seems like all you really care about is whether the service list is equal to the maximum allowed size. I wonder if you need the custom matcher or can just match on SizeIs(kMaxServices). https://code.google.com/p/googlemock/wiki/V1_7_CheatSheet#Container_Matchers
https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:27: const uint8 kMaxServicesPerOnServiceListEvent = 64; On 2015/03/30 17:08:50, kalman wrote: > You can, and should, declare this constant in the schema file. You can access > that from here, and > - it'll be automatically generated as part of the API documentation. > - be available to developers programmatically. I didn't find great support for constants in the IDL for these APIs: https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i... In web IDL there is a concept of a "const," but that isn't part of the IDL available to the mdns API. I see two possibilities: 1) A new interface ("Functions") with a method "static long getMaxServiceInstancesPerEvent ();" 2) A new enum ("Constants") with a member MaxServiceInstancesPerEvent. 2 seems like a perversion of enum semantics. I implemented 1; let me know if you think it's a good solution. https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:146: it != services.end() && args.size() < kMaxServicesPerOnServiceListEvent; On 2015/03/30 17:57:22, mark a. foltz wrote: > On 2015/03/30 17:08:50, kalman wrote: > > It would be nice to warn the developer somehow if this will fail. Silently > > limiting could be hard to debug. > > That would depend on the number of devices advertising on the LAN, which is hard > to tell in advance. I added a VLOG(1) message, which will help a developer who looks at VLOG messages. Beyond that, I could change the callback interface to take another argument to indicate truncation (overkill), or output a warning to the JavaScript console. Is the JavaScript console output worth doing? https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api_unittest.cc (right): https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:276: return service_count_matcher_.MatchAndExplain(services->GetSize(), On 2015/03/30 17:57:22, mark a. foltz wrote: > It seems like all you really care about is whether the service list is equal to > the maximum allowed size. I wonder if you need the custom matcher or can just > match on SizeIs(kMaxServices). > > https://code.google.com/p/googlemock/wiki/V1_7_CheatSheet#Container_Matchers > > I think SizeIs() only works with stl containers, not a ListValue. Instead I got rid of the custom matcher argument and just do a comparison using testing::Eq(x). https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:303: for (int i=0; i < 80; ++i) { On 2015/03/30 17:08:50, kalman wrote: > when it's a shared constant, use that rather than "80". Done, except it is a new function rather than a shared constant. https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:310: .Times(1); On 2015/03/30 17:08:50, kalman wrote: > I dunno - a mock here seems pretty complex and overkill. Can you test it in a > more principled way, like add the services, then get them back and assert that > they're limited? The MDnsAPI object does not hold onto the event it generates, so it's not straightforward to get the truncated service list back without a change in interface. Are you suggesting changing the interface? > another thing to check, and think about, is how eviction should > work. Should earlier registrations be evicted, vs later registrations ignored? There are arguments for both of those strategies. One way or the other, the extension will not see services that are actually available. - Evicting older services will mean the API provides extensions with a less stable list. - Evicting older services allows an extension to see more of the available services on a network, getting around the service limit. - Suppressing new services prevents newly discovered services from appearing, perhaps interesting ones like a just-plugged-in device. I'm content to leave this as an optimization in case the 64-service limit is a problem for someone in the future.
https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:146: it != services.end() && args.size() < kMaxServicesPerOnServiceListEvent; On 2015/03/31 19:23:50, Red Daly wrote: > On 2015/03/30 17:57:22, mark a. foltz wrote: > > On 2015/03/30 17:08:50, kalman wrote: > > > It would be nice to warn the developer somehow if this will fail. Silently > > > limiting could be hard to debug. > > > > That would depend on the number of devices advertising on the LAN, which is > hard > > to tell in advance. > > I added a VLOG(1) message, which will help a developer who looks at VLOG > messages. Beyond that, I could change the callback interface to take another > argument to indicate truncation (overkill), or output a warning to the > JavaScript console. Is the JavaScript console output worth doing? JS console output is worth doing insofar as VLOG output is unlikely to get noticed, and this is for developer's help after all. It's simple enough: https://code.google.com/p/chromium/codesearch#search/&q=%22new%20ExtensionMsg... Incidentally: can all of this be done at registration time, rather than at event sending time? At least at the warning level? I think that would be better. https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/mdns.idl (right): https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... chrome/common/extensions/api/mdns.idl:35: // The maximum number of service instances that will be included in Sorry for the delay with this review, but I'm adding support for constants here: https://codereview.chromium.org/1051563002 after which you'd be able to write this properly (if unfortunately syntactically) as: interface Properties { // The maximum number... [value=64] static long maxServiceInstancesPerEvent(); } pretty similar to what you have, just it'd be a property rather than a getter (and therefore accessible synchronously). Also, the typical style for constants has been MAX_SERVICE_INSTANCES_PER_EVENT. Up to you whether you want to wait for that CL to go in I suppose, but consider it.
https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/mdns.idl (right): https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... chrome/common/extensions/api/mdns.idl:35: // The maximum number of service instances that will be included in On 2015/04/01 20:25:58, kalman wrote: > Sorry for the delay with this review, but I'm adding support for constants here: > https://codereview.chromium.org/1051563002 after which you'd be able to write > this properly (if unfortunately syntactically) as: > > interface Properties { > // The maximum number... > [value=64] static long maxServiceInstancesPerEvent(); > } > > pretty similar to what you have, just it'd be a property rather than a getter > (and therefore accessible synchronously). > > Also, the typical style for constants has been MAX_SERVICE_INSTANCES_PER_EVENT. > > Up to you whether you want to wait for that CL to go in I suppose, but consider > it. That works for me except I'm trying to commit before Friday's branch. If the other CL lands before end of tomorrow, I can change this API accordingly.
On 2015/04/01 22:52:05, Red Daly wrote: > https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... > File chrome/common/extensions/api/mdns.idl (right): > > https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... > chrome/common/extensions/api/mdns.idl:35: // The maximum number of service > instances that will be included in > On 2015/04/01 20:25:58, kalman wrote: > > Sorry for the delay with this review, but I'm adding support for constants > here: > > https://codereview.chromium.org/1051563002 after which you'd be able to write > > this properly (if unfortunately syntactically) as: > > > > interface Properties { > > // The maximum number... > > [value=64] static long maxServiceInstancesPerEvent(); > > } > > > > pretty similar to what you have, just it'd be a property rather than a getter > > (and therefore accessible synchronously). > > > > Also, the typical style for constants has been > MAX_SERVICE_INSTANCES_PER_EVENT. > > > > Up to you whether you want to wait for that CL to go in I suppose, but > consider > > it. > > That works for me except I'm trying to commit before Friday's branch. If the > other CL lands before end of tomorrow, I can change this API accordingly. I see. Branch on Friday, that's news to me. Alright - well I don't think we should submit this as an actual API like this, so I'll hustle to get that other patch in :)
> I see. Branch on Friday, that's news to me. Alright - well I don't think we > should submit this as an actual API like this, so I'll hustle to get that other > patch in :) Patch has been submitted.
https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:146: it != services.end() && args.size() < kMaxServicesPerOnServiceListEvent; On 2015/04/01 20:25:58, kalman wrote: > On 2015/03/31 19:23:50, Red Daly wrote: > > On 2015/03/30 17:57:22, mark a. foltz wrote: > > > On 2015/03/30 17:08:50, kalman wrote: > > > > It would be nice to warn the developer somehow if this will fail. Silently > > > > limiting could be hard to debug. > > > > > > That would depend on the number of devices advertising on the LAN, which is > > hard > > > to tell in advance. > > > > I added a VLOG(1) message, which will help a developer who looks at VLOG > > messages. Beyond that, I could change the callback interface to take another > > argument to indicate truncation (overkill), or output a warning to the > > JavaScript console. Is the JavaScript console output worth doing? > > JS console output is worth doing insofar as VLOG output is unlikely to get > noticed, and this is for developer's help after all. > > It's simple enough: > https://code.google.com/p/chromium/codesearch#search/&q=%22new%20ExtensionMsg... > > Incidentally: can all of this be done at registration time, rather than at event > sending time? At least at the warning level? I think that would be better. I modified this to do JS console output but didn't yet make the change to warn at registration time. I'm worried a warning at registration time would spam the error console too much to educate about a rather rare case. Do you think the new const is enough of a warning to the developer? https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/mdns.idl (right): https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... chrome/common/extensions/api/mdns.idl:35: // The maximum number of service instances that will be included in On 2015/04/01 22:52:05, Red Daly wrote: > On 2015/04/01 20:25:58, kalman wrote: > > Sorry for the delay with this review, but I'm adding support for constants > here: > > https://codereview.chromium.org/1051563002 after which you'd be able to write > > this properly (if unfortunately syntactically) as: > > > > interface Properties { > > // The maximum number... > > [value=64] static long maxServiceInstancesPerEvent(); > > } > > > > pretty similar to what you have, just it'd be a property rather than a getter > > (and therefore accessible synchronously). > > > > Also, the typical style for constants has been > MAX_SERVICE_INSTANCES_PER_EVENT. > > > > Up to you whether you want to wait for that CL to go in I suppose, but > consider > > it. > > That works for me except I'm trying to commit before Friday's branch. If the > other CL lands before end of tomorrow, I can change this API accordingly. Thanks for that - updated. Please take another look. Note that I didn't update to use the MAX_SERVICE_INSTANCES_PER_EVENT style because I interpret your comment to mean the CAPS_STYLE is for C++ code. I think camelCase is the convention in JavaScript APIs.
https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/mdns.idl (right): https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... chrome/common/extensions/api/mdns.idl:35: // The maximum number of service instances that will be included in > > Note that I didn't update to use the MAX_SERVICE_INSTANCES_PER_EVENT style > because I interpret your comment to mean the CAPS_STYLE is for C++ code. I > think camelCase is the convention in JavaScript APIs. AFAIK Caps is the style for web APIs, e.g. see the constants in KeyboardEvent. https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:151: << api::mdns::maxServiceInstancesPerEvent; I think it's more common to use StringPrintf rather than streams for this sort of thing. https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:180: void MDnsAPI::WriteToConsole(const std::string& service_type, I'm a bit lost sorry - why is so much code needed to log a message to the JS console?
https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/mdns.idl (right): https://codereview.chromium.org/1040773002/diff/40001/chrome/common/extension... chrome/common/extensions/api/mdns.idl:35: // The maximum number of service instances that will be included in On 2015/04/02 22:36:22, kalman wrote: > > > > Note that I didn't update to use the MAX_SERVICE_INSTANCES_PER_EVENT style > > because I interpret your comment to mean the CAPS_STYLE is for C++ code. I > > think camelCase is the convention in JavaScript APIs. > > AFAIK Caps is the style for web APIs, e.g. see the constants in KeyboardEvent. Done. https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:151: << api::mdns::maxServiceInstancesPerEvent; Done. https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:180: void MDnsAPI::WriteToConsole(const std::string& service_type, On 2015/04/02 22:36:22, kalman wrote: > I'm a bit lost sorry - why is so much code needed to log a message to the JS > console? There is one JS console per extension. This code gets the list of relevant extensions for a given mDNS service type and sends a message to each console.
https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:180: void MDnsAPI::WriteToConsole(const std::string& service_type, On 2015/04/03 17:58:55, Red Daly wrote: > On 2015/04/02 22:36:22, kalman wrote: > > I'm a bit lost sorry - why is so much code needed to log a message to the JS > > console? > > There is one JS console per extension. This code gets the list of relevant > extensions for a given mDNS service type and sends a message to each console. Ohh I see. So these service registation restrictions are actually shared across Extensions? That's the way I read the code as well, in OnDnsSendEvent? The problem with that is if an app or an extension registers for a lot of services, it could starve other extensions. The restrictions should be per extension, and that is what the bug appears to be filed about. It seems like UpdateMdnsListeners is the right place to do this, you already have the extension whitelist check in there. Sorry if I'm misunderstanding.
On 2015/04/03 18:16:58, kalman wrote: > https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... > File chrome/browser/extensions/api/mdns/mdns_api.cc (right): > > https://codereview.chromium.org/1040773002/diff/80001/chrome/browser/extensio... > chrome/browser/extensions/api/mdns/mdns_api.cc:180: void > MDnsAPI::WriteToConsole(const std::string& service_type, > On 2015/04/03 17:58:55, Red Daly wrote: > > On 2015/04/02 22:36:22, kalman wrote: > > > I'm a bit lost sorry - why is so much code needed to log a message to the JS > > > console? > > > > There is one JS console per extension. This code gets the list of relevant > > extensions for a given mDNS service type and sends a message to each console. > > Ohh I see. So these service registation restrictions are actually shared across > Extensions? That's the way I read the code as well, in OnDnsSendEvent? > > The problem with that is if an app or an extension registers for a lot of > services, it could starve other extensions. The restrictions should be per > extension, and that is what the bug appears to be filed about. > > It seems like UpdateMdnsListeners is the right place to do this, you already > have the extension whitelist check in there. > > Sorry if I'm misunderstanding. UpdateMDnsListeners listens for when an extension calls chrome.mdns.onServiceList.addListener. It isn't the right part of the flow to restrict the number of service _instances_ per client. Note that this change only restrict the number of service instances per client, not the service types per client. The service type limit was already added in a separate change: https://codereview.chromium.org/1005903005. To restrict the service instances per client, I'm truncating the service instances property of the chrome.mdns.onServiceList event, which seems to correspond precisely to the bug request: "I think having reasonable limits on [...] the number of services reported per event handler [is] pretty easy to do."
Ok, I guess I don't fully understand the goal here then. Anyway - the API and extensions stuff lgtm but Mark should review this as well obviously. https://codereview.chromium.org/1040773002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:185: // Check all listeners for service type filters. Much of this code does look very similar to what's already in UpdateMDnsListeners, can you unify it somehow? https://codereview.chromium.org/1040773002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:223: rvh->GetRoutingID(), You should be able to send directly to the RenderViewHost, no need to fetch the RenderProcessHost.
Thanks for the review. https://codereview.chromium.org/1040773002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:185: // Check all listeners for service type filters. On 2015/04/03 20:36:00, kalman wrote: > Much of this code does look very similar to what's already in > UpdateMDnsListeners, can you unify it somehow? Done. https://codereview.chromium.org/1040773002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:223: rvh->GetRoutingID(), On 2015/04/03 20:35:59, kalman wrote: > You should be able to send directly to the RenderViewHost, no need to fetch the > RenderProcessHost. Done.
https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:91: GetValidOnServiceListListeners(nullptr, &new_service_types); What is this nullptr argument? Optional arguments are discouraged. Can you have the function new up a set<> and return it instead? https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:120: api::mdns::MAX_SERVICE_INSTANCES_PER_EVENT) { Can this constant be declared as size_t? https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:124: "Truncating number of service instances in " Is there a prefix logged for this? If not, prepend [chrome.mdns] https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:126: api::mdns::MAX_SERVICE_INSTANCES_PER_EVENT)); This is not the most meaningful way of notifying the application that something bad happened. It will go to the user's console (which most users don't look at) and the developer will be none the wiser. Instead, changing the event to pass the number of discovered instances would allow the caller to know when the list is truncated and tell the user something meaningful in the extension/app. I guess this is okay for now (especially since the practical circumstances when this would be hit are few and far between). https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api_unittest.cc (right): https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:312: testing::Pointee(EventServiceListSize(size_t(64))))) I thought we just needed a regular matcher like testing::Eq(x) here? https://codereview.chromium.org/1040773002/diff/180001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/mdns/api/get_max_service_instances.js (right): https://codereview.chromium.org/1040773002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/api_test/mdns/api/get_max_service_instances.js:8: chrome.test.assertEq(64, chrome.mdns.MAX_SERVICE_INSTANCES_PER_EVENT); This is testing the value of a constant? Is it really worth it?
https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:91: GetValidOnServiceListListeners(nullptr, &new_service_types); On 2015/04/03 23:26:40, mark a. foltz wrote: > What is this nullptr argument? Optional arguments are discouraged. > > Can you have the function new up a set<> and return it instead? Done. I did it the other way to reduce code duplication (one caller needed the service types but not the listeners). https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:120: api::mdns::MAX_SERVICE_INSTANCES_PER_EVENT) { On 2015/04/03 23:26:40, mark a. foltz wrote: > Can this constant be declared as size_t? I don't think there's currently support for that in the IDL parser: https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i... https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:124: "Truncating number of service instances in " On 2015/04/03 23:26:40, mark a. foltz wrote: > Is there a prefix logged for this? If not, prepend [chrome.mdns] Done. https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:126: api::mdns::MAX_SERVICE_INSTANCES_PER_EVENT)); On 2015/04/03 23:26:39, mark a. foltz wrote: > This is not the most meaningful way of notifying the application that something > bad happened. It will go to the user's console (which most users don't look at) > and the developer will be none the wiser. > > Instead, changing the event to pass the number of discovered instances would > allow the caller to know when the list is truncated and tell the user something > meaningful in the extension/app. > > I guess this is okay for now (especially since the practical circumstances when > this would be hit are few and far between). I agree, added a TODO for the future enhancement. https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api_unittest.cc (right): https://codereview.chromium.org/1040773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:312: testing::Pointee(EventServiceListSize(size_t(64))))) On 2015/04/03 23:26:40, mark a. foltz wrote: > I thought we just needed a regular matcher like testing::Eq(x) here? I would prefer a simpler matcher but couldn't figure out to do it. The problem is I'm trying to match an argument in the last call to event_router()->BroadcastEvent(...). For that I think you need an EXPECT_CALL and a non-trivial matcher. Sorry if I'm not understanding. https://codereview.chromium.org/1040773002/diff/180001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/mdns/api/get_max_service_instances.js (right): https://codereview.chromium.org/1040773002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/api_test/mdns/api/get_max_service_instances.js:8: chrome.test.assertEq(64, chrome.mdns.MAX_SERVICE_INSTANCES_PER_EVENT); On 2015/04/03 23:26:40, mark a. foltz wrote: > This is testing the value of a constant? Is it really worth it? Changed. It's worth checking that chrome.mdns.MAX_SERVICE_INSTNACES_PER_EVENT is defined, but the test doesn't need to check the value == 64.
Getting close. https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:162: std::set<const EventListener*> MDnsAPI::GetValidOnServiceListListeners() { So there are two places iterating over this, and both times they iterate again over the result to pull some data out. How about this as: void GetExtensionsAndServiceTypes(std::set<string>* extension_ids, std::set<string>* service_types) I don't feel strongly about it but it would make the calling code a bit cleaner. WDYT? https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:197: for (const EventListener* listener : GetValidOnServiceListListeners()) { How does this filter for |service_type|? https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.h (right): https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.h:78: // outputs them to |listeners| if |listeners| is non-null. Also outputs the There is no parameter named |listeners|. https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.h:81: std::set<const EventListener*> GetValidOnServiceListListeners(); This is just used for iteration, return a const_iterator instead.
https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:162: std::set<const EventListener*> MDnsAPI::GetValidOnServiceListListeners() { On 2015/04/06 23:01:43, mark a. foltz wrote: > So there are two places iterating over this, and both times they iterate again > over the result to pull some data out. > > How about this as: > > void GetExtensionsAndServiceTypes(std::set<string>* extension_ids, > std::set<string>* service_types) > > I don't feel strongly about it but it would make the calling code a bit cleaner. > WDYT? I like the idea - I had something similar in patch set 6. PTAL at the revised function. https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:197: for (const EventListener* listener : GetValidOnServiceListListeners()) { On 2015/04/06 23:01:43, mark a. foltz wrote: > How does this filter for |service_type|? Fixed. Thanks for catching that. https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.h (right): https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.h:78: // outputs them to |listeners| if |listeners| is non-null. Also outputs the On 2015/04/06 23:01:43, mark a. foltz wrote: > There is no parameter named |listeners|. Done. https://codereview.chromium.org/1040773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.h:81: std::set<const EventListener*> GetValidOnServiceListListeners(); On 2015/04/06 23:01:43, mark a. foltz wrote: > This is just used for iteration, return a const_iterator instead. N/A with new interface, unless you think it makes sense to take a const_iterator<std::string>* extension_ids argument instead of a std::set<std::string>* extension_ids argument.
lgtm % a few minor fixes https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:91: GetValidOnServiceListListeners("" /* sevice_type_filter */, service_type_filter https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:91: GetValidOnServiceListListeners("" /* sevice_type_filter */, What does an empty filter mean? Please document. https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:164: extensions::EventRouter::Get(browser_context_)->listeners() This indentation looks funny, but might be right. https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:166: base::DictionaryValue* filter = (listener->filter()); Extra ( )
https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:91: GetValidOnServiceListListeners("" /* sevice_type_filter */, On 2015/04/07 22:43:32, mark a. foltz wrote: > service_type_filter Done. https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:91: GetValidOnServiceListListeners("" /* sevice_type_filter */, On 2015/04/07 22:43:32, mark a. foltz wrote: > What does an empty filter mean? Please document. Done. Added explanation to inline comment; there was already some language in the header file. https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:164: extensions::EventRouter::Get(browser_context_)->listeners() On 2015/04/07 22:43:33, mark a. foltz wrote: > This indentation looks funny, but might be right. Acknowledged - changed to 2 spaces for indentation. I can't tell 100% from the C++ style guide but 2 spaces is the default: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html https://codereview.chromium.org/1040773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/mdns/mdns_api.cc:166: base::DictionaryValue* filter = (listener->filter()); On 2015/04/07 22:43:33, mark a. foltz wrote: > Extra ( ) Done.
The CQ bit was checked by reddaly@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/1040773002/#ps260001 (title: "Rebase changes onto HEAD and compress into a single commit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1040773002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by reddaly@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1040773002/#ps280001 (title: "Add testing:: prefix before MakeMatcher to fix windows build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1040773002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/76db6018031f3eb7dd1ae26c92fd94a353078b0f Cr-Commit-Position: refs/heads/master@{#324345} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
