Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898063002:
Hold cros_component_installers in browser_process. (Closed)
DescriptionHold cros_component_installers in browser_process.
Create a factory method to create and hold installers for chrome os
components.
We add this since we need to query info from components. Since these
info are already in memory, it is inefficient load and search in hard
drive every time (and the complexity of doing so is pretty prohibitive).
BUG=chromium:690521
TEST= 1) unit test: cros_component_installer_unittest.cc, 2)run on a chromebook and register a component successfully.
Review-Url: https://codereview.chromium.org/2898063002
Cr-Commit-Position: refs/heads/master@{#474798}
Committed: https://chromium.googlesource.com/chromium/src/+/be8885d7172d99cd8fff3a6782bd2aeac4a10371
Patch Set 1 #Patch Set 2 : update commit message #
Total comments: 4
Patch Set 3 : add unit test #Patch Set 4 : update commit message. #
Total comments: 2
Patch Set 5 : move to browser_process_platform_part #Patch Set 6 : remove empty line. #Patch Set 7 : rename function #Patch Set 8 : update unit test #Patch Set 9 : switch to smart pointer #
Total comments: 4
Patch Set 10 : use CrOSComponentInstaller inherited from DefaultComponentInstaller #Patch Set 11 : address comments from sky #Patch Set 12 : trim code #
Total comments: 8
Patch Set 13 : address comments from sky&waffles #Patch Set 14 : trim code #Patch Set 15 : update unit test #Patch Set 16 : address comments from waffles #Patch Set 17 : remove unused headers #Patch Set 18 : new line between 5/6 #
Total comments: 4
Patch Set 19 : address comments from waffles #
Total comments: 2
Patch Set 20 : address comments from sky #Patch Set 21 : update unittest #Patch Set 22 : update unittest #Patch Set 23 : update unittest #Patch Set 24 : update unittest #
Total comments: 2
Patch Set 25 : address comments from sky #Patch Set 26 : remove dead code #
Messages
Total messages: 57 (31 generated)
Description was changed from ========== Hold cros_component_installers in browser_process. Create a factory method to create and hold installers for chrome os components. We add this since we need to query info from components. Since these info are already in memory, it is inefficient load and search in hard drive every time (and the complexity of doing so is pretty prohibitive). BUG=chromium:690521 TEST= run on a chromebook and register a component successfully. ========== to ========== Hold cros_component_installers in browser_process. Create a factory method to create and hold installers for chrome os components. We add this since we need to query info from components. Since these info are already in memory, it is inefficient load and search in hard drive every time (and the complexity of doing so is pretty prohibitive). BUG=chromium:690521 TEST= run on a chromebook and register a component successfully. ==========
xiaochu@chromium.org changed reviewers: + waffles@chromium.org
xiaochu@chromium.org changed reviewers: + jochen@chromium.org
xiaochu@chromium.org changed reviewers: + sky@chromium.org
On 2017/05/22 22:31:05, xiaochu wrote: > mailto:xiaochu@chromium.org changed reviewers: > + mailto:sky@chromium.org Can you please take a look? Thanks!
it's not clear to me what this tries to achieve. Could you please share a design doc so I can read up on it? (plz also share with chrome-design-docs@) In any case, should there be a test?
On 2017/05/23 11:22:03, jochen wrote: > it's not clear to me what this tries to achieve. > > Could you please share a design doc so I can read up on it? (plz also share with > chrome-design-docs@) > > In any case, should there be a test? I shared the doc (go/cros_component) with you and chrome-design-docs@. Thanks for pointing this out. I added the test as well.
Description was changed from ========== Hold cros_component_installers in browser_process. Create a factory method to create and hold installers for chrome os components. We add this since we need to query info from components. Since these info are already in memory, it is inefficient load and search in hard drive every time (and the complexity of doing so is pretty prohibitive). BUG=chromium:690521 TEST= run on a chromebook and register a component successfully. ========== to ========== Hold cros_component_installers in browser_process. Create a factory method to create and hold installers for chrome os components. We add this since we need to query info from components. Since these info are already in memory, it is inefficient load and search in hard drive every time (and the complexity of doing so is pretty prohibitive). BUG=chromium:690521 TEST= 1) unit test: cros_component_installer_unittest.cc, 2)run on a chromebook and register a component successfully. ==========
https://codereview.chromium.org/2898063002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_process.h (right): https://codereview.chromium.org/2898063002/diff/20001/chrome/browser/browser_... chrome/browser/browser_process.h:294: #if defined(OS_CHROMEOS) Can this be moved to browser_process_platform_part_chromeos? https://codereview.chromium.org/2898063002/diff/20001/chrome/browser/browser_... chrome/browser/browser_process.h:296: cros_component_installer( Please name this GetCrosComponentInstaller().
sky@ and jochen@: this CL represents one suggestion I made to xiaochu about how to communicate information out of the CrOS component installer(s) to some consumer function. The component installer has a piece of data from the component's manifest.json file. Elsewhere, a static function needs to branch on that data. It would be great if you could give some direction on how best to move the information. The idea in this CL is that the static function can get a reference to appropriate installer via the g_browser_process global, and then call a function of the installer to return the value. Another idea would be to establish a global object that the installers mutate and the static code queries. Another idea would be to use the pref service to communicate this value. Another idea would be to have the static function go to disk and reread/parse the manifest.json file and extract the value directly. And I'm sure you can think of more. Which is best? https://codereview.chromium.org/2898063002/diff/60001/chrome/browser/browser_... File chrome/browser/browser_process.h (right): https://codereview.chromium.org/2898063002/diff/60001/chrome/browser/browser_... chrome/browser/browser_process.h:268: virtual component_updater::DefaultComponentInstaller* I think you actually want to return the Traits object. Having the DCI won't help you much. Also, can we return a const reference (or at least a smart pointer)?
Thanks for clarifying the problem, waffles@! I'm trying to save a reference to some data in memory since one of the static functions needs to access this mutable data. I'm open to all suggestions. Please advise. https://codereview.chromium.org/2898063002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_process.h (right): https://codereview.chromium.org/2898063002/diff/20001/chrome/browser/browser_... chrome/browser/browser_process.h:294: #if defined(OS_CHROMEOS) On 2017/05/23 20:22:23, sky wrote: > Can this be moved to browser_process_platform_part_chromeos? Done. https://codereview.chromium.org/2898063002/diff/20001/chrome/browser/browser_... chrome/browser/browser_process.h:296: cros_component_installer( On 2017/05/23 20:22:23, sky wrote: > Please name this GetCrosComponentInstaller(). Done. https://codereview.chromium.org/2898063002/diff/60001/chrome/browser/browser_... File chrome/browser/browser_process.h (right): https://codereview.chromium.org/2898063002/diff/60001/chrome/browser/browser_... chrome/browser/browser_process.h:268: virtual component_updater::DefaultComponentInstaller* On 2017/05/23 20:40:14, waffles wrote: > I think you actually want to return the Traits object. Having the DCI won't help > you much. > Also, can we return a const reference (or at least a smart pointer)? Done. Thanks for pointing this out. I just realized installer does not have function to return manifest. I save installer here because it holds all the data about component in memory (in addition to manifest and traits).
On 2017/05/23 20:40:14, waffles wrote: > sky@ and jochen@: this CL represents one suggestion I made to xiaochu about how > to communicate information out of the CrOS component installer(s) to some > consumer function. The component installer has a piece of data from the > component's manifest.json file. Elsewhere, a static function needs to branch on > that data. It would be great if you could give some direction on how best to > move the information. > > The idea in this CL is that the static function can get a reference to > appropriate installer via the g_browser_process global, and then call a function > of the installer to return the value. > > Another idea would be to establish a global object that the installers mutate > and the static code queries. > > Another idea would be to use the pref service to communicate this value. > > Another idea would be to have the static function go to disk and reread/parse > the manifest.json file and extract the value directly. > > And I'm sure you can think of more. Which is best? > > https://codereview.chromium.org/2898063002/diff/60001/chrome/browser/browser_... > File chrome/browser/browser_process.h (right): > > https://codereview.chromium.org/2898063002/diff/60001/chrome/browser/browser_... > chrome/browser/browser_process.h:268: virtual > component_updater::DefaultComponentInstaller* > I think you actually want to return the Traits object. Having the DCI won't help > you much. > Also, can we return a const reference (or at least a smart pointer)? I'm not familiar enough with this code to know what the right direction is. Looking at the cl I have to wonder if CrOSComponent should be a real object. That way it can own the state. In such a scenario BrowserProcessPlatformPart would own CrOSComponent. But again, I'm not familiar with this code know what the right ownership model is.
https://codereview.chromium.org/2898063002/diff/160001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.cc (right): https://codereview.chromium.org/2898063002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.cc:163: scoped_refptr<component_updater::DefaultComponentInstaller> Style guide says position definition and declaration position should match (in other words, move this above CreateProfileHelper). https://codereview.chromium.org/2898063002/diff/160001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2898063002/diff/160001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:137: // |cus| will take ownership of |installer| during Update comment? Also, I'm rather confused about the ownership model. I don't see any other changes to CrOSComponentInstallerTraits, so how is it that the installer created by GetCrosComponentInstaller() isn't destroyed like this comment implies it was?
Thanks! The original ownership model (not strict) should be ComponentUpdateService owns DefaultComponentInstaller which owns CrOSComponentTraits. It works fine previously since ComponentUpdateService automatically performs update with very few intervention. However, our new change (Chrome OS component) does many on-demand operations which disrupts the original model. And we need to read data in Traits and Installer from time to time. My new patch set creates a new installer CrOSComponentInstaller that expose manifest (member of installer) to callers. And ComponentUpdateService in this case does not solely own it any more (fortunately the implementation is not too complicated since old model is extensible). https://codereview.chromium.org/2898063002/diff/160001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.cc (right): https://codereview.chromium.org/2898063002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.cc:163: scoped_refptr<component_updater::DefaultComponentInstaller> On 2017/05/23 23:20:29, sky wrote: > Style guide says position definition and declaration position should match (in > other words, move this above CreateProfileHelper). Done. https://codereview.chromium.org/2898063002/diff/160001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2898063002/diff/160001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:137: // |cus| will take ownership of |installer| during On 2017/05/23 23:20:30, sky wrote: > Update comment? > > Also, I'm rather confused about the ownership model. I don't see any other > changes to CrOSComponentInstallerTraits, so how is it that the installer created > by GetCrosComponentInstaller() isn't destroyed like this comment implies it was? Done.
OK; looking at this I would rather we not introduce a subclass of DCI. Rather than having BPPP have a function to return the installer, could we just place a std::vector<std::string> in BPPP that represents the set of known-compatible component-names? (std::set would work as well) CrOSComponentInstallerTraits::ComponentReady would check the component's min_env_version with the hardcoded env_version and if and only if the component is compatible, would place its name into the vector. The static function can then just check for membership in the vector/set and branch on that. (Assuming it runs only on the UI thread.) During shutdown, I think BPPP outlives the component updater, and I assume it outlives whatever code will call the static function. Would that work?
https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.h (right): https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.h:139: std::map<std::string, include <map> and <string> https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:5: #include "chrome/browser/component_updater/cros_component_installer.h" style guide says newline between 5/6. https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:85: #if defined(OS_CHROMEOS) Shouldn't this file only be compiled on chromeos? https://codereview.chromium.org/2898063002/diff/220001/components/component_u... File components/component_updater/default_component_installer.h (right): https://codereview.chromium.org/2898063002/diff/220001/components/component_u... components/component_updater/default_component_installer.h:129: std::unique_ptr<base::DictionaryValue> current_manifest_; Style guide says no protected members.
thanks for sharing the design doc. I see that sky@ started the actual review, so I'll defer to them.
Thanks! waffles@ I removed subclass. In BPPP, I added a map from 'name' to manifest. I didn't use vector because the check has to happen at loading time (instead of ComponentReady which always reports compatible otherwise component wouldn't get downloaded in the first place). Since the incompatibility happens when system is updated while component is outdated, we need to store min_env_version for each installed components and avoids loading when env_version is up-reved. Please let me know your opinion. https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.h (right): https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.h:139: std::map<std::string, On 2017/05/24 16:47:20, sky wrote: > include <map> and <string> Done. https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:5: #include "chrome/browser/component_updater/cros_component_installer.h" On 2017/05/24 16:47:20, sky wrote: > style guide says newline between 5/6. Done. https://codereview.chromium.org/2898063002/diff/220001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer_unittest.cc:85: #if defined(OS_CHROMEOS) On 2017/05/24 16:47:20, sky wrote: > Shouldn't this file only be compiled on chromeos? Done. Yes, macro removed. https://codereview.chromium.org/2898063002/diff/220001/components/component_u... File components/component_updater/default_component_installer.h (right): https://codereview.chromium.org/2898063002/diff/220001/components/component_u... components/component_updater/default_component_installer.h:129: std::unique_ptr<base::DictionaryValue> current_manifest_; On 2017/05/24 16:47:20, sky wrote: > Style guide says no protected members. Done.
On 2017/05/24 19:56:55, xiaochu wrote: > Thanks! > > waffles@ I removed subclass. In BPPP, I added a map from 'name' to manifest. > > I didn't use vector because the check has to happen at loading time (instead of > ComponentReady which always reports compatible otherwise component wouldn't get > downloaded in the first place). Since the incompatibility happens when system is > updated while component is outdated, we need to store min_env_version for each > installed components and avoids loading when env_version is up-reved. > > Please let me know your opinion. Hm. I don't see how it's possible for ComponentReady to think the environment is OK but for that decision to change at loading time. The system update will always require a reboot to the new partition (which will cause any value stored in BPPP to be lost). ComponentReady is called when the browser starts (and on each successful update to the component), so the incompatibility check can be done in ComponentReady directly: I mean, the env_version is hard-coded in the very same file. > ComponentReady which always reports compatible otherwise component wouldn't get > downloaded in the first place ComponentReady is run at browser start with the current on-disk version of the component (assuming the component is registered). So it's entirely possible that it downloads compatible version 1, the system updates and reboots, now with env_version=2, and then ComponentReady is called with the old env_version.
Thanks! I get it now. The changes are made.
lgtm I'm OK with this if sky is. https://codereview.chromium.org/2898063002/diff/340001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.cc (right): https://codereview.chromium.org/2898063002/diff/340001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.cc:174: } Just "return it != compatible_cros_components_.end();" https://codereview.chromium.org/2898063002/diff/340001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2898063002/diff/340001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:158: {{"env_version", "2.0"}, Did you mean to change this line?
Thanks! https://codereview.chromium.org/2898063002/diff/340001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.cc (right): https://codereview.chromium.org/2898063002/diff/340001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.cc:174: } On 2017/05/24 21:14:35, waffles wrote: > Just "return it != compatible_cros_components_.end();" Done. https://codereview.chromium.org/2898063002/diff/340001/chrome/browser/compone... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2898063002/diff/340001/chrome/browser/compone... chrome/browser/component_updater/cros_component_installer.cc:158: {{"env_version", "2.0"}, On 2017/05/24 21:14:35, waffles wrote: > Did you mean to change this line? Done. Sorry about that. I'm saving this to next CL.
https://codereview.chromium.org/2898063002/diff/360001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.h (right): https://codereview.chromium.org/2898063002/diff/360001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.h:133: std::vector<std::string> compatible_cros_components_; Given your usage, wouldn't a std::set be a better fit? And possibly a flat_set if the size is small?
Thanks! https://codereview.chromium.org/2898063002/diff/360001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.h (right): https://codereview.chromium.org/2898063002/diff/360001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.h:133: std::vector<std::string> compatible_cros_components_; On 2017/05/25 02:25:50, sky wrote: > Given your usage, wouldn't a std::set be a better fit? And possibly a flat_set > if the size is small? Done. Yeah, max size is number of cros components (currently only one). Thanks for pointing this out.
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by 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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2898063002/diff/460001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.cc (right): https://codereview.chromium.org/2898063002/diff/460001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.cc:165: return it != compatible_cros_components_.end(); return compatible_cros_components_.count(name) > 0
Thanks! https://codereview.chromium.org/2898063002/diff/460001/chrome/browser/browser... File chrome/browser/browser_process_platform_part_chromeos.cc (right): https://codereview.chromium.org/2898063002/diff/460001/chrome/browser/browser... chrome/browser/browser_process_platform_part_chromeos.cc:165: return it != compatible_cros_components_.end(); On 2017/05/25 18:03:50, sky wrote: > return compatible_cros_components_.count(name) > 0 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by 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, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2898063002/#ps500001 (title: "remove dead code")
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": 500001, "attempt_start_ts": 1495745222196280,
"parent_rev": "54b6e52c6f6407acc0b4ea107dbfa18b2ba3eb0c", "commit_rev":
"be8885d7172d99cd8fff3a6782bd2aeac4a10371"}
Message was sent while issue was closed.
Description was changed from ========== Hold cros_component_installers in browser_process. Create a factory method to create and hold installers for chrome os components. We add this since we need to query info from components. Since these info are already in memory, it is inefficient load and search in hard drive every time (and the complexity of doing so is pretty prohibitive). BUG=chromium:690521 TEST= 1) unit test: cros_component_installer_unittest.cc, 2)run on a chromebook and register a component successfully. ========== to ========== Hold cros_component_installers in browser_process. Create a factory method to create and hold installers for chrome os components. We add this since we need to query info from components. Since these info are already in memory, it is inefficient load and search in hard drive every time (and the complexity of doing so is pretty prohibitive). BUG=chromium:690521 TEST= 1) unit test: cros_component_installer_unittest.cc, 2)run on a chromebook and register a component successfully. Review-Url: https://codereview.chromium.org/2898063002 Cr-Commit-Position: refs/heads/master@{#474798} Committed: https://chromium.googlesource.com/chromium/src/+/be8885d7172d99cd8fff3a6782bd... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001) as https://chromium.googlesource.com/chromium/src/+/be8885d7172d99cd8fff3a6782bd...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 474798 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2017/05/26 00:25:47, findit-for-me wrote: > Findit (https://goo.gl/kROfz5) identified this CL at revision 474798 as the > culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... Unittest has a raw pointer which causes memory lea. Working on a fix.
Message was sent while issue was closed.
On 2017/05/26 00:29:51, xiaochu wrote: > On 2017/05/26 00:25:47, findit-for-me wrote: > > Findit (https://goo.gl/kROfz5) identified this CL at revision 474798 as the > > culprit for > > failures in the build cycles as shown on: > > > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... > > Unittest has a raw pointer which causes memory lea. Working on a fix. Fix is here: https://codereview.chromium.org/2902373003 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
