Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
668983003:
Enable chrome.mdns for Chrome Apps (Closed)
|
Created:
6 years, 2 months ago by Red Daly Modified:
5 years, 9 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. |
DescriptionAllows all packaged apps to use the chrome.mdns API, and removes whitelist that restricts the set of service type that can be discovered.
R=mkwst@chromium.org,mfoltz@chromium.org
BUG=426500
Committed: https://crrev.com/68ea490084597d5d4640e782989c0a6a094dcd21
Cr-Commit-Position: refs/heads/master@{#322671}
Patch Set 1 #Patch Set 2 : Add one more extension to mdns whitelist. #Patch Set 3 : Open up access to chrome.mdns API to all packaged apps #
Total comments: 6
Patch Set 4 : Reinstate mdns service type whitelist for extensions and fix formatting. #
Total comments: 13
Patch Set 5 : Respond to kalman's comments. #
Total comments: 13
Patch Set 6 : Fix unsubscription bug and refactor according to comments #
Total comments: 4
Patch Set 7 : Get extension object using different means, and modify line spacing. #Patch Set 8 : Add unit test for MDnsAPI #
Total comments: 6
Patch Set 9 : Enable chrome.mdns for Chrome Apps #Patch Set 10 : Rebase chrome.mdns changes onto master HEAD before submission #Patch Set 11 : Revert additions to chrome.mdns whitelist accidentally reintroduced during rebase #Patch Set 12 : Squash all changes into a single git commit #
Messages
Total messages: 31 (6 generated)
This is my first Chromium change, so I don't exactly know what I'm doing :)
mkwst@chromium.org changed reviewers: + kalman@chromium.org
kalman@ is a better reviewer for this than I.
Let's discuss this on the bug.
mkwst@chromium.org changed reviewers: - mkwst@chromium.org
I don't see anything wrong with the way these are whitelisted. If you want, add some debugging to see what whitelisted extensions are being compared against.
reddaly@chromium.org changed reviewers: + mfoltz@chromium.org
On 2014/10/23 17:37:49, kalman wrote: > I don't see anything wrong with the way these are whitelisted. If you want, add > some debugging to see what whitelisted extensions are being compared against. Thanks for that help. In light of the discussion we had I have changed the code to open up access to the mdns API to all Chrome apps. Yay opening up this API! Before this can be checked in, I think it will need to go through an API review process. We can discuss what needs to be done for approval on the thread for the bug.
https://codereview.chromium.org/668983003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:102: registry->RegisterDnsSdListener(*it); To preserve existing behavior, I think extensions should be limited by the existing whitelist as far as what services they can listen for. We can discuss as part of the API review if this can be relaxed. However, this would require some refactoring as the event filtering is currently done only by service type and not the type of listener (extension vs. platform app). https://codereview.chromium.org/668983003/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/668983003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:739: "F9119B8B18C7C82B51E7BC6FF816B694F2EC3E89" // http://crbug.com/397691 Nit: Keep this space for alignment. https://codereview.chromium.org/668983003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:744: "extension_types": ["platform_app"], // Allow platform apps to access the API. 80 cols.
Responded to Mark's comments. PTAL. https://codereview.chromium.org/668983003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:102: registry->RegisterDnsSdListener(*it); On 2014/10/31 06:29:15, mark a. foltz wrote: > To preserve existing behavior, I think extensions should be limited by the > existing whitelist as far as what services they can listen for. We can discuss > as part of the API review if this can be relaxed. > > However, this would require some refactoring as the event filtering is currently > done only by service type and not the type of listener (extension vs. platform > app). Done. https://codereview.chromium.org/668983003/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/668983003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:739: "F9119B8B18C7C82B51E7BC6FF816B694F2EC3E89" // http://crbug.com/397691 On 2014/10/31 06:29:15, mark a. foltz wrote: > Nit: Keep this space for alignment. Done. https://codereview.chromium.org/668983003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:744: "extension_types": ["platform_app"], // Allow platform apps to access the API. On 2014/10/31 06:29:15, mark a. foltz wrote: > 80 cols. Done.
https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:122: const extensions::ExtensionRegistry* ext_registry = This file is in the extensions namespace, no extensions:: necessary. https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:124: if (!ext_registry) { There should always be an ExtensionRegistry, since the extension system has started (given we're receiving extension function calls). https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:127: const extensions::Extension* extension = ext_registry->GetExtensionById( Why do you want all extensions, rather than just the enabled ones? https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:136: if (IsServiceTypeWhitelisted(*it, extension->is_extension())) I don't understand what the fact that an Extension is has anything to do with whether some service type is whitelisted? https://codereview.chromium.org/668983003/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/668983003/diff/60001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:744: "extension_types": ["platform_app"], // Allow platform apps to access the This line is self explanatory, no need to have a comment. https://codereview.chromium.org/668983003/diff/60001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:746: "whitelist": [] no "whitelist"
https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:122: const extensions::ExtensionRegistry* ext_registry = On 2014/11/17 23:22:15, kalman wrote: > This file is in the extensions namespace, no extensions:: necessary. Done. https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:124: if (!ext_registry) { On 2014/11/17 23:22:15, kalman wrote: > There should always be an ExtensionRegistry, since the extension system has > started (given we're receiving extension function calls). Done. https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:127: const extensions::Extension* extension = ext_registry->GetExtensionById( On 2014/11/17 23:22:15, kalman wrote: > Why do you want all extensions, rather than just the enabled ones? I assumed the extension is enabled if UpdateMDnsListeners is called for it, so ENABLED and EVERYTHING would be equivalent. I've changed to ENABLED here instead of EVERYTHING. https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:136: if (IsServiceTypeWhitelisted(*it, extension->is_extension())) On 2014/11/17 23:22:15, kalman wrote: > I don't understand what the fact that an Extension is has anything to do with > whether some service type is whitelisted? This change is in response to Mark's earlier comment. We would like to preserve the existing service whitelist for extensions but allow apps to discover all types of services. https://codereview.chromium.org/668983003/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/668983003/diff/60001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:744: "extension_types": ["platform_app"], // Allow platform apps to access the On 2014/11/17 23:22:15, kalman wrote: > This line is self explanatory, no need to have a comment. Done. https://codereview.chromium.org/668983003/diff/60001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:746: "whitelist": [] On 2014/11/17 23:22:15, kalman wrote: > no "whitelist" Done.
The extension vs. app behavior should be unit tested if possible (failing that, a browser test can be written). I am okay opening up the service type list for extensions but I'll let Benjamin provide some input on whether the risk is acceptable. (Basically, if there is an exploit on a whitelisted extension then it could be used to find any mDNS advertised device on the user's LAN.) https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:120: service_types_ = new_service_types; If we update |service_types_| here for a disabled extension, a later call may not get all of its service types registered. I think this should go at the end of the method. https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:123: DnsSdRegistry* registry = dns_sd_registry(); This doesn't need to be done until after |extension| is known to be not null. https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:126: GetExtensionById(details.extension_id, ExtensionRegistry::ENABLED); Are only ENABLED extensions allowed to register event listeners? Or should we allow DISABLED extensions/apps to follow this code path? https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:130: Slight preference for declaring a bool here and leaving IsServiceTypeWhitelisted alone: bool all_service_types_allowed = !extension.is_extension() if (all_service_types_allowed || IsServiceTypeWhitelisted(*it)) { ... }
https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:30: // whitelisted set of service types. Note that a more correct check here would be is_platform_app, not !is_extension, because there are things that aren't platform apps (like hosted apps, and V1 apps) that shouldn't be whitelisted (at least, not as part of this change - we can think about that later). https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:31: if (!is_extension) { I actually think that doing all of this work inside UpdateMdnsListeners, then just passing a bool here and checking it once, looks weird. If you: - made IsServiceTypeWhitelisted a member rather than static function, it could grab the ExtensionRegistry itself, find the extension, etc - which would require fewer changes at the call site and make more sense to me. - just do what Mark suggests and don't change this function at all, and put *all* logic inside UpdateMdnsListeners. https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:120: service_types_ = new_service_types; On 2014/12/09 23:56:34, mark a. foltz wrote: > If we update |service_types_| here for a disabled extension, a later call may > not get all of its service types registered. I think this should go at the end > of the method. This shouldn't happen because disabled extensions can't have listened for new service types, they shouldn't be executing code. However, disabling them presumably would clear *all* service types. https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:126: GetExtensionById(details.extension_id, ExtensionRegistry::ENABLED); On 2014/12/09 23:56:34, mark a. foltz wrote: > Are only ENABLED extensions allowed to register event listeners? Or should we > allow DISABLED extensions/apps to follow this code path? Yes I think that EVERYTHING was correct, whoops, because this is called for listener removal as well (I didn't notice that before), which could be called for extensions that were just disabled (or uninstalled, or terminated). Etc. Assuming they actually are in that state when OnListenerRemoved is called - I'm not sure. Can't hurt to check, not that expensive. With that in mind (either the extension might be there, or it might not be), returning early looks wrong to me, if the behaviour is that uninstalling an extension does actually make it disappear from all extension sets: 1. App is installed with service type kSomeServiceType. 2. App is uninstalled. 3. |removed_service_types| will now contain kSomeServiceType. 4. The code never actually gets to the reregistration stage because this is an early-return. 5. |registry| is still watching for kSomeServiceType. Maybe that can't happen. However, returning early here.... just doesn't feel right. I see that Mark is making similar but different comments.
Uploading my changes in response to the comments. I still need to write an accompanying unit test. https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:127: const extensions::Extension* extension = ext_registry->GetExtensionById( On 2014/11/19 21:31:50, Red Daly wrote: > On 2014/11/17 23:22:15, kalman wrote: > > Why do you want all extensions, rather than just the enabled ones? > > I assumed the extension is enabled if UpdateMDnsListeners is called for it, so > ENABLED and EVERYTHING would be equivalent. I've changed to ENABLED here > instead of EVERYTHING. I checked and this assumption was incorrect: This method is called for disabled extensions, too. https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:30: // whitelisted set of service types. On 2014/12/10 18:23:04, kalman wrote: > Note that a more correct check here would be is_platform_app, not !is_extension, > because there are things that aren't platform apps (like hosted apps, and V1 > apps) that shouldn't be whitelisted (at least, not as part of this change - we > can think about that later). Done, but in UpdateMDnsListeners. https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:31: if (!is_extension) { On 2014/12/10 18:23:05, kalman wrote: > I actually think that doing all of this work inside UpdateMdnsListeners, then > just passing a bool here and checking it once, looks weird. If you: > > - made IsServiceTypeWhitelisted a member rather than static function, it could > grab the ExtensionRegistry itself, find the extension, etc - which would require > fewer changes at the call site and make more sense to me. > > - just do what Mark suggests and don't change this function at all, and put > *all* logic inside UpdateMdnsListeners. I took the second approach (kept function static and with original signature). https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:123: DnsSdRegistry* registry = dns_sd_registry(); On 2014/12/09 23:56:34, mark a. foltz wrote: > This doesn't need to be done until after |extension| is known to be not null. The control flow changed a little. At this point, the registry is needed even when extension is null because that is the case for a disabled extension (registry->UnregisterDnsSdListener should be called for every listener of a disabled extension). https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:126: GetExtensionById(details.extension_id, ExtensionRegistry::ENABLED); On 2014/12/10 18:23:04, kalman wrote: > On 2014/12/09 23:56:34, mark a. foltz wrote: > > Are only ENABLED extensions allowed to register event listeners? Or should we > > allow DISABLED extensions/apps to follow this code path? > > Yes I think that EVERYTHING was correct, whoops, because this is called for > listener removal as well (I didn't notice that before), which could be called > for extensions that were just disabled (or uninstalled, or terminated). Etc. > Assuming they actually are in that state when OnListenerRemoved is called - I'm > not sure. Can't hurt to check, not that expensive. > > With that in mind (either the extension might be there, or it might not be), > returning early looks wrong to me, if the behaviour is that uninstalling an > extension does actually make it disappear from all extension sets: > 1. App is installed with service type kSomeServiceType. > 2. App is uninstalled. > 3. |removed_service_types| will now contain kSomeServiceType. > 4. The code never actually gets to the reregistration stage because this is an > early-return. > 5. |registry| is still watching for kSomeServiceType. > > Maybe that can't happen. However, returning early here.... just doesn't feel > right. > > I see that Mark is making similar but different comments. I checked and this is called when an extension is disabled that was listening for mDNS services. I changed the flow to correct the erroneous behavior described above. https://codereview.chromium.org/668983003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:130: On 2014/12/09 23:56:34, mark a. foltz wrote: > Slight preference for declaring a bool here and leaving IsServiceTypeWhitelisted > alone: > > bool all_service_types_allowed = !extension.is_extension() > > if (all_service_types_allowed || IsServiceTypeWhitelisted(*it)) { > ... > } Obsolete (moved IsServiceTypeWhitelisted call to loop above).
lgtm https://codereview.chromium.org/668983003/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:105: GetExtensionById((*it)->extension_id(), ExtensionRegistry::ENABLED); Just do: ... = ExtensionRegistry::Get(...)->enabled_extensions().Get(extension_id()); https://codereview.chromium.org/668983003/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:133: Nits: delete this empty line, don't add an extra one above.
https://chromiumcodereview.appspot.com/668983003/diff/100001/chrome/browser/e... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://chromiumcodereview.appspot.com/668983003/diff/100001/chrome/browser/e... chrome/browser/extensions/api/mdns/mdns_api.cc:105: GetExtensionById((*it)->extension_id(), ExtensionRegistry::ENABLED); On 2015/01/26 22:13:35, kalman wrote: > Just do: > > ... = ExtensionRegistry::Get(...)->enabled_extensions().Get(extension_id()); Done. https://chromiumcodereview.appspot.com/668983003/diff/100001/chrome/browser/e... chrome/browser/extensions/api/mdns/mdns_api.cc:133: On 2015/01/26 22:13:35, kalman wrote: > Nits: delete this empty line, don't add an extra one above. Done.
I am trying to unit test the whitelisting functionality, as suggested by Mark. The code currently depends on the browser_context_ object and static methods, so I'm not sure of the best way to add a unit test. It seems people do not commonly mock BrowserContext... Before I get too far down the road creating mock classes and changing the interface of MDnsAPI, it would be good to hear what a more seasoned Chrome eng would do if he/she were to add a unit test for this class. Any ideas or code pointers? Thanks.
On 2015/01/27 00:39:57, Red Daly wrote: > I am trying to unit test the whitelisting functionality, as suggested by Mark. > The code currently depends on the browser_context_ object and static methods, so > I'm not sure of the best way to add a unit test. It seems people do not > commonly mock BrowserContext... > > Before I get too far down the road creating mock classes and changing the > interface of MDnsAPI, it would be good to hear what a more seasoned Chrome eng > would do if he/she were to add a unit test for this class. Any ideas or code > pointers? > > Thanks. Check out ExtensionsTest: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
lgtm You should be able to mock out just the parts of BrowserContext that you need with GMock. Of course you will need to find a way to inject the mock instance into the API. Perhaps the listener update logic should be refactored into dns_sd_registry without dependencies on API-ish things like BrowserContext. That would make the unit testing cleaner.
On 2015/01/27 00:57:20, mark a. foltz wrote: > lgtm > > You should be able to mock out just the parts of BrowserContext that you need > with GMock. Of course you will need to find a way to inject the mock instance > into the API. > > Perhaps the listener update logic should be refactored into dns_sd_registry > without dependencies on API-ish things like BrowserContext. That would make the > unit testing cleaner. Sorry for the gap in change uplaods. I added a unit test - PTAL.
vitalybuka@chromium.org changed reviewers: + vitalybuka@chromium.org
lgtm https://codereview.chromium.org/668983003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:132: for (std::set<std::string>::iterator it = added_service_types.begin(); maybe: for (const auto& srv: added_service_types) https://codereview.chromium.org/668983003/diff/140001/chrome/common/extension... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/668983003/diff/140001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:728: "channel": "stable", We need mdns for PrinterProvider api users. and user will be mostly extensions. May I ask you to open this for "extension" too? https://codereview.chromium.org/668983003/diff/140001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:731: "63ED55E43214C211F82122ED56407FF1A807F2A3", // Dev 728-744: need two more spacess
lgtm https://codereview.chromium.org/668983003/diff/140001/chrome/common/extension... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/668983003/diff/140001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:728: "channel": "stable", Please ignore this one, we are good with app too. On 2015/03/18 19:07:00, Vitaly Buka wrote: > We need mdns for PrinterProvider api users. and user will be mostly extensions. > > May I ask you to open this for "extension" too? > https://codereview.chromium.org/668983003/diff/140001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:731: "63ED55E43214C211F82122ED56407FF1A807F2A3", // Dev On 2015/03/18 19:07:00, Vitaly Buka wrote: > 728-744: need two more spacess Please ignore this one too.
Responded to all comments. I'll now rebase onto HEAD, upload a new patch set and submit. https://codereview.chromium.org/668983003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/668983003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/mdns_api.cc:132: for (std::set<std::string>::iterator it = added_service_types.begin(); On 2015/03/18 19:07:00, Vitaly Buka wrote: > maybe: for (const auto& srv: added_service_types) Done.
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, vitalybuka@chromium.org Link to the patchset: https://codereview.chromium.org/668983003/#ps220001 (title: "Squash all changes into a single git commit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/668983003/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/68ea490084597d5d4640e782989c0a6a094dcd21 Cr-Commit-Position: refs/heads/master@{#322671} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
