Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854983002:
Add the ThirdPartyModules.Uninstallable histogram. (Closed)
|
Created:
3 years, 7 months ago by Patrick Monette Modified:
3 years, 6 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the ThirdPartyModules.Uninstallable histogram.
This metric tracks how many third-party modules could potentially be
matched to an entry in the Apps & Features settings page.
BUG=717696
Review-Url: https://codereview.chromium.org/2854983002
Cr-Commit-Position: refs/heads/master@{#476359}
Committed: https://chromium.googlesource.com/chromium/src/+/721fa86c3f15c5c695070bc4f8358d2eff23a297
Patch Set 1 #
Total comments: 37
Patch Set 2 : Split in multiple CLs #
Total comments: 2
Patch Set 3 : Rebase + fix clang warning #Patch Set 4 : Rebase #
Messages
Total messages: 35 (17 generated)
pmonette@chromium.org changed reviewers: + chrisha@chromium.org
PTAL
Great start! Is it worth also implementing the other ThirdParty metrics in third_party_metrics_recorder? Most of them we won't care about but at least audit them and see if there are any we do still want to track. Small note: You could easily have chopped this up into many sub CLs that are each independent and quicker to review. My personal opinion is that the optimal size for a CL is < 100 lines. - Create ModuleDatabaseObserver and plumbing. - Create Msi wrapper. - Create InstalledPrograms. - Create ThirdPartyMetricsRecorder. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... File chrome/browser/conflicts/installed_programs_win.cc (right): https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:21: // could not be retrived. retrieved* https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:37: // be retrived. retrieved* https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:77: // An invalid product guid may have been passed to this function. In those In this* https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:86: if (!IsRegistryComponentPath(component_path)) { I have a very minor preference: if (...) continue; // Rest of logic. ... It avoids one level of indentation. YMMV. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:88: if (base::EqualsCaseInsensitiveASCII(file_path.Extension(), L".dll")) This might be an artificial constraint. Loadable modules aren't required to have .dll as an extension, and can in fact have anything.... the only real constraint is that they be PE files. This is probably fine for me, but let's at least add a comment. It might also be interesting to log a metric that count show many loaded modules don't have .dll as an extension? https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:130: // Because this class is used to display warning to the user, not having the s/the// https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:133: if (!GetValue(candidate, L"DisplayName", &display_name)) In this case could we directly check the image file for a ProductName? This is one failure case that might be useful to explicitly count via UMA, to know how big an issue it really is. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:141: return; So if we find an InstallPath then we don't want to also query the MSI? Why? https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:156: bool BinarySearch( Yay, you can write a binary search! You're hired :) This is a very generic and non-descriptive name. Also, don't write your own binary search. Use std::binary_search and a custom comparator. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:239: install_locations_ = std::move(internal_data->install_locations); Just std::move the entire InternalData object? https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... File chrome/browser/conflicts/installed_programs_win.h (right): https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:19: class InstalledPrograms { Some class documentation might be useful. How is this thing intended to be used? Who will own it? What is it's lifetime? What is the threading model? https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:21: // This structure has the same representation than the internal state of as* the internal https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:22: // InstalledPrograms. It is created on a background sequenced and passed back sequence* https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:24: struct InternalData { Is there a reason why we don't simply document these variables here, and have InstalledPrograms own an instance of InternalData? Which is then moved on the OnInternalDataReceived callback? https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:34: void Initialize(const base::Closure& on_initialized_callback); Invokes the callback on the background sequence, or on the sequence calling Initialize? Please clarify. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:44: // sequence. Document the fact that these will be used by Initialize, and *will* be executed in the task scheduler with background priority (rather than "should") (It wasn't clear to me how these were going to be used until I read the full impl.) https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:61: base::flat_map<base::FilePath, size_t, FilePathLess> dll_map_; Is there a reason why we use a flat_map here and a std::vector above? https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:379: return std::tie(process_id, creation_time) < I always forget about 'tie'.... https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/ms... File chrome/browser/conflicts/msi_util_win.h (right): https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/ms... chrome/browser/conflicts/msi_util_win.h:33: }; I'm not a huge fan of this pattern as there's effectively a global singleton that is introduced. Maybe a class with an interface, and a mock implementation of that interface for testing? And an explicit injection of an instance of that interface? This is slightly less "magical" and prone to abuse than introducing yet another singleton. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/win/enumerat... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2854983002/diff/1/chrome/browser/win/enumerat... chrome/browser/win/enumerate_modules_model.cc:275: module.recommended_action = RecommendedAction::UNINSTALL; This is debugging code?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
PTAL Lots of comments that are no longer applicable to this CL. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... File chrome/browser/conflicts/installed_programs_win.cc (right): https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:21: // could not be retrived. On 2017/05/02 21:28:24, chrisha wrote: > retrieved* Done. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:37: // be retrived. On 2017/05/02 21:28:25, chrisha wrote: > retrieved* Done. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:77: // An invalid product guid may have been passed to this function. In those On 2017/05/02 21:28:24, chrisha wrote: > In this* Done. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:86: if (!IsRegistryComponentPath(component_path)) { On 2017/05/02 21:28:25, chrisha wrote: > I have a very minor preference: > > if (...) > continue; > > // Rest of logic. > ... > > It avoids one level of indentation. YMMV. Done. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:88: if (base::EqualsCaseInsensitiveASCII(file_path.Extension(), L".dll")) On 2017/05/02 21:28:24, chrisha wrote: > This might be an artificial constraint. Loadable modules aren't required to have > .dll as an extension, and can in fact have anything.... the only real constraint > is that they be PE files. > > This is probably fine for me, but let's at least add a comment. > > It might also be interesting to log a metric that count show many loaded modules > don't have .dll as an extension? This was mainly done so that searching for the associated program is a bit faster (it doesn't have to check for unrelated files). I've removed it. I'll add a metric to track this too. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:130: // Because this class is used to display warning to the user, not having the On 2017/05/02 21:28:25, chrisha wrote: > s/the// Done. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:133: if (!GetValue(candidate, L"DisplayName", &display_name)) On 2017/05/02 21:28:25, chrisha wrote: > In this case could we directly check the image file for a ProductName? > It would be possible. But remember that having no DisplayName in the registry means the entry in the Apps&Features page is not recognizable. > This is one failure case that might be useful to explicitly count via UMA, to > know how big an issue it really is. I agree. Done. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:141: return; On 2017/05/02 21:28:25, chrisha wrote: > So if we find an InstallPath then we don't want to also query the MSI? Why? If the InstallPath is good, it's faster to check via IsParent() and takes less memory than to compare against a bunch of files retrieved from the MSI. Were you suggesting that we check both to potentially get better coverage? https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:156: bool BinarySearch( On 2017/05/02 21:28:25, chrisha wrote: > Yay, you can write a binary search! You're hired :) > Yay! > This is a very generic and non-descriptive name. Also, don't write your own > binary search. Use std::binary_search and a custom comparator. Mmm this is a case of premature optimization on my part. I'll switch to a flat_map with this Compare function which often unnecessarily call IsParent(): bool MyCompare(const base::FilePath& lhs, const base::FilePath& rhs) { if (lhs.IsParent(rhs) || rhs.IsParent(lhs)) return false; return base::FilePath::CompareLessIgnoreCase(lhs.value(), rhs.value()); } https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.cc:239: install_locations_ = std::move(internal_data->install_locations); On 2017/05/02 21:28:25, chrisha wrote: > Just std::move the entire InternalData object? Done. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... File chrome/browser/conflicts/installed_programs_win.h (right): https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:19: class InstalledPrograms { On 2017/05/02 21:28:25, chrisha wrote: > Some class documentation might be useful. How is this thing intended to be used? > Who will own it? What is it's lifetime? What is the threading model? I added documentation. I feel like only a small description and a note on thread-safety is needed. Nothing special is going on with the lifetime and it's only meant to be owned via composition. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:21: // This structure has the same representation than the internal state of On 2017/05/02 21:28:25, chrisha wrote: > as* the internal I got rid of InternalData. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:22: // InstalledPrograms. It is created on a background sequenced and passed back On 2017/05/02 21:28:25, chrisha wrote: > sequence* Ditto. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:24: struct InternalData { On 2017/05/02 21:28:25, chrisha wrote: > Is there a reason why we don't simply document these variables here, and have > InstalledPrograms own an instance of InternalData? Which is then moved on the > OnInternalDataReceived callback? No longer applicable. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/in... chrome/browser/conflicts/installed_programs_win.h:61: base::flat_map<base::FilePath, size_t, FilePathLess> dll_map_; On 2017/05/02 21:28:25, chrisha wrote: > Is there a reason why we use a flat_map here and a std::vector above? Switched both to flat_map. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/ms... File chrome/browser/conflicts/msi_util_win.h (right): https://codereview.chromium.org/2854983002/diff/1/chrome/browser/conflicts/ms... chrome/browser/conflicts/msi_util_win.h:33: }; On 2017/05/02 21:28:25, chrisha wrote: > I'm not a huge fan of this pattern as there's effectively a global singleton > that is introduced. Maybe a class with an interface, and a mock implementation > of that interface for testing? And an explicit injection of an instance of that > interface? > > This is slightly less "magical" and prone to abuse than introducing yet another > singleton. Done. https://codereview.chromium.org/2854983002/diff/1/chrome/browser/win/enumerat... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2854983002/diff/1/chrome/browser/win/enumerat... chrome/browser/win/enumerate_modules_model.cc:275: module.recommended_action = RecommendedAction::UNINSTALL; On 2017/05/02 21:28:25, chrisha wrote: > This is debugging code? Yes. Removed.
This lgtm! (Looking forward: In the code that adds the warnings, it would also be great to have a metric that counts how many different warnings the user would be presented with overall, and broken down by type.) https://codereview.chromium.org/2854983002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2854983002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:380: std::make_tuple(pik.process_id, pik.creation_time); Is there a reason why tie doesn't work here? (ie, by reference instead of copying?)
Thanks https://codereview.chromium.org/2854983002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2854983002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:380: std::make_tuple(pik.process_id, pik.creation_time); On 2017/05/30 19:30:03, chrisha wrote: > Is there a reason why tie doesn't work here? (ie, by reference instead of > copying?) I want to change it eventually but it seems too unrelated to this patch. Anyway in this specific case process_id and creation_time are integers so there is no real benefit in changing it. The few time you saw it in recent code reviews was because I sometime was forgetting to remove it.
The CQ bit was checked by pmonette@chromium.org
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
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2854983002/#ps80001 (title: "Rebase + fix clang warning")
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
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2854983002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pmonette@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@ PTAL at histograms.xml
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by pmonette@chromium.org
Thanks!
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by pmonette@chromium.org
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": 100001, "attempt_start_ts": 1496338468505360,
"parent_rev": "cd00499d569a693685c81d4042b3cfe3219edd61", "commit_rev":
"721fa86c3f15c5c695070bc4f8358d2eff23a297"}
Message was sent while issue was closed.
Description was changed from ========== Add the ThirdPartyModules.Uninstallable histogram. This metric tracks how many third-party modules could potentially be matched to an entry in the Apps & Features settings page. BUG=717696 ========== to ========== Add the ThirdPartyModules.Uninstallable histogram. This metric tracks how many third-party modules could potentially be matched to an entry in the Apps & Features settings page. BUG=717696 Review-Url: https://codereview.chromium.org/2854983002 Cr-Commit-Position: refs/heads/master@{#476359} Committed: https://chromium.googlesource.com/chromium/src/+/721fa86c3f15c5c695070bc4f835... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/721fa86c3f15c5c695070bc4f835... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
