Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
729273002:
Modernize on-demand update checks on Windows. (Closed)
|
Created:
6 years, 1 month ago by grt (UTC plus 2) Modified:
6 years ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionModernize on-demand update checks on Windows.
The code was a bit long in the tooth. Some changes:
- There is now a fairly comprehensive unit test.
- Update checks no longer do a nested Run() of the FILE thread's message
loop. Rather, they return to the main loop while waiting for Google
Update.
- A callback is used to return the results rather than a single-method
listener interface.
- Lifetime management is simpler: the interface can be fire-and-forget
since the caller could specify a null callback.
BUG=424689
Committed: https://crrev.com/5a83109b2fe5f328635cc297dc6db67518071dda
Cr-Commit-Position: refs/heads/master@{#306421}
Patch Set 1 #
Total comments: 25
Patch Set 2 : disable tests that don't apply to chromium builds #
Total comments: 42
Patch Set 3 : comments #
Total comments: 13
Patch Set 4 : more comments #Patch Set 5 : more comments #Patch Set 6 : moved scoped_path_override to issue 766813002 #Patch Set 7 : ForTesting since it's enforced by presubmit checks #
Total comments: 6
Patch Set 8 : sync to position 306381 #Patch Set 9 : final tweaks #
Messages
Total messages: 34 (15 generated)
Patchset #1 (id:1) has been deleted
grt@chromium.org changed reviewers: + dbeam@chromium.org, pkasting@chromium.org
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:100001) has been deleted
grt@chromium.org changed reviewers: + phajdan.jr@chromium.org
Please take a look as per: pkasting: chrome/browser/google/* dbeam: chrome/browser/ui/webui/* phajdan.jr: base/test/* Thanks.
https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:46: DCHECK_EQ(!InstallUtil::IsPerUserInstall(chrome_exe_path.value().c_str()), Nit: DCHECK_NE and remove the ! https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:92: wchar_t class_id_as_string[MAX_PATH] = {}; Nit: "{ 0 }" is more common to indicate "I'm trying to zero-initialize everything here", I would leave it here and use it on line 100 as well. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:126: // instance. I'm confused. This comment seems to actually be talking about the return statement on line 130? https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:146: GoogleUpdateJobObserver() Nit: While here: Prefer not to inline non-"cheap accessor" class methods even for classes declared in C++ files. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:312: void UpdateCheckDriver::RunUpdateCheck( It seems like the only reason we need this static method is that the constructor is private. That in turn seems like needless paranoia given that the class is defined within a .cc file. I suggest making the constructor public and then inlining this method into the lone caller below. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:71: namespace chrome { The chrome namespace is deprecated (see http://crbug.com/289619 ); please don't add anything new to it. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:83: // Initiates a update check on FILE thread. If |install_if_newer| is true, an Nit: a -> an; on -> on the https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:84: // update will be applied. |elevation_window| is the window by which elevation Nit: "...the window which should own any necessary elevation UI." https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:88: HWND elevation_window, // gfx::AcceleratedWidget Why are you using HWNDs and then commenting that they're AcceleratedWidgets? Why not use AcceleratedWidget here? https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:103: // Initiates a update check on |task_runner|, which must run tasks on a thread Nit: Don't duplicate the comments above here; say how this differs from the function above and why we need it. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:55: LOG(ERROR) << L"Google Update cannot update Chrome installed in a " Do we actually obtain and use this log? If not, remove it; we normally avoid logging statements unless there's some mechanism to collect their data, since users don't see them anyway. (Same comment applies to the logging statement that got added later on) https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:148: result_(UPGRADE_ERROR) {} While here: Prefer not to inline non-"cheap accessor" methods, even for classes declared in a .cc file. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:153: void SetOnCompleteCallback(const base::Closure& on_complete_callback) { Nit: Cheap accessors that get inlined should be named unix_hacker()-style. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:160: DCHECK(result_ != UPGRADE_STARTED && result_ != UPGRADE_CHECK_STARTED); Nit: Use two DCHECK_NEs https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:247: }; Nit: While here: DISALLOW_COPY_AND_ASSIGN (both classes) https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:336: // Runs on the caller's thread. Nit: Is this comment necessary, what with the first DCHECK below? https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:340: DCHECK_EQ(result_ == UPGRADE_ERROR, error_code_ != GOOGLE_UPDATE_NO_ERROR); Nit: I would find this slightly easier to understand if it was DCHECK_NE and != was changed to ==. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:382: HRESULT hr = S_OK; Nit: Just init |hr| directly to the result of CreateInstance(). https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:406: if (!install_if_newer) Nit: Remove ! and switch arms https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:411: if (hr != S_OK) { Nit: Shorter: if (hr == S_OK) return true; OnUpgradeError(GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR, hr, system_level); return false; https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:428: on_demand_ = nullptr; Don't these need to .Release(), rather than just being set to null? https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:451: void CheckForUpdate(bool install_if_newer, I don't think this wrapper buys us very much. AFAICT there are only two callers, both in version_updater_win.cc, which pass the same second and third args. I think I would eliminate this function and expose the one below in the header file; then in version_updater_win.cc, optionally write a wrapper that takes only a bool arg and makes the four-arg call. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:456: content::BrowserThread::FILE)); Note that technically you don't need to declare a temp for this (you could just inline it in the call). The lifetime will be extended until the call returns. Up to you. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win_unittest.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win_unittest.cc:256: }; Nit: DISALLOW_COPY_AND_ASSIGN (both classes) https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win_unittest.cc:385: // Test that an update check failes with the proper error code if Chrome isn't Nit: fails
dbeam@chromium.org changed reviewers: + jhawkins@chromium.org - dbeam@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
-dbeam@, +jhawkins@
https://codereview.chromium.org/729273002/diff/140001/base/test/scoped_path_o... File base/test/scoped_path_override.cc (right): https://codereview.chromium.org/729273002/diff/140001/base/test/scoped_path_o... base/test/scoped_path_override.cc:29: bool result = PathService::OverrideAndCreateIfNeeded(key, path, Why not have the interface of ScopedPathOverride match one of PathService::OverrideAndCreateIfNeeded?
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:160001) has been deleted
Thanks, PTAL. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:46: DCHECK_EQ(!InstallUtil::IsPerUserInstall(chrome_exe_path.value().c_str()), On 2014/11/26 00:51:46, Peter Kasting wrote: > Nit: DCHECK_NE and remove the ! Done. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:92: wchar_t class_id_as_string[MAX_PATH] = {}; On 2014/11/26 00:51:46, Peter Kasting wrote: > Nit: "{ 0 }" is more common to indicate "I'm trying to zero-initialize > everything here", I would leave it here and use it on line 100 as well. I think this is bad practice since one could reasonably figure that "{1}" would mean "one-initialize everything here," but it doesn't. It means "one-initialize the first thing here and then value-initialize the rest." https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:126: // instance. On 2014/11/26 00:51:46, Peter Kasting wrote: > I'm confused. This comment seems to actually be talking about the return > statement on line 130? Done. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:146: GoogleUpdateJobObserver() On 2014/11/26 00:51:46, Peter Kasting wrote: > Nit: While here: Prefer not to inline non-"cheap accessor" class methods even > for classes declared in C++ files. Done. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:312: void UpdateCheckDriver::RunUpdateCheck( On 2014/11/26 00:51:46, Peter Kasting wrote: > It seems like the only reason we need this static method is that the constructor > is private. That's not why I exposed this static method. I did it this way because how and what this class does on |task_runner| is an implementation detail. I think it's bad style for it to expose a function that a caller needs to invoke on some other thread. I prefer a contract that's more like "Hey, do your stuff for me on ->this<- thread here." Rather than "I'm going to create you and then invoke some function of yours on some other thread, which I also want you to use for other stuff you may do." https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:71: namespace chrome { On 2014/11/26 00:51:46, Peter Kasting wrote: > The chrome namespace is deprecated (see http://crbug.com/289619 ); please don't > add anything new to it. Done. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:83: // Initiates a update check on FILE thread. If |install_if_newer| is true, an On 2014/11/26 00:51:47, Peter Kasting wrote: > Nit: a -> an; on -> on the Done. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:84: // update will be applied. |elevation_window| is the window by which elevation On 2014/11/26 00:51:47, Peter Kasting wrote: > Nit: "...the window which should own any necessary elevation UI." Thanks for the suggestion. I didn't like the wording of that before. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:88: HWND elevation_window, // gfx::AcceleratedWidget On 2014/11/26 00:51:47, Peter Kasting wrote: > Why are you using HWNDs and then commenting that they're AcceleratedWidgets? > Why not use AcceleratedWidget here? Ah, that comment was for me. I hadn't meant to leave it there. I debated whether or not to use gfx::AcceleratedWidget instead of HWND, but in this case an HWND is what's used on both sides of the API (consumer and implementation), so I'm inclined to leave it as-is (but without the comment). https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:103: // Initiates a update check on |task_runner|, which must run tasks on a thread On 2014/11/26 00:51:47, Peter Kasting wrote: > Nit: Don't duplicate the comments above here; say how this differs from the > function above and why we need it. Done. https://codereview.chromium.org/729273002/diff/140001/base/test/scoped_path_o... File base/test/scoped_path_override.cc (right): https://codereview.chromium.org/729273002/diff/140001/base/test/scoped_path_o... base/test/scoped_path_override.cc:29: bool result = PathService::OverrideAndCreateIfNeeded(key, path, On 2014/11/26 15:44:36, Paweł Hajdan Jr. wrote: > Why not have the interface of ScopedPathOverride match one of > PathService::OverrideAndCreateIfNeeded? Do you mean add the two additional bool params to this ctor, or replace the ctor on line 19? i'll do the former to avoid the churn of the latter. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:55: LOG(ERROR) << L"Google Update cannot update Chrome installed in a " On 2014/11/26 00:51:47, Peter Kasting wrote: > Do we actually obtain and use this log? If not, remove it; we normally avoid > logging statements unless there's some mechanism to collect their data, since > users don't see them anyway. > > (Same comment applies to the logging statement that got added later on) Hmm. Since there's a nice error message in the UX specifically for this case, I agree that this one isn't needed. Thanks for asking. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:148: result_(UPGRADE_ERROR) {} On 2014/11/26 00:51:48, Peter Kasting wrote: > While here: Prefer not to inline non-"cheap accessor" methods, even for classes > declared in a .cc file. Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:153: void SetOnCompleteCallback(const base::Closure& on_complete_callback) { On 2014/11/26 00:51:47, Peter Kasting wrote: > Nit: Cheap accessors that get inlined should be named unix_hacker()-style. Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:160: DCHECK(result_ != UPGRADE_STARTED && result_ != UPGRADE_CHECK_STARTED); On 2014/11/26 00:51:48, Peter Kasting wrote: > Nit: Use two DCHECK_NEs Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:247: }; On 2014/11/26 00:51:47, Peter Kasting wrote: > Nit: While here: DISALLOW_COPY_AND_ASSIGN (both classes) Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:336: // Runs on the caller's thread. On 2014/11/26 00:51:48, Peter Kasting wrote: > Nit: Is this comment necessary, what with the first DCHECK below? Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:340: DCHECK_EQ(result_ == UPGRADE_ERROR, error_code_ != GOOGLE_UPDATE_NO_ERROR); On 2014/11/26 00:51:48, Peter Kasting wrote: > Nit: I would find this slightly easier to understand if it was DCHECK_NE and != > was changed to ==. Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:382: HRESULT hr = S_OK; On 2014/11/26 00:51:47, Peter Kasting wrote: > Nit: Just init |hr| directly to the result of CreateInstance(). Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:406: if (!install_if_newer) On 2014/11/26 00:51:48, Peter Kasting wrote: > Nit: Remove ! and switch arms Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:411: if (hr != S_OK) { On 2014/11/26 00:51:48, Peter Kasting wrote: > Nit: Shorter: > > if (hr == S_OK) > return true; > OnUpgradeError(GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR, hr, system_level); > return false; I had considered that, but decided not to so that all calls to OnUpgradeError followed the exact same pattern. Do you feel strongly? https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:428: on_demand_ = nullptr; On 2014/11/26 00:51:48, Peter Kasting wrote: > Don't these need to .Release(), rather than just being set to null? No. on_demand_ and job_holder_ are ScopedComPtr, which call Release in their dtors. job_observer_ is a raw pointer and doesn't hold a reference (the reference is owned by job_holder_). https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:451: void CheckForUpdate(bool install_if_newer, On 2014/11/26 00:51:48, Peter Kasting wrote: > I don't think this wrapper buys us very much. AFAICT there are only two > callers, both in version_updater_win.cc, which pass the same second and third > args. > > I think I would eliminate this function and expose the one below in the header > file; then in version_updater_win.cc, optionally write a wrapper that takes only > a bool arg and makes the four-arg call. This puts the responsibility of picking a thread that has CoInitialized properly on the caller. I think it's cleaner as-is. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:456: content::BrowserThread::FILE)); On 2014/11/26 00:51:48, Peter Kasting wrote: > Note that technically you don't need to declare a temp for this (you could just > inline it in the call). The lifetime will be extended until the call returns. Of course. I did this for clarity so that the CheckForUpdate call wouldn't be ginormous. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win_unittest.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win_unittest.cc:256: }; On 2014/11/26 00:51:48, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN (both classes) Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win_unittest.cc:385: // Test that an update check failes with the proper error code if Chrome isn't On 2014/11/26 00:51:48, Peter Kasting wrote: > Nit: fails Done.
The Release() issue is the only substantive blocker below, everything else is style debates. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:92: wchar_t class_id_as_string[MAX_PATH] = {}; On 2014/11/26 21:40:41, grt wrote: > On 2014/11/26 00:51:46, Peter Kasting wrote: > > Nit: "{ 0 }" is more common to indicate "I'm trying to zero-initialize > > everything here", I would leave it here and use it on line 100 as well. > > I think this is bad practice since one could reasonably figure that "{1}" would > mean "one-initialize everything here," but it doesn't. It means "one-initialize > the first thing here and then value-initialize the rest." One could figure that, but I doubt one does, considering how longstanding this idiom is in the C++ world (decades) and how widespread it is across our codebase. (I've literally never reviewed a change in Chromium before that uses "{}" for this.) I agree that from an absolute perspective "{}" is clearer about "value-initialize everything, but on the other hand "{ 0 }" seems much clearer to me about "hey, I didn't forget the initializers here, typo them, or have them removed by a bumbling maintainer later -- I intended to leave this empty and value-initialize". And I think that latter is more important. I'm not going to block the CL for this; it's up to you. But I think you should change it. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:312: void UpdateCheckDriver::RunUpdateCheck( On 2014/11/26 21:40:41, grt wrote: > On 2014/11/26 00:51:46, Peter Kasting wrote: > > It seems like the only reason we need this static method is that the > constructor > > is private. > > That's not why I exposed this static method. I did it this way because how and > what this class does on |task_runner| is an implementation detail. That's true, but this whole file is implementation detail. This class is the means by which the mechanisms in the header are implemented, and it seems unnecessary (and more verbose and confusing) to me to try and hide the implementation details of this class from the caller in this same .cc file. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:88: HWND elevation_window, // gfx::AcceleratedWidget On 2014/11/26 21:40:41, grt wrote: > On 2014/11/26 00:51:47, Peter Kasting wrote: > > Why are you using HWNDs and then commenting that they're AcceleratedWidgets? > > Why not use AcceleratedWidget here? > > Ah, that comment was for me. I hadn't meant to leave it there. I debated whether > or not to use gfx::AcceleratedWidget instead of HWND, but in this case an HWND > is what's used on both sides of the API (consumer and implementation), so I'm > inclined to leave it as-is (but without the comment). Can we use gfx::AcceleratedWidget everywhere? I am reluctantly OK with Windows-specific types in .cc files but would really prefer not to have them in headers if possible (even _win.h ones). Using AcceleratedWidget everywhere seems like one way to accomplish this. It's not the end of the world if this isn't possible. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:428: on_demand_ = nullptr; On 2014/11/26 21:40:41, grt wrote: > On 2014/11/26 00:51:48, Peter Kasting wrote: > > Don't these need to .Release(), rather than just being set to null? > > No. on_demand_ and job_holder_ are ScopedComPtr, which call Release in their > dtors. But we're nulling them before the dtors run. They don't call Release() in that case. That's why I'm worried. > job_observer_ is a raw pointer and doesn't hold a reference (the > reference is owned by job_holder_). Ah, good, I overlooked that for that one. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:451: void CheckForUpdate(bool install_if_newer, On 2014/11/26 21:40:41, grt wrote: > On 2014/11/26 00:51:48, Peter Kasting wrote: > > I don't think this wrapper buys us very much. AFAICT there are only two > > callers, both in version_updater_win.cc, which pass the same second and third > > args. > > > > I think I would eliminate this function and expose the one below in the header > > file; then in version_updater_win.cc, optionally write a wrapper that takes > only > > a bool arg and makes the four-arg call. > > This puts the responsibility of picking a thread that has CoInitialized properly > on the caller. I think it's cleaner as-is. Yes, but there's only really one non-test caller. It seems reasonable to simply say in the header that non-test callers should pass in the file thread. Really, basically all threads in Chrome have been set up correctly for COM (the default low-level initialization code for Chrome threads does so unless you ask it not to), so it's not likely a caller would violate this constraint anyway, even if they passed in another thread. Most critical here, though, is that it was very confusing to me as a reader to have two methods in the header with the exact same name and similar args but in different namespaces. The redundancy made it harder to tell e.g. which one was being called in the test files. And it makes this file longer and harder to scan. It's not always true, but I usually find that shorter code trumps everything else for readability, and I think so here as well. If you still violently disagree, then as a fallback, I think at the least these functions should have different names, e.g. "CheckForUpdateOnFileThread" versus "CheckForUpdateUsingTaskRunner" or something. It's not as good as combining IMO but it would help. https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:135: class GoogleUpdateJobObserver : public CComObjectRootEx<CComSingleThreadModel>, Nit: Because this file has several classes in it, consider putting divider and blank lines between things to improve scannability, a la https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut... . https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:155: DCHECK_NE(result_, UPGRADE_CHECK_STARTED); Nit: (expected, actual) for DCHECK_NE/EQ https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:370: DCHECK_NE(result_ != UPGRADE_ERROR, error_code_ != GOOGLE_UPDATE_NO_ERROR); Nit: Please flip the "!="s to "=="s ... trying to do "!= doesn't equal !=" in my head boggles my mind. https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:89: namespace internal { We also shouldn't be using an ad-hoc "internal" namespace when we're outside the chrome namespace. (Note that if we combine the two CheckForUpdate() methods as I want to do, the primary reason for this namespace disappears anyway.) https://codereview.chromium.org/729273002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/729273002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_win.cc:133: ::CheckForUpdate(false, GetElevationParent(), Nit: "::" shouldn't be necessary here, I wouldn't think, since argument-dependent lookup should find the right method? If it's not, I'd avoid it; we generally try to either avoid "::" entirely or else use it only for system APIs (e.g. win32), though usage is admittedly inconsistent.
https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:428: on_demand_ = nullptr; On 2014/11/26 22:13:04, Peter Kasting wrote: > On 2014/11/26 21:40:41, grt wrote: > > On 2014/11/26 00:51:48, Peter Kasting wrote: > > > Don't these need to .Release(), rather than just being set to null? > > > > No. on_demand_ and job_holder_ are ScopedComPtr, which call Release in their > > dtors. > > But we're nulling them before the dtors run. They don't call Release() in that > case. That's why I'm worried. Your worry is well placed. In this case I meant to say that operator= also does what you would expect: it releases the held ref. See scoped_refptr to confirm. > > > job_observer_ is a raw pointer and doesn't hold a reference (the > > reference is owned by job_holder_). > > Ah, good, I overlooked that for that one.
LGTM, please do consider my feedback in the last mail + this one. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:428: on_demand_ = nullptr; On 2014/11/26 23:45:47, grt wrote: > On 2014/11/26 22:13:04, Peter Kasting wrote: > > On 2014/11/26 21:40:41, grt wrote: > > > On 2014/11/26 00:51:48, Peter Kasting wrote: > > > > Don't these need to .Release(), rather than just being set to null? > > > > > > No. on_demand_ and job_holder_ are ScopedComPtr, which call Release in their > > > dtors. > > > > But we're nulling them before the dtors run. They don't call Release() in > that > > case. That's why I'm worried. > > Your worry is well placed. In this case I meant to say that operator= also does > what you would expect: it releases the held ref. See scoped_refptr to confirm. The short summary is that I was mistakenly thinking that yes, it released the held ref, but somehow that didn't actually call the underlying COM Release() method on the pointer, it just freed the memory. That was wrong, though. Your code is fine. Consider explicitly writing ".Release()" instead of "= nullptr" for the two non-raw objects here. I think that would make things clearer to readers. (It would also obviate the need for the comment.)
Thanks for the comments, Peter. I'm going to wait to land until I have a chance to get your thoughts about using a dedicated thread. Happy turkey day. https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:92: wchar_t class_id_as_string[MAX_PATH] = {}; On 2014/11/26 22:13:04, Peter Kasting wrote: > On 2014/11/26 21:40:41, grt wrote: > > On 2014/11/26 00:51:46, Peter Kasting wrote: > > > Nit: "{ 0 }" is more common to indicate "I'm trying to zero-initialize > > > everything here", I would leave it here and use it on line 100 as well. > > > > I think this is bad practice since one could reasonably figure that "{1}" > would > > mean "one-initialize everything here," but it doesn't. It means > "one-initialize > > the first thing here and then value-initialize the rest." > > One could figure that, but I doubt one does, considering how longstanding this > idiom is in the C++ world (decades) and how widespread it is across our > codebase. (I've literally never reviewed a change in Chromium before that uses > "{}" for this.) While your style is more prevalent in the codebase, over 25% are in my style based on the query I just ran. I assure you that I didn't write them all. I really, really don't like {0}. I also wish I had a time machine. :-) https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/729273002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:88: HWND elevation_window, // gfx::AcceleratedWidget On 2014/11/26 22:13:04, Peter Kasting wrote: > On 2014/11/26 21:40:41, grt wrote: > > On 2014/11/26 00:51:47, Peter Kasting wrote: > > > Why are you using HWNDs and then commenting that they're AcceleratedWidgets? > > > > Why not use AcceleratedWidget here? > > > > Ah, that comment was for me. I hadn't meant to leave it there. I debated > whether > > or not to use gfx::AcceleratedWidget instead of HWND, but in this case an HWND > > is what's used on both sides of the API (consumer and implementation), so I'm > > inclined to leave it as-is (but without the comment). > > Can we use gfx::AcceleratedWidget everywhere? I am reluctantly OK with > Windows-specific types in .cc files but would really prefer not to have them in > headers if possible (even _win.h ones). Using AcceleratedWidget everywhere > seems like one way to accomplish this. > > It's not the end of the world if this isn't possible. Done. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:428: on_demand_ = nullptr; On 2014/11/27 00:28:51, Peter Kasting wrote: > On 2014/11/26 23:45:47, grt wrote: > > On 2014/11/26 22:13:04, Peter Kasting wrote: > > > On 2014/11/26 21:40:41, grt wrote: > > > > On 2014/11/26 00:51:48, Peter Kasting wrote: > > > > > Don't these need to .Release(), rather than just being set to null? > > > > > > > > No. on_demand_ and job_holder_ are ScopedComPtr, which call Release in > their > > > > dtors. > > > > > > But we're nulling them before the dtors run. They don't call Release() in > > that > > > case. That's why I'm worried. > > > > Your worry is well placed. In this case I meant to say that operator= also > does > > what you would expect: it releases the held ref. See scoped_refptr to confirm. > > The short summary is that I was mistakenly thinking that yes, it released the > held ref, but somehow that didn't actually call the underlying COM Release() > method on the pointer, it just freed the memory. That was wrong, though. Your > code is fine. > > Consider explicitly writing ".Release()" instead of "= nullptr" for the two > non-raw objects here. I think that would make things clearer to readers. (It > would also obviate the need for the comment.) That makes sense. I've switched to Release() and expanded the comment since a future reader may wonder why this is done here just before the call to DeleteSoon: // Release the reference on the COM objects before bouncing back to the // caller's thread. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:451: void CheckForUpdate(bool install_if_newer, On 2014/11/26 22:13:04, Peter Kasting wrote: > On 2014/11/26 21:40:41, grt wrote: > > On 2014/11/26 00:51:48, Peter Kasting wrote: > > > I don't think this wrapper buys us very much. AFAICT there are only two > > > callers, both in version_updater_win.cc, which pass the same second and > third > > > args. > > > > > > I think I would eliminate this function and expose the one below in the > header > > > file; then in version_updater_win.cc, optionally write a wrapper that takes > > only > > > a bool arg and makes the four-arg call. > > > > This puts the responsibility of picking a thread that has CoInitialized > properly > > on the caller. I think it's cleaner as-is. > > Yes, but there's only really one non-test caller. It seems reasonable to simply > say in the header that non-test callers should pass in the file thread. Really, > basically all threads in Chrome have been set up correctly for COM (the default > low-level initialization code for Chrome threads does so unless you ask it not > to), so it's not likely a caller would violate this constraint anyway, even if > they passed in another thread. Ah, now that I've tried it out again I remember the other reason why any-old-thread isn't good enough: the thread needs to pump windows messages. I'm glad you've asked about this more deeply, since you prodded me into looking deeper into why the FILE thread runs a TYPE_UI message loop, whereas the threads in the blocking pool (which I'd wanted to use) don't. Surprise, surprise: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br.... According to that comment, the FILE thread runs a UI message loop precisely because of the code I'm updating here. This brings me back to one of the ideas I had from the get-go, which was that perhaps this update checker should just run its own private thread. This would allow for the removal of what appears to be a Chrome-specific hack in content/. Of course, for all I know, there's now a whole lot of code that assumes the FILE thread runs a UI message loop. What do you think about a dedicated thread here? https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:135: class GoogleUpdateJobObserver : public CComObjectRootEx<CComSingleThreadModel>, On 2014/11/26 22:13:05, Peter Kasting wrote: > Nit: Because this file has several classes in it, consider putting divider and > blank lines between things to improve scannability, a la > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut... > . git cl format doesn't like the extra blank line. :-( https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:155: DCHECK_NE(result_, UPGRADE_CHECK_STARTED); On 2014/11/26 22:13:05, Peter Kasting wrote: > Nit: (expected, actual) for DCHECK_NE/EQ Is this for consistency with Google Tests's EXPECT_EQ and ASSERT_EQ (but not _NE) macros? I find this confusing since if (expected == actual) is frowned upon in Chromium (e.g., "if (5 == foo)"). Grumble. https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:370: DCHECK_NE(result_ != UPGRADE_ERROR, error_code_ != GOOGLE_UPDATE_NO_ERROR); On 2014/11/26 22:13:05, Peter Kasting wrote: > Nit: Please flip the "!="s to "=="s ... trying to do "!= doesn't equal !=" in my > head boggles my mind. Oops. I mis-read your previous comment. I wondered why you preferred that strange negation. :-) https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:89: namespace internal { On 2014/11/26 22:13:05, Peter Kasting wrote: > We also shouldn't be using an ad-hoc "internal" namespace when we're outside the > chrome namespace. I don't understand this suggestion, nor why it seems ad-hoc to you. The factory type and factory setter are also exposed only for testing. A consumer of the function above has no need to see any of this. The options I can think of are: 1. Use the internal namespace as here. 2. Put "Internal" in the names of these things (e.g., CheckForUpdateInternal). 3. Put "for testing only" or something similar in a doc comment. 4. Introduce a new "google_update_internal_win.h" header with these declarations. #1 is done a fair amount throughout the codebase (in some cases mixed with #4). It seems to very clearly group multiple declarations/definitions together in a "don't touch this" sort of way. I find #2 uglier, especially if it's for multiple names. #3 is too easy to miss. I like #4, but it seems a bit heavy-weight for this. > (Note that if we combine the two CheckForUpdate() methods as > I want to do, the primary reason for this namespace disappears anyway.) https://codereview.chromium.org/729273002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/729273002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_win.cc:133: ::CheckForUpdate(false, GetElevationParent(), On 2014/11/26 22:13:05, Peter Kasting wrote: > Nit: "::" shouldn't be necessary here, I wouldn't think, since > argument-dependent lookup should find the right method? If it's not, I'd avoid > it; we generally try to either avoid "::" entirely or else use it only for > system APIs (e.g. win32), though usage is admittedly inconsistent. ADL wasn't enough to find it. I've renamed the function in google_update_win to avoid the collision.
https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:451: void CheckForUpdate(bool install_if_newer, On 2014/11/27 04:44:56, grt wrote: > On 2014/11/26 22:13:04, Peter Kasting wrote: > > On 2014/11/26 21:40:41, grt wrote: > > > On 2014/11/26 00:51:48, Peter Kasting wrote: > > > > I don't think this wrapper buys us very much. AFAICT there are only two > > > > callers, both in version_updater_win.cc, which pass the same second and > > third > > > > args. > > > > > > > > I think I would eliminate this function and expose the one below in the > > header > > > > file; then in version_updater_win.cc, optionally write a wrapper that > takes > > > only > > > > a bool arg and makes the four-arg call. > > > > > > This puts the responsibility of picking a thread that has CoInitialized > > properly > > > on the caller. I think it's cleaner as-is. > > > > Yes, but there's only really one non-test caller. It seems reasonable to > simply > > say in the header that non-test callers should pass in the file thread. > Really, > > basically all threads in Chrome have been set up correctly for COM (the > default > > low-level initialization code for Chrome threads does so unless you ask it not > > to), so it's not likely a caller would violate this constraint anyway, even if > > they passed in another thread. > > Ah, now that I've tried it out again I remember the other reason why > any-old-thread isn't good enough: the thread needs to pump windows messages. I'm > glad you've asked about this more deeply, since you prodded me into looking > deeper into why the FILE thread runs a TYPE_UI message loop, whereas the threads > in the blocking pool (which I'd wanted to use) don't. Surprise, surprise: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br.... > According to that comment, the FILE thread runs a UI message loop precisely > because of the code I'm updating here. The SequencedWorkerPool threads (which is what you were referring to, right?) don't support MessageLoop at all, I don't think, so it wouldn't be trivial to make them use this. It seems like this relates to https://codereview.chromium.org/242513006/ . I believe I looked into making them support message loops at the time and couldn't figure it out. Most of our threads that DO run loops run TYPE_UI loops, though -- I think that came about as a result of https://codereview.chromium.org/11048029/ . If it's possible to easily make the blocking pool threads run message loops, it might be worth simply doing that. > This brings me back to one of the ideas I had from the get-go, which was that > perhaps this update checker should just run its own private thread. That would be OK, but I'd only do it if it's infeasible to make the blocking pool usable. We have sort of an excess of different threads at this point and adding yet another one isn't supremely exciting. I also don't think it's bad to use the FILE thread for this, but I don't have a very informed opinion about that. Maybe there's a reason that's potentially bad for perf. If not, I would probably take using the FILE thread over using a dedicated thread. It's simpler. > This would > allow for the removal of what appears to be a Chrome-specific hack in content/. > Of course, for all I know, there's now a whole lot of code that assumes the FILE > thread runs a UI message loop. What do you think about a dedicated thread here? I would probably not try to make the FILE thread run a non-UI loop. As I said, most of our threads to that at this point to my knowledge, and I don't think having the FILE thread do so is a layering violation or a hack. I _would_ remove the comment about Google Update and change the content layer to just unilaterally running a UI loop for this. I think it's dangerous for that thread's loop type to differ per platform regardless of what else we do. https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:135: class GoogleUpdateJobObserver : public CComObjectRootEx<CComSingleThreadModel>, On 2014/11/27 04:44:57, grt wrote: > On 2014/11/26 22:13:05, Peter Kasting wrote: > > Nit: Because this file has several classes in it, consider putting divider and > > blank lines between things to improve scannability, a la > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut... > > . > > git cl format doesn't like the extra blank line. :-( I prefer that people not run git cl format at all. (An opinion that seems to be shared by multiple other Google readability reviewers, in the last thread where we discussed this...) If you do run it, take its output as an option, not as the only option. Readability is ultimately up to the author, git cl format is only a tool to try and help. https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:155: DCHECK_NE(result_, UPGRADE_CHECK_STARTED); On 2014/11/27 04:44:57, grt wrote: > On 2014/11/26 22:13:05, Peter Kasting wrote: > > Nit: (expected, actual) for DCHECK_NE/EQ > > Is this for consistency with Google Tests's EXPECT_EQ and ASSERT_EQ (but not > _NE) macros? I find this confusing since > if (expected == actual) > is frowned upon in Chromium (e.g., "if (5 == foo)"). Grumble. Yes, that's why we instituted that practice. (We also suggest people use (expected, actual) on EXPECT/ASSERT_NE as well, but the reverse on the other inequalities.) Practically every option is inconsistent in some way; personally I wish the gtest people had used (actual, expected) in all their macros and this never would have come up. But oh well. https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g... chrome/browser/google/google_update_win.h:89: namespace internal { On 2014/11/27 04:44:57, grt wrote: > On 2014/11/26 22:13:05, Peter Kasting wrote: > > We also shouldn't be using an ad-hoc "internal" namespace when we're outside > the > > chrome namespace. > > I don't understand this suggestion, nor why it seems ad-hoc to you. Namespace usage in Chrome has been discussed on the mailing lists several times, not always clearly, and the general conclusion has been that we mostly ban namespaces that don't correspond to top-level modules. This is what led to e.g. the "chrome::" namespace being banned. If other code in Chrome is using "internal" namespaces, that code is likely wrong per those discussions, and should be fixed. It's not a big enough deal to go around trying to wipe it out, but I don't want to introduce more cases. > The factory > type and factory setter are also exposed only for testing. A consumer of the > function above has no need to see any of this. As I've said several times, I really really really think the best way to go here is to combine the two CheckForUpdate() methods. At that point you're down to one remaining method that's only needed by tests. I don't think it's terrible for that to just live alongside the other lone method in this file. > The options I can think of are: > > 1. Use the internal namespace as here. See above. Plus, "internal" is meaningless. It doesn't mean "for testing", it just means "don't touch me". We can do better than that if we really want to tell callers something about this stuff. > 2. Put "Internal" in the names of these things (e.g., CheckForUpdateInternal). I suggested something like this as a fallback in another comment, but with a better name. > 3. Put "for testing only" or something similar in a doc comment. You already do this (and IMO it's sufficient) > 4. Introduce a new "google_update_internal_win.h" header with these > declarations. Too heavyweight. There's a general theme I'm trying to convey in this .h and .cc: I think you're overdesigning. You're introducing too many methods, too many namespaces, too many abstractions. Yes, all of those abstractions separate implementations from each other and black box things and so forth, but this code is simple enough that all that rigor is not really necessary for maintainability, and at that point it just gets in the way of readability and simplicity. Please, just make these files as short and simple as possible. In a case like this that pays off more than any of the other benefits.
base/test LGTM
I've simplified things a bit as you've requested. I also moved the base/test changes into their own CL since I have another change that needs them. Please have another (maybe final?) look. Thanks. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:451: void CheckForUpdate(bool install_if_newer, On 2014/11/27 08:07:34, Peter Kasting wrote: > On 2014/11/27 04:44:56, grt wrote: > > On 2014/11/26 22:13:04, Peter Kasting wrote: > > > On 2014/11/26 21:40:41, grt wrote: > > > > On 2014/11/26 00:51:48, Peter Kasting wrote: > > > > > I don't think this wrapper buys us very much. AFAICT there are only two > > > > > callers, both in version_updater_win.cc, which pass the same second and > > > third > > > > > args. > > > > > > > > > > I think I would eliminate this function and expose the one below in the > > > header > > > > > file; then in version_updater_win.cc, optionally write a wrapper that > > takes > > > > only > > > > > a bool arg and makes the four-arg call. > > > > > > > > This puts the responsibility of picking a thread that has CoInitialized > > > properly > > > > on the caller. I think it's cleaner as-is. > > > > > > Yes, but there's only really one non-test caller. It seems reasonable to > > simply > > > say in the header that non-test callers should pass in the file thread. > > Really, > > > basically all threads in Chrome have been set up correctly for COM (the > > default > > > low-level initialization code for Chrome threads does so unless you ask it > not > > > to), so it's not likely a caller would violate this constraint anyway, even > if > > > they passed in another thread. > > > > Ah, now that I've tried it out again I remember the other reason why > > any-old-thread isn't good enough: the thread needs to pump windows messages. > I'm > > glad you've asked about this more deeply, since you prodded me into looking > > deeper into why the FILE thread runs a TYPE_UI message loop, whereas the > threads > > in the blocking pool (which I'd wanted to use) don't. Surprise, surprise: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br.... > > According to that comment, the FILE thread runs a UI message loop precisely > > because of the code I'm updating here. > > The SequencedWorkerPool threads (which is what you were referring to, right?) > don't support MessageLoop at all, I don't think, so it wouldn't be trivial to > make them use this. It seems like this relates to > https://codereview.chromium.org/242513006/ . I believe I looked into making > them support message loops at the time and couldn't figure it out. > > Most of our threads that DO run loops run TYPE_UI loops, though -- I think that > came about as a result of https://codereview.chromium.org/11048029/ . > > If it's possible to easily make the blocking pool threads run message loops, it > might be worth simply doing that. > > > This brings me back to one of the ideas I had from the get-go, which was that > > perhaps this update checker should just run its own private thread. > > That would be OK, but I'd only do it if it's infeasible to make the blocking > pool usable. We have sort of an excess of different threads at this point and > adding yet another one isn't supremely exciting. > > I also don't think it's bad to use the FILE thread for this, but I don't have a > very informed opinion about that. Maybe there's a reason that's potentially bad > for perf. If not, I would probably take using the FILE thread over using a > dedicated thread. It's simpler. > > > This would > > allow for the removal of what appears to be a Chrome-specific hack in > content/. > > Of course, for all I know, there's now a whole lot of code that assumes the > FILE > > thread runs a UI message loop. What do you think about a dedicated thread > here? > > I would probably not try to make the FILE thread run a non-UI loop. As I said, > most of our threads to that at this point to my knowledge, and I don't think > having the FILE thread do so is a layering violation or a hack. I _would_ > remove the comment about Google Update and change the content layer to just > unilaterally running a UI loop for this. I think it's dangerous for that > thread's loop type to differ per platform regardless of what else we do. My interest in getting this work off of the FILE thread was: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OaA-9rnjOqI/H0cDC.... I'll leave it as-is for now since it's at least straightforward. I didn't realize that the threads in the blocking pool (yes, SequencedWorkerPool) didn't run message loops at all. That would make this use of COM problematic. Thanks for the pointers.
LGTM https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:411: if (hr != S_OK) { On 2014/11/26 21:40:41, grt wrote: > On 2014/11/26 00:51:48, Peter Kasting wrote: > > Nit: Shorter: > > > > if (hr == S_OK) > > return true; > > OnUpgradeError(GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR, hr, > system_level); > > return false; > > I had considered that, but decided not to so that all calls to OnUpgradeError > followed the exact same pattern. Do you feel strongly? I failed to reply to this! No, I don't feel strongly. I don't think there's any benefit in making them follow the same pattern, but if you prefer it I don't mind. https://codereview.chromium.org/729273002/diff/250001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/250001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:44: DCHECK_NE(InstallUtil::IsPerUserInstall(chrome_exe_path.value().c_str()), Bonus points if in a separate CL you make this take a string or FilePath or something instead of a wchar_t*. https://codereview.chromium.org/729273002/diff/250001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:470: // Release the reference on the COM objects before bouncing back to the Nit: Blank line above? https://codereview.chromium.org/729273002/diff/250001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:496: // Public API------------------------------------------------------------------- Nit: I would probably just call this "Globals" and have it encompass both functions below, but there's no formal rule. Space before "---" though.
Thanks. https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/140001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:411: if (hr != S_OK) { On 2014/12/02 00:56:39, Peter Kasting wrote: > On 2014/11/26 21:40:41, grt wrote: > > On 2014/11/26 00:51:48, Peter Kasting wrote: > > > Nit: Shorter: > > > > > > if (hr == S_OK) > > > return true; > > > OnUpgradeError(GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR, hr, > > system_level); > > > return false; > > > > I had considered that, but decided not to so that all calls to OnUpgradeError > > followed the exact same pattern. Do you feel strongly? > > I failed to reply to this! No, I don't feel strongly. I don't think there's > any benefit in making them follow the same pattern, but if you prefer it I don't > mind. The benefit I see has to do with human pattern recognition. I think it's easier to see that all calls to OnUpgradeError follow the exact same pattern, and (by extension) that any future additions should do the same. I can understand the "code is a little tighter otherwise" argument, but I'm drawn toward consistency over compactness here. https://codereview.chromium.org/729273002/diff/250001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/729273002/diff/250001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:44: DCHECK_NE(InstallUtil::IsPerUserInstall(chrome_exe_path.value().c_str()), On 2014/12/02 00:56:40, Peter Kasting wrote: > Bonus points if in a separate CL you make this take a string or FilePath or > something instead of a wchar_t*. Acknowledged. That is one ugly line of code there. https://codereview.chromium.org/729273002/diff/250001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:470: // Release the reference on the COM objects before bouncing back to the On 2014/12/02 00:56:40, Peter Kasting wrote: > Nit: Blank line above? Done. https://codereview.chromium.org/729273002/diff/250001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:496: // Public API------------------------------------------------------------------- On 2014/12/02 00:56:40, Peter Kasting wrote: > Nit: I would probably just call this "Globals" and have it encompass both > functions below, but there's no formal rule. > > Space before "---" though. Done.
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/729273002/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/729273002/290001
Message was sent while issue was closed.
Committed patchset #9 (id:290001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5a83109b2fe5f328635cc297dc6db67518071dda Cr-Commit-Position: refs/heads/master@{#306421} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
