Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2707063002:
Universial component install for chrome os. (Closed)
DescriptionUniversial component install for chrome os.
Adding a component installer for all chrome os components. It is invoked
with a component name and then it looks up this component in a
configuration file (https://chromium.googlesource.com/chromiumos/platform2/+/master/cros_component/cros_component.config). If this file exists, valid and this component is in
the file, then this component got registered. Otherwise, nothing
happens.
Support key per component. Each key is also stored in config file.
format of this key is verified when it is parsed.
unit test includes component_update_service on any platform and config file parsing on chrome os to cover many cases.
BUG=690521, 694731
TEST=cros_component_installer_unittest tests the configuration file
parser on chrome os. Component registeration itself is tested on a chrome device.
Review-Url: https://codereview.chromium.org/2707063002
Cr-Commit-Position: refs/heads/master@{#454650}
Committed: https://chromium.googlesource.com/chromium/src/+/4c9aa5146e5df6682b3fa25e050571c18fd127e6
Patch Set 1 #
Total comments: 12
Patch Set 2 : change configuration file name. #Patch Set 3 : fix to waffles coment #Patch Set 4 : remove TODO. #Patch Set 5 : switch from xml to json config file + update unit test #Patch Set 6 : small fix to last cl #Patch Set 7 : Add an example config file content in comment. #Patch Set 8 : move config file path to a const static variable. #Patch Set 9 : more checks on configuration file #Patch Set 10 : fix a bug #
Total comments: 11
Patch Set 11 : fix according to waffles comment. #Patch Set 12 : rename internal function. #Patch Set 13 : remove comment #Patch Set 14 : add comment for usage in details. #
Total comments: 8
Patch Set 15 : Fix according to waffles' comment #Patch Set 16 : use the blocking thread from ComponentUpdateService instead of creating a new one #Patch Set 17 : remove unnecessary headers #
Total comments: 14
Patch Set 18 : addressing comments from waffles #Patch Set 19 : more fix #Patch Set 20 : remove commented code #Patch Set 21 : more removal #Patch Set 22 : disable dynamic configuration loading #
Total comments: 6
Patch Set 23 : switch from json to std::map for static configuration #
Total comments: 16
Patch Set 24 : address comments from waffles #
Total comments: 21
Patch Set 25 : address comments from sorin #
Total comments: 16
Patch Set 26 : address comments from sorin #Patch Set 27 : build unit_test only on chromeos #
Messages
Total messages: 50 (14 generated)
xiaochu@chromium.org changed reviewers: + adlr@chromium.org, kerrnel@chromium.org, sorin@chromium.org, waffles@chromium.org
I moved this commit to another local cl to modify (need an updated version of chrome). but it creates a new cl instead of patch set. So please review this cl and ignore the old one (https://codereview.chromium.org/2674323002/).
https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.cc:18: 0xe7, 0x08, 0xe6, 0x93, 0x2d, 0x13, 0x5d, 0x8a}; Delete these lines. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.cc:92: #endif // defined(OS_LINUX) OS_LINUX → OS_CHROMEOS https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.cc:103: // TODO: verify installation. What does verifying the installation look like? https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.h:66: // <component name='samba' dir_name='samba'></component> The sha256 hash of the public key must also appear here. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.h:67: // </components> Can we use JSON or protocol buffers instead? XML is a large language.
On 2017/02/21 21:16:04, waffles wrote: > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > File chrome/browser/component_updater/cros_component_installer.cc (right): > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/cros_component_installer.cc:18: 0xe7, 0x08, > 0xe6, 0x93, 0x2d, 0x13, 0x5d, 0x8a}; > Delete these lines. > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/cros_component_installer.cc:92: #endif // > defined(OS_LINUX) > OS_LINUX → OS_CHROMEOS > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/cros_component_installer.cc:103: // TODO: > verify installation. > What does verifying the installation look like? > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > File chrome/browser/component_updater/cros_component_installer.h (right): > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/cros_component_installer.h:66: // <component > name='samba' dir_name='samba'></component> > The sha256 hash of the public key must also appear here. > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/cros_component_installer.h:67: // </components> > Can we use JSON or protocol buffers instead? XML is a large language. Dones.
https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.cc:103: // TODO: verify installation. On 2017/02/21 21:16:03, waffles wrote: > What does verifying the installation look like? Maybe I should be more clear: if there isn't a concrete plan of how this can be implemented, remove the TODO. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.h:67: // </components> On 2017/02/21 21:16:04, waffles wrote: > Can we use JSON or protocol buffers instead? XML is a large language. ...so is that a "no"?
Sorry, didn't reply again. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.cc:18: 0xe7, 0x08, 0xe6, 0x93, 0x2d, 0x13, 0x5d, 0x8a}; On 2017/02/21 21:16:04, waffles wrote: > Delete these lines. Done. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.cc:92: #endif // defined(OS_LINUX) On 2017/02/21 21:16:04, waffles wrote: > OS_LINUX → OS_CHROMEOS Done. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.cc:103: // TODO: verify installation. On 2017/02/21 21:16:03, waffles wrote: > What does verifying the installation look like? For adobe flash, it checks the content of the manifest inside the downloaded image to verify certain properties. escpr's manifest is very simple which only includes name, description and sha2hash which are already verfied along the way of installation. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.h:66: // <component name='samba' dir_name='samba'></component> On 2017/02/21 21:16:04, waffles wrote: > The sha256 hash of the public key must also appear here. Done. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.h:67: // </components> On 2017/02/21 21:16:04, waffles wrote: > Can we use JSON or protocol buffers instead? XML is a large language. I agree. I created another issue here (https://bugs.chromium.org/p/chromium/issues/detail?id=694731) and assigned to me. can we migrate to json by that issue? I can also rewrite the parser here as well if you think it's a good time to switch now..
Sorry, didn't reply again. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.cc:18: 0xe7, 0x08, 0xe6, 0x93, 0x2d, 0x13, 0x5d, 0x8a}; On 2017/02/21 21:16:04, waffles wrote: > Delete these lines. Done. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.cc:103: // TODO: verify installation. On 2017/02/21 21:16:03, waffles wrote: > What does verifying the installation look like? For adobe flash, it checks the content of the manifest inside the downloaded image to verify certain properties. escpr's manifest is very simple which only includes name, description and sha2hash which are already verfied along the way of installation. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.h:66: // <component name='samba' dir_name='samba'></component> On 2017/02/21 21:16:04, waffles wrote: > The sha256 hash of the public key must also appear here. Done. https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.h:67: // </components> On 2017/02/21 21:16:04, waffles wrote: > Can we use JSON or protocol buffers instead? XML is a large language. I agree. I created another issue here (https://bugs.chromium.org/p/chromium/issues/detail?id=694731) and assigned to me. can we migrate to json by that issue? I can also rewrite the parser here as well if you think it's a good time to switch now..
On 2017/02/21 21:39:26, waffles wrote: > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > File chrome/browser/component_updater/cros_component_installer.cc (right): > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/cros_component_installer.cc:103: // TODO: > verify installation. > On 2017/02/21 21:16:03, waffles wrote: > > What does verifying the installation look like? > > Maybe I should be more clear: if there isn't a concrete plan of how this can be > implemented, remove the TODO. > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > File chrome/browser/component_updater/cros_component_installer.h (right): > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/cros_component_installer.h:67: // </components> > On 2017/02/21 21:16:04, waffles wrote: > > Can we use JSON or protocol buffers instead? XML is a large language. > > ...so is that a "no"? sorry, I forgot to send drafts earlier...
https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer.h:67: // </components> On 2017/02/21 21:39:40, xiaochu wrote: > On 2017/02/21 21:16:04, waffles wrote: > > Can we use JSON or protocol buffers instead? XML is a large language. > > I agree. > I created another issue here > (https://bugs.chromium.org/p/chromium/issues/detail?id=694731) and assigned to > me. can we migrate to json by that issue? I can also rewrite the parser here as > well if you think it's a good time to switch now.. I suggest switching now, since there's no reason to check in a parser that we're about to move away from, and the parser makes up a significant portion of this CL.
On 2017/02/21 21:52:13, waffles wrote: > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > File chrome/browser/component_updater/cros_component_installer.h (right): > > https://codereview.chromium.org/2707063002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/cros_component_installer.h:67: // </components> > On 2017/02/21 21:39:40, xiaochu wrote: > > On 2017/02/21 21:16:04, waffles wrote: > > > Can we use JSON or protocol buffers instead? XML is a large language. > > > > I agree. > > I created another issue here > > (https://bugs.chromium.org/p/chromium/issues/detail?id=694731) and assigned to > > me. can we migrate to json by that issue? I can also rewrite the parser here > as > > well if you think it's a good time to switch now.. > > I suggest switching now, since there's no reason to check in a parser that we're > about to move away from, and the parser makes up a significant portion of this > CL. I removed all xml parser code and switched to a simple json wrapper. Code is very easier to review now:)
Description was changed from ========== Universial component install for chrome os. Adding a component installer for all chrome os components. It is invoked with a component name and then it looks up this component in a configuration file. If this file exists, valid and this component is in the file, then this component got registered. Otherwise, nothing happens. Support key per component. Each key is also stored in config file. format of this key is verified when it is parsed. unit test includes configparser and component_update_service to cover many cases. BUG=690521 TEST=cros_component_installer_unittest tests the configuration file parser on any platform. Component register itself is tested on a chrome device. ========== to ========== Universial component install for chrome os. Adding a component installer for all chrome os components. It is invoked with a component name and then it looks up this component in a configuration file. If this file exists, valid and this component is in the file, then this component got registered. Otherwise, nothing happens. Support key per component. Each key is also stored in config file. format of this key is verified when it is parsed. unit test includes configparser and component_update_service to cover many cases. BUG=690521, 694731 TEST=cros_component_installer_unittest tests the configuration file parser on any platform. Component register itself is tested on a chrome device. ==========
Description was changed from ========== Universial component install for chrome os. Adding a component installer for all chrome os components. It is invoked with a component name and then it looks up this component in a configuration file. If this file exists, valid and this component is in the file, then this component got registered. Otherwise, nothing happens. Support key per component. Each key is also stored in config file. format of this key is verified when it is parsed. unit test includes configparser and component_update_service to cover many cases. BUG=690521, 694731 TEST=cros_component_installer_unittest tests the configuration file parser on any platform. Component register itself is tested on a chrome device. ========== to ========== Universial component install for chrome os. Adding a component installer for all chrome os components. It is invoked with a component name and then it looks up this component in a configuration file. If this file exists, valid and this component is in the file, then this component got registered. Otherwise, nothing happens. Support key per component. Each key is also stored in config file. format of this key is verified when it is parsed. unit test includes component_update_service on any platform and config file parsing on chrome os to cover many cases. BUG=690521, 694731 TEST=cros_component_installer_unittest tests the configuration file parser on chrome os. Component registeration itself is tested on a chrome device. ==========
https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:136: // [ If you always are using the name as a key, do you want to make this an object rather than an array? e.g. "components": { "escpr": { "dir": "epson-inkjet-printer-escpr" "sha2hashstr":"1913a5e0a6cad30b6f03..." } } https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:143: // } Adding indentation could make this clearer. https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:145: base::FilePath configFilePath(filepathstr); Can we remember the contents of this file instead of repeatedly reading it? This may require you to make a factory object. https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:146: JSONFileValueDeserializer deserializer(configFilePath); What thread do you intend this function to be run on? Any file I/O should be done using the TaskScheduler (not on the UI thread), but installer->Register must be called on the UI thread. https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:150: VLOG(0) << "[RegisterCrOSComponents] configuration not exist or in wrong " Can we use DVLOG(1)? Also elsewhere in this file, for every VLOG or LOG.
Dones. https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:136: // [ On 2017/02/23 17:13:20, waffles wrote: > If you always are using the name as a key, do you want to make this an object > rather than an array? > > e.g. > > "components": { > "escpr": { > "dir": "epson-inkjet-printer-escpr" > "sha2hashstr":"1913a5e0a6cad30b6f03..." > } > } Done. https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:143: // } On 2017/02/23 17:13:20, waffles wrote: > Adding indentation could make this clearer. Done. https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:145: base::FilePath configFilePath(filepathstr); On 2017/02/23 17:13:20, waffles wrote: > Can we remember the contents of this file instead of repeatedly reading it? This > may require you to make a factory object. Done. https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:146: JSONFileValueDeserializer deserializer(configFilePath); On 2017/02/23 17:13:20, waffles wrote: > What thread do you intend this function to be run on? Any file I/O should be > done using the TaskScheduler (not on the UI thread), but installer->Register > must be called on the UI thread. I added some more options for developers: 1) UI threads can call RegisterCrOSCompnent, if config is not loaded yet, it won't register. 2) RegisterCrOSComponentIO will call LoadCrOSConfig and then RegisterCrOSComponent. So we can either call this or schedule LoadCrOSConfig somewhere at startup. And we won't do incorrect things here by calling either of them. Please advise. https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:150: VLOG(0) << "[RegisterCrOSComponents] configuration not exist or in wrong " On 2017/02/23 17:13:20, waffles wrote: > Can we use DVLOG(1)? Also elsewhere in this file, for every VLOG or LOG. Done.
Description was changed from ========== Universial component install for chrome os. Adding a component installer for all chrome os components. It is invoked with a component name and then it looks up this component in a configuration file. If this file exists, valid and this component is in the file, then this component got registered. Otherwise, nothing happens. Support key per component. Each key is also stored in config file. format of this key is verified when it is parsed. unit test includes component_update_service on any platform and config file parsing on chrome os to cover many cases. BUG=690521, 694731 TEST=cros_component_installer_unittest tests the configuration file parser on chrome os. Component registeration itself is tested on a chrome device. ========== to ========== Universial component install for chrome os. Adding a component installer for all chrome os components. It is invoked with a component name and then it looks up this component in a configuration file (https://chromium.googlesource.com/chromiumos/platform2/+/master/cros_componen...). If this file exists, valid and this component is in the file, then this component got registered. Otherwise, nothing happens. Support key per component. Each key is also stored in config file. format of this key is verified when it is parsed. unit test includes component_update_service on any platform and config file parsing on chrome os to cover many cases. BUG=690521, 694731 TEST=cros_component_installer_unittest tests the configuration file parser on chrome os. Component registeration itself is tested on a chrome device. ==========
https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/180001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:146: JSONFileValueDeserializer deserializer(configFilePath); On 2017/02/23 20:54:56, xiaochu wrote: > On 2017/02/23 17:13:20, waffles wrote: > > What thread do you intend this function to be run on? Any file I/O should be > > done using the TaskScheduler (not on the UI thread), but installer->Register > > must be called on the UI thread. > > I added some more options for developers: > > 1) UI threads can call RegisterCrOSCompnent, if config is not loaded yet, it > won't register. > > 2) RegisterCrOSComponentIO will call LoadCrOSConfig and then > RegisterCrOSComponent. So we can either call this or schedule LoadCrOSConfig > somewhere at startup. > > And we won't do incorrect things here by calling either of them. Please advise. I just documented my idea in .h file in details. Basically, we allow developers to do an asynchronous call from a taskrunner (they owned) to do registration.
https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:129: installer->Register(cus, base::Closure()); Since installer->Register must be called on the UI thread (it will fail a DCHECK if not), RegisterCrOSComponentInternal must be called on the UI thread. This means that the implementation of RegisterCrOSComponentIO isn't correct. If you have to both read the file and register the component, a synchronous implementation is not possible. https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:99: ComponentConfig& config); Is it necessary to declare this in the header? I suggest moving it into an anonymous namespace in the .cc file. https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:112: // "component_name_to_update"); Could this logic be moved into RegisterCrOSComponent itself, rather than making the callers responsible for triggering LoadCrOSConfig and worrying about the threading issues? https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:116: // comopnentConfigReaderRoot; load only when call the first time, first call when call → when called
dones. https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:129: installer->Register(cus, base::Closure()); On 2017/02/28 17:37:21, waffles wrote: > Since installer->Register must be called on the UI thread (it will fail a DCHECK > if not), RegisterCrOSComponentInternal must be called on the UI thread. This > means that the implementation of RegisterCrOSComponentIO isn't correct. > > If you have to both read the file and register the component, a synchronous > implementation is not possible. I changed to allow only synchronous registration (consistent with other component registration) from UI thread. Different from other registration, CrOS component registration is not done at browser launch (an asynchronous LoadCrOSConfig is called at browser launch instead). Then at runtime after browser launch (hopefully LoadCrOSConfig does its job already), RegisterCrOSComponent can be called in UI thread. Is this reasonable? https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:99: ComponentConfig& config); On 2017/02/28 17:37:21, waffles wrote: > Is it necessary to declare this in the header? I suggest moving it into an > anonymous namespace in the .cc file. Done. https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:112: // "component_name_to_update"); On 2017/02/28 17:37:21, waffles wrote: > Could this logic be moved into RegisterCrOSComponent itself, rather than making > the callers responsible for triggering LoadCrOSConfig and worrying about the > threading issues? Done. https://codereview.chromium.org/2707063002/diff/260001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:116: // comopnentConfigReaderRoot; load only when call the first time, first call On 2017/02/28 17:37:21, waffles wrote: > when call → when called replaced.
https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:173: "component " Why not combine with the above line? https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:183: componentConfigReaderRoot = deserializer.Deserialize(NULL, &error); Consider DCHECKing that error.empty(). If you don't want to do that, you can pass NULL for error. https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:188: base::Bind(&LoadCrOSConfigInternal)); I recommend instead doing: base::PostTaskWithTraits( FROM_HERE, base::TaskTraits().MayBlock().WithPriority( base::TaskPriority::BACKGROUND), base::Bind(&LoadCrOSConfigInternal)); and removing the dependency on ComponentUpdaterService, unless you are trying to guarantee that this task happens in a certain sequence with certain other ComponentUpdateService tasks (which I doubt is useful). https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. no (c) after Copyright, and use 2017. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:50: #include "chrome/common/component_flash_hint_file_linux.h" Do you really need this? I'm also unsure about many of the other headers - can we remove some, or move the includes into the cc file? https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:51: #endif // defined(OS_CHROMEOS) Do you want to consider having the entire CrOSComponentInstallerTraits object conditionally compiled? https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:57: static std::unique_ptr<base::Value> componentConfigReaderRoot = nullptr; Not sure this is allowed. See https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables In any case I don't think you need the = nullptr.
Config is constructed as a factory class. Code is tested on Chromebook and working correctly. https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:173: "component " On 2017/03/01 18:49:20, waffles wrote: > Why not combine with the above line? Done. https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:183: componentConfigReaderRoot = deserializer.Deserialize(NULL, &error); On 2017/03/01 18:49:20, waffles wrote: > Consider DCHECKing that error.empty(). > If you don't want to do that, you can pass NULL for error. Done. https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:188: base::Bind(&LoadCrOSConfigInternal)); On 2017/03/01 18:49:20, waffles wrote: > I recommend instead doing: > > base::PostTaskWithTraits( > FROM_HERE, > base::TaskTraits().MayBlock().WithPriority( > base::TaskPriority::BACKGROUND), > base::Bind(&LoadCrOSConfigInternal)); > > and removing the dependency on ComponentUpdaterService, unless you are trying to > guarantee that this task happens in a certain sequence with certain other > ComponentUpdateService tasks (which I doubt is useful). Done. https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/03/01 18:49:20, waffles wrote: > no (c) after Copyright, and use 2017. See > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:50: #include "chrome/common/component_flash_hint_file_linux.h" On 2017/03/01 18:49:20, waffles wrote: > Do you really need this? I'm also unsure about many of the other headers - can > we remove some, or move the includes into the cc file? Done. https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:51: #endif // defined(OS_CHROMEOS) On 2017/03/01 18:49:20, waffles wrote: > Do you want to consider having the entire CrOSComponentInstallerTraits object > conditionally compiled? Done. https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:57: static std::unique_ptr<base::Value> componentConfigReaderRoot = nullptr; On 2017/03/01 18:49:20, waffles wrote: > Not sure this is allowed. See > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > In any case I don't think you need the = nullptr. Done.
On 2017/03/02 01:50:06, xiaochu wrote: > Config is constructed as a factory class. Code is tested on Chromebook and > working correctly. > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > File chrome/browser/component_updater/cros_component_installer.cc (right): > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > chrome/browser/component_updater/cros_component_installer.cc:173: "component " > On 2017/03/01 18:49:20, waffles wrote: > > Why not combine with the above line? > > Done. > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > chrome/browser/component_updater/cros_component_installer.cc:183: > componentConfigReaderRoot = deserializer.Deserialize(NULL, &error); > On 2017/03/01 18:49:20, waffles wrote: > > Consider DCHECKing that error.empty(). > > If you don't want to do that, you can pass NULL for error. > > Done. > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > chrome/browser/component_updater/cros_component_installer.cc:188: > base::Bind(&LoadCrOSConfigInternal)); > On 2017/03/01 18:49:20, waffles wrote: > > I recommend instead doing: > > > > base::PostTaskWithTraits( > > FROM_HERE, > > base::TaskTraits().MayBlock().WithPriority( > > base::TaskPriority::BACKGROUND), > > base::Bind(&LoadCrOSConfigInternal)); > > > > and removing the dependency on ComponentUpdaterService, unless you are trying > to > > guarantee that this task happens in a certain sequence with certain other > > ComponentUpdateService tasks (which I doubt is useful). > > Done. > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > File chrome/browser/component_updater/cros_component_installer.h (right): > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > chrome/browser/component_updater/cros_component_installer.h:1: // Copyright (c) > 2012 The Chromium Authors. All rights reserved. > On 2017/03/01 18:49:20, waffles wrote: > > no (c) after Copyright, and use 2017. See > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Done. > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > chrome/browser/component_updater/cros_component_installer.h:50: #include > "chrome/common/component_flash_hint_file_linux.h" > On 2017/03/01 18:49:20, waffles wrote: > > Do you really need this? I'm also unsure about many of the other headers - can > > we remove some, or move the includes into the cc file? > > Done. > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > chrome/browser/component_updater/cros_component_installer.h:51: #endif // > defined(OS_CHROMEOS) > On 2017/03/01 18:49:20, waffles wrote: > > Do you want to consider having the entire CrOSComponentInstallerTraits object > > conditionally compiled? > > Done. > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > chrome/browser/component_updater/cros_component_installer.h:57: static > std::unique_ptr<base::Value> componentConfigReaderRoot = nullptr; > On 2017/03/01 18:49:20, waffles wrote: > > Not sure this is allowed. See > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > > > In any case I don't think you need the = nullptr. > > Done. I just realized the factory class is still static... What we want is to maintain a pointer pointing to a loaded base::Value in memory. However, I don't think there is a solution without creating something static and global since we only have functions which don't carry static data across functions. Maybe there is some build-in global object in Chrome that we can use to store that pointer? Please advise.
On 2017/03/02 02:14:31, xiaochu wrote: > On 2017/03/02 01:50:06, xiaochu wrote: > > Config is constructed as a factory class. Code is tested on Chromebook and > > working correctly. > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > File chrome/browser/component_updater/cros_component_installer.cc (right): > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > chrome/browser/component_updater/cros_component_installer.cc:173: "component " > > On 2017/03/01 18:49:20, waffles wrote: > > > Why not combine with the above line? > > > > Done. > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > chrome/browser/component_updater/cros_component_installer.cc:183: > > componentConfigReaderRoot = deserializer.Deserialize(NULL, &error); > > On 2017/03/01 18:49:20, waffles wrote: > > > Consider DCHECKing that error.empty(). > > > If you don't want to do that, you can pass NULL for error. > > > > Done. > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > chrome/browser/component_updater/cros_component_installer.cc:188: > > base::Bind(&LoadCrOSConfigInternal)); > > On 2017/03/01 18:49:20, waffles wrote: > > > I recommend instead doing: > > > > > > base::PostTaskWithTraits( > > > FROM_HERE, > > > base::TaskTraits().MayBlock().WithPriority( > > > base::TaskPriority::BACKGROUND), > > > base::Bind(&LoadCrOSConfigInternal)); > > > > > > and removing the dependency on ComponentUpdaterService, unless you are > trying > > to > > > guarantee that this task happens in a certain sequence with certain other > > > ComponentUpdateService tasks (which I doubt is useful). > > > > Done. > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > File chrome/browser/component_updater/cros_component_installer.h (right): > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > chrome/browser/component_updater/cros_component_installer.h:1: // Copyright > (c) > > 2012 The Chromium Authors. All rights reserved. > > On 2017/03/01 18:49:20, waffles wrote: > > > no (c) after Copyright, and use 2017. See > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > Done. > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > chrome/browser/component_updater/cros_component_installer.h:50: #include > > "chrome/common/component_flash_hint_file_linux.h" > > On 2017/03/01 18:49:20, waffles wrote: > > > Do you really need this? I'm also unsure about many of the other headers - > can > > > we remove some, or move the includes into the cc file? > > > > Done. > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > chrome/browser/component_updater/cros_component_installer.h:51: #endif // > > defined(OS_CHROMEOS) > > On 2017/03/01 18:49:20, waffles wrote: > > > Do you want to consider having the entire CrOSComponentInstallerTraits > object > > > conditionally compiled? > > > > Done. > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > chrome/browser/component_updater/cros_component_installer.h:57: static > > std::unique_ptr<base::Value> componentConfigReaderRoot = nullptr; > > On 2017/03/01 18:49:20, waffles wrote: > > > Not sure this is allowed. See > > > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > > > > > In any case I don't think you need the = nullptr. > > > > Done. > > I just realized the factory class is still static... > What we want is to maintain a pointer pointing to a loaded base::Value in > memory. However, I don't think there is a solution without creating something > static and global since we only have functions which don't carry static data > across functions. > Maybe there is some build-in global object in Chrome that we can use to store > that pointer? Please advise. You can try hang something off of g_browser_process; this is e.g. how the lifetime of the component updater itself is handled: https://cs.chromium.org/chromium/src/chrome/browser/browser_process.h?l=258 At the very least, if that's not the right approach, the browser process owners can probably suggest a better one. I think in the past people have tried more complex tricks but I don't see any surviving in the code we own today.
On 2017/03/02 16:27:19, waffles wrote: > On 2017/03/02 02:14:31, xiaochu wrote: > > On 2017/03/02 01:50:06, xiaochu wrote: > > > Config is constructed as a factory class. Code is tested on Chromebook and > > > working correctly. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > File chrome/browser/component_updater/cros_component_installer.cc (right): > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.cc:173: "component > " > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Why not combine with the above line? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.cc:183: > > > componentConfigReaderRoot = deserializer.Deserialize(NULL, &error); > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Consider DCHECKing that error.empty(). > > > > If you don't want to do that, you can pass NULL for error. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.cc:188: > > > base::Bind(&LoadCrOSConfigInternal)); > > > On 2017/03/01 18:49:20, waffles wrote: > > > > I recommend instead doing: > > > > > > > > base::PostTaskWithTraits( > > > > FROM_HERE, > > > > base::TaskTraits().MayBlock().WithPriority( > > > > base::TaskPriority::BACKGROUND), > > > > base::Bind(&LoadCrOSConfigInternal)); > > > > > > > > and removing the dependency on ComponentUpdaterService, unless you are > > trying > > > to > > > > guarantee that this task happens in a certain sequence with certain other > > > > ComponentUpdateService tasks (which I doubt is useful). > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > File chrome/browser/component_updater/cros_component_installer.h (right): > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.h:1: // Copyright > > (c) > > > 2012 The Chromium Authors. All rights reserved. > > > On 2017/03/01 18:49:20, waffles wrote: > > > > no (c) after Copyright, and use 2017. See > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.h:50: #include > > > "chrome/common/component_flash_hint_file_linux.h" > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Do you really need this? I'm also unsure about many of the other headers - > > can > > > > we remove some, or move the includes into the cc file? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.h:51: #endif // > > > defined(OS_CHROMEOS) > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Do you want to consider having the entire CrOSComponentInstallerTraits > > object > > > > conditionally compiled? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.h:57: static > > > std::unique_ptr<base::Value> componentConfigReaderRoot = nullptr; > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Not sure this is allowed. See > > > > > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > > > > > > > In any case I don't think you need the = nullptr. > > > > > > Done. > > > > I just realized the factory class is still static... > > What we want is to maintain a pointer pointing to a loaded base::Value in > > memory. However, I don't think there is a solution without creating something > > static and global since we only have functions which don't carry static data > > across functions. > > Maybe there is some build-in global object in Chrome that we can use to store > > that pointer? Please advise. > > You can try hang something off of g_browser_process; this is e.g. how the > lifetime of the component updater itself is handled: > https://cs.chromium.org/chromium/src/chrome/browser/browser_process.h?l=258 At > the very least, if that's not the right approach, the browser process owners can > probably suggest a better one. I think in the past people have tried more > complex tricks but I don't see any surviving in the code we own today.
On 2017/03/02 16:27:19, waffles wrote: > On 2017/03/02 02:14:31, xiaochu wrote: > > On 2017/03/02 01:50:06, xiaochu wrote: > > > Config is constructed as a factory class. Code is tested on Chromebook and > > > working correctly. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > File chrome/browser/component_updater/cros_component_installer.cc (right): > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.cc:173: "component > " > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Why not combine with the above line? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.cc:183: > > > componentConfigReaderRoot = deserializer.Deserialize(NULL, &error); > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Consider DCHECKing that error.empty(). > > > > If you don't want to do that, you can pass NULL for error. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.cc:188: > > > base::Bind(&LoadCrOSConfigInternal)); > > > On 2017/03/01 18:49:20, waffles wrote: > > > > I recommend instead doing: > > > > > > > > base::PostTaskWithTraits( > > > > FROM_HERE, > > > > base::TaskTraits().MayBlock().WithPriority( > > > > base::TaskPriority::BACKGROUND), > > > > base::Bind(&LoadCrOSConfigInternal)); > > > > > > > > and removing the dependency on ComponentUpdaterService, unless you are > > trying > > > to > > > > guarantee that this task happens in a certain sequence with certain other > > > > ComponentUpdateService tasks (which I doubt is useful). > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > File chrome/browser/component_updater/cros_component_installer.h (right): > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.h:1: // Copyright > > (c) > > > 2012 The Chromium Authors. All rights reserved. > > > On 2017/03/01 18:49:20, waffles wrote: > > > > no (c) after Copyright, and use 2017. See > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.h:50: #include > > > "chrome/common/component_flash_hint_file_linux.h" > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Do you really need this? I'm also unsure about many of the other headers - > > can > > > > we remove some, or move the includes into the cc file? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.h:51: #endif // > > > defined(OS_CHROMEOS) > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Do you want to consider having the entire CrOSComponentInstallerTraits > > object > > > > conditionally compiled? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > chrome/browser/component_updater/cros_component_installer.h:57: static > > > std::unique_ptr<base::Value> componentConfigReaderRoot = nullptr; > > > On 2017/03/01 18:49:20, waffles wrote: > > > > Not sure this is allowed. See > > > > > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > > > > > > > In any case I don't think you need the = nullptr. > > > > > > Done. > > > > I just realized the factory class is still static... > > What we want is to maintain a pointer pointing to a loaded base::Value in > > memory. However, I don't think there is a solution without creating something > > static and global since we only have functions which don't carry static data > > across functions. > > Maybe there is some build-in global object in Chrome that we can use to store > > that pointer? Please advise. > > You can try hang something off of g_browser_process; this is e.g. how the > lifetime of the component updater itself is handled: > https://cs.chromium.org/chromium/src/chrome/browser/browser_process.h?l=258 At > the very least, if that's not the right approach, the browser process owners can > probably suggest a better one. I think in the past people have tried more > complex tricks but I don't see any surviving in the code we own today. thanks for your suggestion! i guess Component Updater probably needs this info at some point since it maintains all the runtime info of chrome component. is it a good idea to pass that pointer to the update service?
On 2017/03/02 16:43:32, xiaochu wrote: > On 2017/03/02 16:27:19, waffles wrote: > > On 2017/03/02 02:14:31, xiaochu wrote: > > > On 2017/03/02 01:50:06, xiaochu wrote: > > > > Config is constructed as a factory class. Code is tested on Chromebook and > > > > working correctly. > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > File chrome/browser/component_updater/cros_component_installer.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > chrome/browser/component_updater/cros_component_installer.cc:173: > "component > > " > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > Why not combine with the above line? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > chrome/browser/component_updater/cros_component_installer.cc:183: > > > > componentConfigReaderRoot = deserializer.Deserialize(NULL, &error); > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > Consider DCHECKing that error.empty(). > > > > > If you don't want to do that, you can pass NULL for error. > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > chrome/browser/component_updater/cros_component_installer.cc:188: > > > > base::Bind(&LoadCrOSConfigInternal)); > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > I recommend instead doing: > > > > > > > > > > base::PostTaskWithTraits( > > > > > FROM_HERE, > > > > > base::TaskTraits().MayBlock().WithPriority( > > > > > base::TaskPriority::BACKGROUND), > > > > > base::Bind(&LoadCrOSConfigInternal)); > > > > > > > > > > and removing the dependency on ComponentUpdaterService, unless you are > > > trying > > > > to > > > > > guarantee that this task happens in a certain sequence with certain > other > > > > > ComponentUpdateService tasks (which I doubt is useful). > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > File chrome/browser/component_updater/cros_component_installer.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > chrome/browser/component_updater/cros_component_installer.h:1: // > Copyright > > > (c) > > > > 2012 The Chromium Authors. All rights reserved. > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > no (c) after Copyright, and use 2017. See > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > chrome/browser/component_updater/cros_component_installer.h:50: #include > > > > "chrome/common/component_flash_hint_file_linux.h" > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > Do you really need this? I'm also unsure about many of the other headers > - > > > can > > > > > we remove some, or move the includes into the cc file? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > chrome/browser/component_updater/cros_component_installer.h:51: #endif // > > > > defined(OS_CHROMEOS) > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > Do you want to consider having the entire CrOSComponentInstallerTraits > > > object > > > > > conditionally compiled? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > chrome/browser/component_updater/cros_component_installer.h:57: static > > > > std::unique_ptr<base::Value> componentConfigReaderRoot = nullptr; > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > Not sure this is allowed. See > > > > > > > > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > > > > > > > > > In any case I don't think you need the = nullptr. > > > > > > > > Done. > > > > > > I just realized the factory class is still static... > > > What we want is to maintain a pointer pointing to a loaded base::Value in > > > memory. However, I don't think there is a solution without creating > something > > > static and global since we only have functions which don't carry static data > > > across functions. > > > Maybe there is some build-in global object in Chrome that we can use to > store > > > that pointer? Please advise. > > > > You can try hang something off of g_browser_process; this is e.g. how the > > lifetime of the component updater itself is handled: > > https://cs.chromium.org/chromium/src/chrome/browser/browser_process.h?l=258 At > > the very least, if that's not the right approach, the browser process owners > can > > probably suggest a better one. I think in the past people have tried more > > complex tricks but I don't see any surviving in the code we own today. > > thanks for your suggestion! i guess Component Updater probably needs this info > at some point since it maintains all the runtime info of chrome component. is it > a good idea to pass that pointer to the update service? No, the API of the update service is cross-platform and I don't think we want to add a function to expose this. In other words, this could reasonably be owned by a component installer or installer factory, but not the updater itself (which stands alone from any installers).
On 2017/03/02 16:54:33, waffles wrote: > On 2017/03/02 16:43:32, xiaochu wrote: > > On 2017/03/02 16:27:19, waffles wrote: > > > On 2017/03/02 02:14:31, xiaochu wrote: > > > > On 2017/03/02 01:50:06, xiaochu wrote: > > > > > Config is constructed as a factory class. Code is tested on Chromebook > and > > > > > working correctly. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > File chrome/browser/component_updater/cros_component_installer.cc > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > chrome/browser/component_updater/cros_component_installer.cc:173: > > "component > > > " > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > Why not combine with the above line? > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > chrome/browser/component_updater/cros_component_installer.cc:183: > > > > > componentConfigReaderRoot = deserializer.Deserialize(NULL, &error); > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > Consider DCHECKing that error.empty(). > > > > > > If you don't want to do that, you can pass NULL for error. > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > chrome/browser/component_updater/cros_component_installer.cc:188: > > > > > base::Bind(&LoadCrOSConfigInternal)); > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > I recommend instead doing: > > > > > > > > > > > > base::PostTaskWithTraits( > > > > > > FROM_HERE, > > > > > > base::TaskTraits().MayBlock().WithPriority( > > > > > > base::TaskPriority::BACKGROUND), > > > > > > base::Bind(&LoadCrOSConfigInternal)); > > > > > > > > > > > > and removing the dependency on ComponentUpdaterService, unless you are > > > > trying > > > > > to > > > > > > guarantee that this task happens in a certain sequence with certain > > other > > > > > > ComponentUpdateService tasks (which I doubt is useful). > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > File chrome/browser/component_updater/cros_component_installer.h > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > chrome/browser/component_updater/cros_component_installer.h:1: // > > Copyright > > > > (c) > > > > > 2012 The Chromium Authors. All rights reserved. > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > no (c) after Copyright, and use 2017. See > > > > > > > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > chrome/browser/component_updater/cros_component_installer.h:50: #include > > > > > "chrome/common/component_flash_hint_file_linux.h" > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > Do you really need this? I'm also unsure about many of the other > headers > > - > > > > can > > > > > > we remove some, or move the includes into the cc file? > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > chrome/browser/component_updater/cros_component_installer.h:51: #endif > // > > > > > defined(OS_CHROMEOS) > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > Do you want to consider having the entire CrOSComponentInstallerTraits > > > > object > > > > > > conditionally compiled? > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > chrome/browser/component_updater/cros_component_installer.h:57: static > > > > > std::unique_ptr<base::Value> componentConfigReaderRoot = nullptr; > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > Not sure this is allowed. See > > > > > > > > > > > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > > > > > > > > > > > In any case I don't think you need the = nullptr. > > > > > > > > > > Done. > > > > > > > > I just realized the factory class is still static... > > > > What we want is to maintain a pointer pointing to a loaded base::Value in > > > > memory. However, I don't think there is a solution without creating > > something > > > > static and global since we only have functions which don't carry static > data > > > > across functions. > > > > Maybe there is some build-in global object in Chrome that we can use to > > store > > > > that pointer? Please advise. > > > > > > You can try hang something off of g_browser_process; this is e.g. how the > > > lifetime of the component updater itself is handled: > > > https://cs.chromium.org/chromium/src/chrome/browser/browser_process.h?l=258 > At > > > the very least, if that's not the right approach, the browser process owners > > can > > > probably suggest a better one. I think in the past people have tried more > > > complex tricks but I don't see any surviving in the code we own today. > > > > thanks for your suggestion! i guess Component Updater probably needs this info > > at some point since it maintains all the runtime info of chrome component. is > it > > a good idea to pass that pointer to the update service? > > No, the API of the update service is cross-platform and I don't think we want to > add a function to expose this. In other words, this could reasonably be owned by > a component installer or installer factory, but not the updater itself (which > stands alone from any installers). I see. Can you point to me the owner for that browser_process? I can't find from the owner file: https://cs.chromium.org/chromium/src/chrome/browser/OWNERS Also, it seems that loading a component became the major roadblock for this CL. Can I instead the configuration static and in the meanwhile asking help from browser_process guys?
On 2017/03/02 17:04:49, xiaochu wrote: > On 2017/03/02 16:54:33, waffles wrote: > > On 2017/03/02 16:43:32, xiaochu wrote: > > > On 2017/03/02 16:27:19, waffles wrote: > > > > On 2017/03/02 02:14:31, xiaochu wrote: > > > > > On 2017/03/02 01:50:06, xiaochu wrote: > > > > > > Config is constructed as a factory class. Code is tested on Chromebook > > and > > > > > > working correctly. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > > File chrome/browser/component_updater/cros_component_installer.cc > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > > chrome/browser/component_updater/cros_component_installer.cc:173: > > > "component > > > > " > > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > > Why not combine with the above line? > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > > chrome/browser/component_updater/cros_component_installer.cc:183: > > > > > > componentConfigReaderRoot = deserializer.Deserialize(NULL, &error); > > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > > Consider DCHECKing that error.empty(). > > > > > > > If you don't want to do that, you can pass NULL for error. > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > > chrome/browser/component_updater/cros_component_installer.cc:188: > > > > > > base::Bind(&LoadCrOSConfigInternal)); > > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > > I recommend instead doing: > > > > > > > > > > > > > > base::PostTaskWithTraits( > > > > > > > FROM_HERE, > > > > > > > base::TaskTraits().MayBlock().WithPriority( > > > > > > > base::TaskPriority::BACKGROUND), > > > > > > > base::Bind(&LoadCrOSConfigInternal)); > > > > > > > > > > > > > > and removing the dependency on ComponentUpdaterService, unless you > are > > > > > trying > > > > > > to > > > > > > > guarantee that this task happens in a certain sequence with certain > > > other > > > > > > > ComponentUpdateService tasks (which I doubt is useful). > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > > File chrome/browser/component_updater/cros_component_installer.h > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > > chrome/browser/component_updater/cros_component_installer.h:1: // > > > Copyright > > > > > (c) > > > > > > 2012 The Chromium Authors. All rights reserved. > > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > > no (c) after Copyright, and use 2017. See > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > > chrome/browser/component_updater/cros_component_installer.h:50: > #include > > > > > > "chrome/common/component_flash_hint_file_linux.h" > > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > > Do you really need this? I'm also unsure about many of the other > > headers > > > - > > > > > can > > > > > > > we remove some, or move the includes into the cc file? > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > > chrome/browser/component_updater/cros_component_installer.h:51: #endif > > > // > > > > > > defined(OS_CHROMEOS) > > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > > Do you want to consider having the entire > CrOSComponentInstallerTraits > > > > > object > > > > > > > conditionally compiled? > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2707063002/diff/320001/chrome/browser/compone... > > > > > > chrome/browser/component_updater/cros_component_installer.h:57: static > > > > > > std::unique_ptr<base::Value> componentConfigReaderRoot = nullptr; > > > > > > On 2017/03/01 18:49:20, waffles wrote: > > > > > > > Not sure this is allowed. See > > > > > > > > > > > > > > > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > > > > > > > > > > > > > In any case I don't think you need the = nullptr. > > > > > > > > > > > > Done. > > > > > > > > > > I just realized the factory class is still static... > > > > > What we want is to maintain a pointer pointing to a loaded base::Value > in > > > > > memory. However, I don't think there is a solution without creating > > > something > > > > > static and global since we only have functions which don't carry static > > data > > > > > across functions. > > > > > Maybe there is some build-in global object in Chrome that we can use to > > > store > > > > > that pointer? Please advise. > > > > > > > > You can try hang something off of g_browser_process; this is e.g. how the > > > > lifetime of the component updater itself is handled: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/browser_process.h?l=258 > > At > > > > the very least, if that's not the right approach, the browser process > owners > > > can > > > > probably suggest a better one. I think in the past people have tried more > > > > complex tricks but I don't see any surviving in the code we own today. > > > > > > thanks for your suggestion! i guess Component Updater probably needs this > info > > > at some point since it maintains all the runtime info of chrome component. > is > > it > > > a good idea to pass that pointer to the update service? > > > > No, the API of the update service is cross-platform and I don't think we want > to > > add a function to expose this. In other words, this could reasonably be owned > by > > a component installer or installer factory, but not the updater itself (which > > stands alone from any installers). > > I see. Can you point to me the owner for that browser_process? I can't find from > the owner file: https://cs.chromium.org/chromium/src/chrome/browser/OWNERS > Also, it seems that loading a component became the major roadblock for this CL. > Can I instead the configuration static and in the meanwhile asking help from > browser_process guys? You'll want one of jochen@chromium.org sky@chromium.org thakis@chromium.org thestig@chromium.org (from src/chrome/OWNERS). They are all cool people, lets see what they suggest.
thanks! I temporarily disabled configuration loading while I am waiting for response from owners. I think configuration is for purpose of developer experience instead of functionality. Can we move on this CL with static config while leaving it for another issue? https://bugs.chromium.org/p/chromium/issues/detail?id=697957
thanks! I temporarily disabled configuration loading while I am waiting for response from owners. I think configuration is for purpose of developer experience instead of functionality. Can we move on this CL with static config while leaving it for another issue? https://bugs.chromium.org/p/chromium/issues/detail?id=697957
https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:164: "}"; If we are going this way, just use a std::map and remove the JSON pieces. (Can always put them back.) https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:214: void LoadCrOSConfig() { Just remove it, if unused. https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:59: // It also assumes LoadCrOSConfig has finished loading configuration. Otherwise, Comment is now out of date.
Thanks! All done and tested. https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:164: "}"; On 2017/03/02 18:52:39, waffles wrote: > If we are going this way, just use a std::map and remove the JSON pieces. (Can > always put them back.) Done. https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:214: void LoadCrOSConfig() { On 2017/03/02 18:52:39, waffles wrote: > Just remove it, if unused. Done. https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/420001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:59: // It also assumes LoadCrOSConfig has finished loading configuration. Otherwise, On 2017/03/02 18:52:39, waffles wrote: > Comment is now out of date. Done.
Thanks! This is getting close, just a few minor issues left from me. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:7: #include "base/task_scheduler/task_traits.h" Do you need the two above headers? https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:12: #include "content/public/browser/plugin_service.h" Can we remove the dependency on plugin_service.h? https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:21: using content::PluginService; remove this line https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:34: LOG(ERROR) << "Component registration failed"; Can this be DVLOG(1)? Also on lines 30 and 49. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:54: bool SkipCupsRegistration(ComponentUpdateService* cus) { I'm not sure what the purpose of this function is. Can it be removed? https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:76: return false; We should think about this carefully. Since the intent is not to ship these components to everyone, can a MITM infer information (e.g. fingerprint the user) based on which set of components they have installed? Can a MITM infer information about how the user is using the browser? It might be best to set this to true for now. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:106: return base::FilePath(FILE_PATH_LITERAL(dir_name)); dir_name is not literal, so don't use FILE_PATH_LITERAL. Since this is compiled only on chromeos, you can just pass dir_name without the macro. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:10: #include "base/task_scheduler/task_traits.h" I don't think you need any of the above headers.
Done. Exciting! https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:7: #include "base/task_scheduler/task_traits.h" On 2017/03/02 20:50:16, waffles wrote: > Do you need the two above headers? Done. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:12: #include "content/public/browser/plugin_service.h" On 2017/03/02 20:50:16, waffles wrote: > Can we remove the dependency on plugin_service.h? Done. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:21: using content::PluginService; On 2017/03/02 20:50:16, waffles wrote: > remove this line Done. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:34: LOG(ERROR) << "Component registration failed"; On 2017/03/02 20:50:16, waffles wrote: > Can this be DVLOG(1)? > > Also on lines 30 and 49. Done. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:54: bool SkipCupsRegistration(ComponentUpdateService* cus) { On 2017/03/02 20:50:15, waffles wrote: > I'm not sure what the purpose of this function is. Can it be removed? Done. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:76: return false; On 2017/03/02 20:50:15, waffles wrote: > We should think about this carefully. Since the intent is not to ship these > components to everyone, can a MITM infer information (e.g. fingerprint the user) > based on which set of components they have installed? Can a MITM infer > information about how the user is using the browser? > > It might be best to set this to true for now. Done. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:106: return base::FilePath(FILE_PATH_LITERAL(dir_name)); On 2017/03/02 20:50:15, waffles wrote: > dir_name is not literal, so don't use FILE_PATH_LITERAL. > > Since this is compiled only on chromeos, you can just pass dir_name without the > macro. Done. https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/440001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:10: #include "base/task_scheduler/task_traits.h" On 2017/03/02 20:50:16, waffles wrote: > I don't think you need any of the above headers. Done.
lgtm (Please wait for an lgtm from sorin, too, since Sorin often catches things I miss.)
Thank you! Please excuse the brevity, I just wanted to get this done before the upcoming VC meeting. The comments are related to style issue, please consider addressing them. I will take another look soon after. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:11: #if defined(OS_CHROMEOS) #include "build/build_config.h" https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:56: for (unsigned int i = 0; i < sizeof(kSha2Hash_); i++) { use size_t for the loop variable. Also I wonder if you could use a for range construct to iterate through the array. Not sure how this compiles kSha2Hash_[i] = stoul() without truncation warnings. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:132: if (name.length() == 0) { use empty() https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:141: auto component = components.find(name); std::map has this odd api, where find returns an iterator to its value type, which in this case is a pair. The code reads better is we say: const auto it = components.find(name); if (it != components.end()) { .... https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:142: if (component != components.end()) { Some people prefer to handle the early exit from functions sooner. It makes the code easier to read, arguably. if (it == components.end()) { DVLOG...; return false; } .... return true; https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:14: struct ComponentConfig { Also, we usually declare ctor and dtor for such data types, otherwise they get inlined and bloat the code size. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:15: std::string name; include <string> https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:19: #if defined(OS_CHROMEOS) Superimportant: #include "build/build_config.h" when platform depended compile time definitions are used. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:22: CrOSComponentInstallerTraits(std::string dir_name, & to const type https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:22: CrOSComponentInstallerTraits(std::string dir_name, Can it take a ComponentConfig as a param? https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:42: #if defined(OS_CHROMEOS) #include "build/build_config.h"
Thanks! Done and tested on device. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:11: #if defined(OS_CHROMEOS) On 2017/03/02 22:27:25, Sorin Jianu wrote: > #include "build/build_config.h" Done. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:56: for (unsigned int i = 0; i < sizeof(kSha2Hash_); i++) { On 2017/03/02 22:27:24, Sorin Jianu wrote: > use size_t for the loop variable. Also I wonder if you could use a for range > construct to iterate through the array. Not sure how this compiles kSha2Hash_[i] > = stoul() without truncation warnings. Done. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:132: if (name.length() == 0) { On 2017/03/02 22:27:24, Sorin Jianu wrote: > use empty() Done. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:141: auto component = components.find(name); On 2017/03/02 22:27:25, Sorin Jianu wrote: > std::map has this odd api, where find returns an iterator to its value type, > which in this case is a pair. > > The code reads better is we say: > > const auto it = components.find(name); > if (it != components.end()) { > .... Done. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:142: if (component != components.end()) { On 2017/03/02 22:27:25, Sorin Jianu wrote: > Some people prefer to handle the early exit from functions sooner. It makes the > code easier to read, arguably. > > if (it == components.end()) { > DVLOG...; > return false; > } > > .... > return true; Done. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:14: struct ComponentConfig { On 2017/03/02 22:27:25, Sorin Jianu wrote: > Also, we usually declare ctor and dtor for such data types, otherwise they get > inlined and bloat the code size. Done. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:15: std::string name; On 2017/03/02 22:27:25, Sorin Jianu wrote: > include <string> Done. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:19: #if defined(OS_CHROMEOS) On 2017/03/02 22:27:25, Sorin Jianu wrote: > Superimportant: #include "build/build_config.h" > when platform depended compile time definitions are used. included once here. not including it in .cc file. is that fine? https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:22: CrOSComponentInstallerTraits(std::string dir_name, On 2017/03/02 22:27:25, Sorin Jianu wrote: > & to const type Done. https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2707063002/diff/460001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:42: #if defined(OS_CHROMEOS) On 2017/03/02 22:27:25, Sorin Jianu wrote: > #include "build/build_config.h" Done.
lgtm Thank you very much! A couple more style issues but lgtm otherwise. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:161: } ampty line https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:3: // found in the LICENSE file. Needs an empty line after this. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:13: #include "build/build_config.h" This can and must be included outside the #if directives since it is the header that defines some of these platform symbols. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:27: : name(name), dir(dir), sha2hashstr(sha2hashstr) {} we want to define the ctor and dtor in a cc file, otherwise they will still be inline. The issue here is that an inline ctor results into calling 3 constructors for the string members and may bloat the code size. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:30: #if defined(OS_CHROMEOS) an empty line between 29 and 30 would be helpful for readability. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:49: new CrOSMockComponentUpdateService()); can use base::MakeUnique<CrOSMockComponentUpdateService>() In general, direct calls to the new operator are looked upon with increased scrutiny and they are better avoided. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:56: new CrOSMockComponentUpdateService()); same https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:62: } // namespace component_updater an empty line above would help
Thanks! Done. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:161: } On 2017/03/03 02:19:03, Sorin Jianu wrote: > ampty line Done. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.h (right): https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:3: // found in the LICENSE file. On 2017/03/03 02:19:03, Sorin Jianu wrote: > Needs an empty line after this. Done. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:13: #include "build/build_config.h" On 2017/03/03 02:19:03, Sorin Jianu wrote: > This can and must be included outside the #if directives since it is the header > that defines some of these platform symbols. Done. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:27: : name(name), dir(dir), sha2hashstr(sha2hashstr) {} On 2017/03/03 02:19:03, Sorin Jianu wrote: > we want to define the ctor and dtor in a cc file, otherwise they will still be > inline. > The issue here is that an inline ctor results into calling 3 constructors for > the string members and may bloat the code size. Done. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.h:30: #if defined(OS_CHROMEOS) On 2017/03/03 02:19:03, Sorin Jianu wrote: > an empty line between 29 and 30 would be helpful for readability. Done. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:49: new CrOSMockComponentUpdateService()); On 2017/03/03 02:19:03, Sorin Jianu wrote: > can use base::MakeUnique<CrOSMockComponentUpdateService>() > > In general, direct calls to the new operator are looked upon with increased > scrutiny and they are better avoided. Done. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:56: new CrOSMockComponentUpdateService()); On 2017/03/03 02:19:03, Sorin Jianu wrote: > same Done. https://codereview.chromium.org/2707063002/diff/480001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:62: } // namespace component_updater On 2017/03/03 02:19:03, Sorin Jianu wrote: > an empty line above would help Done.
The CQ bit was checked by xiaochu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xiaochu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaochu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org, sorin@chromium.org Link to the patchset: https://codereview.chromium.org/2707063002/#ps520001 (title: "build unit_test only on chromeos")
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": 520001, "attempt_start_ts": 1488569588791760,
"parent_rev": "e29cfa202aa6f41e862093ddb24353aa27638afb", "commit_rev":
"4c9aa5146e5df6682b3fa25e050571c18fd127e6"}
Message was sent while issue was closed.
Description was changed from ========== Universial component install for chrome os. Adding a component installer for all chrome os components. It is invoked with a component name and then it looks up this component in a configuration file (https://chromium.googlesource.com/chromiumos/platform2/+/master/cros_componen...). If this file exists, valid and this component is in the file, then this component got registered. Otherwise, nothing happens. Support key per component. Each key is also stored in config file. format of this key is verified when it is parsed. unit test includes component_update_service on any platform and config file parsing on chrome os to cover many cases. BUG=690521, 694731 TEST=cros_component_installer_unittest tests the configuration file parser on chrome os. Component registeration itself is tested on a chrome device. ========== to ========== Universial component install for chrome os. Adding a component installer for all chrome os components. It is invoked with a component name and then it looks up this component in a configuration file (https://chromium.googlesource.com/chromiumos/platform2/+/master/cros_componen...). If this file exists, valid and this component is in the file, then this component got registered. Otherwise, nothing happens. Support key per component. Each key is also stored in config file. format of this key is verified when it is parsed. unit test includes component_update_service on any platform and config file parsing on chrome os to cover many cases. BUG=690521, 694731 TEST=cros_component_installer_unittest tests the configuration file parser on chrome os. Component registeration itself is tested on a chrome device. Review-Url: https://codereview.chromium.org/2707063002 Cr-Commit-Position: refs/heads/master@{#454650} Committed: https://chromium.googlesource.com/chromium/src/+/4c9aa5146e5df6682b3fa25e0505... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:520001) as https://chromium.googlesource.com/chromium/src/+/4c9aa5146e5df6682b3fa25e0505... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
