Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10698106:
Switch about box to web ui on Windows. (Closed)
|
Created:
8 years, 5 months ago by MAD Modified:
8 years, 5 months ago Reviewers:
Daniel Erat, James Hawkins, cpu_(ooo_6.6-7.5), Roger Tawa OOO till Jul 10th, Ben Goodger (Google) CC:
chromium-reviews, Daniel Erat Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSwitch about box to web ui on Windows.
This CLs adds proper support for update check on Windows so that we can turn on the WebUI support and eventually remove the code for about box in a modal dialog.
BUG=115123
TEST=Make sure about works and properly checks for updates.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146738
Patch Set 1 : #
Total comments: 32
Patch Set 2 : Code review comments addressed. #Patch Set 3 : Completed addressing code review comments... #
Total comments: 12
Patch Set 4 : OWNERs review comments addressed... #
Total comments: 4
Patch Set 5 : More OWNERs review comments addressed... #Patch Set 6 : Removed useless thread checks. #Patch Set 7 : Added GetForegroundWindow #
Total comments: 2
Patch Set 8 : With GoogleUpdate on Windows only. #
Total comments: 4
Patch Set 9 : Addressed James comments about comments... #Patch Set 10 : Sync'd to ToT. #
Total comments: 2
Patch Set 11 : Better Getter for elevation parent HWND. #
Total comments: 2
Patch Set 12 : No more window_ data member. #Patch Set 13 : Merged to ToT #Patch Set 14 : Updating a Copyright header goof... #Patch Set 15 : Fixed a merge issue where ChromeOS started using google_update.h again. #
Total comments: 2
Patch Set 16 : Better hack for reusing GOOGLE_UPDATE_ERROR_UPDATING #
Total comments: 1
Patch Set 17 : Using 0 instead #Patch Set 18 : Adapted to install_util API change. #Messages
Total messages: 50 (0 generated)
This new code enables the usage of the Help page webui to show the "About Chrome" info on Windows by taking care of the Windows specific version update check. I manually tested the cases of not needing an update at all and needing an update for both single user or system installs (which requires elevation), and it all works (at least on Windows 8).
Salut MAD, Some comments/questions below. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:43: } This will find any browser object, not necessarily the one that initiated the about box. That's OK? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:45: // We use this class to read the version of the FILE thread and then call back of --> on ? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:49: // the WeakPtr, which can only use from the thread that created it. use --> be used ? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:53: explicit VersionReader(base::WeakPtr<VersionUpdaterWin> version_updater) const ref? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:55: } add DCHECK to make sure we are on UI thread? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:65: &VersionReader::SetVersionInUIThread, this)); if installed_version_ is still null at line 64, is it still worth posting the task? I say this because GotInstalledVersion() will ignore it in that case. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:109: !base::win::UserAccountControlIsEnabled())) { indent line 109 4 more spaces? do you need to check for vista and later OS? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:140: #endif this is strange. why not reverse condition and put the entire method content inside the #ifdef? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:161: google_updater_->set_status_listener(this); lines 160 and 161 are repeated three times (if you include the ctor). Would be better to write a small helper function to do this. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:163: google_updater_->CheckForUpdate(true, GetWidget()); // Upgrade now. is the comment valid for this line ? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:195: } I'm not sure I understand the logic of setting message. Won't the value assigned at line 105 always be overridden by the value assigned at either line 190 or 193? Or is line 189 supposed to be an "else if"? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:200: // TODO(mad): Get proper progress value instead of passing 0. open a bug to track this? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:204: void VersionUpdaterWin::GotInstalledVersion(Version* version) { use const Version* http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:220: callback_.Run(NEARLY_UPDATED, 0, string16()); The callback will be called in the file thread, whereas when it is called at line 201 above it is called in the ui thread. Is that ok? Does the caller make any assumption about which thread it is called in? Would be worth adding to documentation of base class VersionUpdater. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/version_updater_win.h (right): http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.h:22: void GotInstalledVersion(Version* version); Could make this method private by declaring the VersionReader class inside VersionUpdaterWin and making it a friend. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.h:25: friend class VersionUpdater; do friends go in the private section?
Thanks... How about this? BYE MAD... http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:43: } On 2012/07/05 16:13:22, Roger Tawa wrote: > This will find any browser object, not necessarily the one that initiated the > about box. That's OK? Yes, it's OK, all we need is A browser window... This will be needed by the elevation prompt which will take over the whole desktop anyway... http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:45: // We use this class to read the version of the FILE thread and then call back On 2012/07/05 16:13:22, Roger Tawa wrote: > of --> on ? Done. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:49: // the WeakPtr, which can only use from the thread that created it. On 2012/07/05 16:13:22, Roger Tawa wrote: > use --> be used ? Done. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:53: explicit VersionReader(base::WeakPtr<VersionUpdaterWin> version_updater) On 2012/07/05 16:13:22, Roger Tawa wrote: > const ref? Done. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:55: } On 2012/07/05 16:13:22, Roger Tawa wrote: > add DCHECK to make sure we are on UI thread? Actually, we don't really care where this one comes from... The important thing is that we send to FILE and come back to UI... http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:65: &VersionReader::SetVersionInUIThread, this)); On 2012/07/05 16:13:22, Roger Tawa wrote: > if installed_version_ is still null at line 64, is it still worth posting the > task? I say this because GotInstalledVersion() will ignore it in that case. It doesn't actually ignore, it still makes a call to callback_, but use a different status. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:109: !base::win::UserAccountControlIsEnabled())) { On 2012/07/05 16:13:22, Roger Tawa wrote: > indent line 109 4 more spaces? Why? It's at the same logic level as 108, isn't it? > > do you need to check for vista and later OS? Based on the comment above, we are only looking for a specific VISTA version that had that specific problem. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:140: #endif On 2012/07/05 16:13:22, Roger Tawa wrote: > this is strange. why not reverse condition and put the entire method content > inside the #ifdef? Simply copied from: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/ab... http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:161: google_updater_->set_status_listener(this); On 2012/07/05 16:13:22, Roger Tawa wrote: > lines 160 and 161 are repeated three times (if you include the ctor). Would be > better to write a small helper function to do this. Done. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:163: google_updater_->CheckForUpdate(true, GetWidget()); // Upgrade now. On 2012/07/05 16:13:22, Roger Tawa wrote: > is the comment valid for this line ? Yes, this explains the passing of true... Made it more explicit now, OK? http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:195: } On 2012/07/05 16:13:22, Roger Tawa wrote: > I'm not sure I understand the logic of setting message. Won't the value > assigned at line 105 always be overridden by the value assigned at either line > 190 or 193? Or is line 189 supposed to be an "else if"? Good point, I don't know... :-) This was stolen from: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/ab... By someone else, and I guess he tried to merge both messages into a single one... I'll see how I can fix that... http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:200: // TODO(mad): Get proper progress value instead of passing 0. On 2012/07/05 16:13:22, Roger Tawa wrote: > open a bug to track this? Done. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:204: void VersionUpdaterWin::GotInstalledVersion(Version* version) { On 2012/07/05 16:13:22, Roger Tawa wrote: > use const Version* Done. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:220: callback_.Run(NEARLY_UPDATED, 0, string16()); On 2012/07/05 16:13:22, Roger Tawa wrote: > The callback will be called in the file thread, whereas when it is called at > line 201 above it is called in the ui thread. Is that ok? Does the caller make > any assumption about which thread it is called in? Would be worth adding to > documentation of base class VersionUpdater. Here, we should be in the UI thread... The file thread posted back to the UI thread after reading the version data from withih the FILE thread. http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/version_updater_win.h (right): http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.h:22: void GotInstalledVersion(Version* version); On 2012/07/05 16:13:22, Roger Tawa wrote: > Could make this method private by declaring the VersionReader class inside > VersionUpdaterWin and making it a friend. Yeah, I was hesitating... I was wondering which one was the less evil of the two. I chose the public call instead, since nobody include us anyway, which makes me wonder if we actually need this header file... All this code could be in the .cc file and then we could friend the other class... http://codereview.chromium.org/10698106/diff/1014/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.h:25: friend class VersionUpdater; On 2012/07/05 16:13:22, Roger Tawa wrote: > do friends go in the private section? Done.
lgtm
Thanks Roger... Now I need OWNERS approval for browser/ui[/webui]... Ben, I think you were waiting for this to be fixed, so maybe you could OWNER approve it? :-) And James, you started this thing, are you happy with the way it ended? Thanks all! BYE MAD...
I believe you may have forgotten to 'git add' version_updater_basic.[cc,h]. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:32: views::Widget* GetWidget() { Optional nit: Technically you can forward declare Widget instead of including widget.h. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:32: views::Widget* GetWidget() { nit: Document the function, specifically *which* widget is being returned. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:48: class VersionUpdaterWin : public VersionUpdater, nit: Document the class. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:73: // Got the intalled version so we can complete the handling of the nit: Don't use pronouns in comments (we). Pronouns are ambiguous. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:107: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); Optional nit: Technically you can forward declare BrowserDistribution instead of including browser_distribution.h. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:239: #endif nit: #endif // defined(GOOGLE_CHROME_BUILD)
Thanks James... How about this now? For the basic files, they're already there: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/help/... Added over 3 weeks ago... :-) http://codereview.chromium.org/10698106/diff/13002/chrome/browser/ui/webui/he... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://codereview.chromium.org/10698106/diff/13002/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:32: views::Widget* GetWidget() { On 2012/07/06 21:03:44, James Hawkins wrote: > nit: Document the function, specifically *which* widget is being returned. Done. http://codereview.chromium.org/10698106/diff/13002/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:32: views::Widget* GetWidget() { On 2012/07/06 21:03:44, James Hawkins wrote: > Optional nit: Technically you can forward declare Widget instead of > including widget.h. I can't, I use views::Widget::GetWidgetForNativeWindow(). http://codereview.chromium.org/10698106/diff/13002/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:48: class VersionUpdaterWin : public VersionUpdater, On 2012/07/06 21:03:44, James Hawkins wrote: > nit: Document the class. Done. http://codereview.chromium.org/10698106/diff/13002/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:73: // Got the intalled version so we can complete the handling of the On 2012/07/06 21:03:44, James Hawkins wrote: > nit: Don't use pronouns in comments (we). Pronouns are ambiguous. Done. http://codereview.chromium.org/10698106/diff/13002/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:107: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); On 2012/07/06 21:03:44, James Hawkins wrote: > Optional nit: Technically you can forward declare BrowserDistribution instead of > including browser_distribution.h. But then I won't be able to call BrowserDistribution::GetDistribution() http://codereview.chromium.org/10698106/diff/13002/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:239: #endif On 2012/07/06 21:03:44, James Hawkins wrote: > nit: #endif // defined(GOOGLE_CHROME_BUILD) Done.
Can you also remove all the old about box code. http://codereview.chromium.org/10698106/diff/15001/chrome/browser/ui/webui/he... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://codereview.chromium.org/10698106/diff/15001/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:35: views::Widget* GetWidget() { rather than use BrowserList here, since this code is windows-specific, can't you use some win32 method to obtain a hwnd from this process/thread? (e.g. EnumThreadWindows). http://codereview.chromium.org/10698106/diff/15001/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:161: google_updater_->CheckForUpdate(false /* Don't upgrade yet */, GetWidget()); Also, why does CheckForUpdate() take a views::Widget*? That seems sketchy. It should take a gfx::NativeWindow or a raw HWND, if google_updater_ is win32-specific.
Thanks Ben... I removed the need for GetWidget()... As for removing the about box code, I wanted to do it in a separate CL once this one landed, but I can merge it into this one if you prefer... BYE MAD http://codereview.chromium.org/10698106/diff/15001/chrome/browser/ui/webui/he... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://codereview.chromium.org/10698106/diff/15001/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:35: views::Widget* GetWidget() { On 2012/07/09 15:44:57, Ben Goodger (Google) wrote: > rather than use BrowserList here, since this code is windows-specific, can't you > use some win32 method to obtain a hwnd from this process/thread? (e.g. > EnumThreadWindows). Done. http://codereview.chromium.org/10698106/diff/15001/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:161: google_updater_->CheckForUpdate(false /* Don't upgrade yet */, GetWidget()); On 2012/07/09 15:44:57, Ben Goodger (Google) wrote: > Also, why does CheckForUpdate() take a views::Widget*? That seems sketchy. It > should take a gfx::NativeWindow or a raw HWND, if google_updater_ is > win32-specific. I don't know this code too well... My guess is that the API was written in a platform independent way... But you are right, it's only used on Windows... So I changed it to not require a window, since passing NULL to CoCreateInstanceAsAdmin is like calling it with GetActiveWindow()... I'll make sure to test it, otherwise, we can always use GetForegroundWindow from within GoogleUpdate.
Added GetForegroundWindow() instead of NULL when calling CoCreateInstanceAsAdmin() because passing NULL didn't open the elevation prompt in the foreground. I think it's because GetActiveWindow can only get a window from the current thread and we make that call from the FILE thread as opposed to the UI thread. I could also make the call to GetActiveWindow from the UI thread and then send that window to the FILE thread if we don't like the idea of taking any foreground window.
http://codereview.chromium.org/10698106/diff/33001/chrome/browser/google/goog... File chrome/browser/google/google_update.cc (right): http://codereview.chromium.org/10698106/diff/33001/chrome/browser/google/goog... chrome/browser/google/google_update.cc:297: IID_IGoogleUpdate, GetForegroundWindow(), Will this always return the foreground-most window in the current process/thread? What if this function is called when Chrome isn't active? Is that possible?
OK, I'll try the alternative that I initially thought wasn't worth the extra hoop in case we are called in a non-UI thread... http://codereview.chromium.org/10698106/diff/33001/chrome/browser/google/goog... File chrome/browser/google/google_update.cc (right): http://codereview.chromium.org/10698106/diff/33001/chrome/browser/google/goog... chrome/browser/google/google_update.cc:297: IID_IGoogleUpdate, GetForegroundWindow(), On 2012/07/09 22:28:01, Ben Goodger (Google) wrote: > Will this always return the foreground-most window in the current > process/thread? What if this function is called when Chrome isn't active? Is > that possible? Yes, but I think that's OK, it's just that the elevation prompt will be shown on top of that other app... But maybe it's not what we want? And note that this also has a small window of opportunity to have GetForegroundWindow return NULL if we call it as we are losing activation because the user switches to another app at that time... This would have the side effect of having the elevation prompt not be shown in the foreground, which I think is OK.. Although, I tried to confirm that there wouldn't be any other problems with using another process' window as the owner of the elevation prompt, so I'll change the code to use GetActiveWindow() in the UI thread...
On 2012/07/10 00:20:08, MAD wrote: > Yes, but I think that's OK, it's just that the elevation prompt will be shown on > top of that other app... But maybe it's not what we want? > And note that this also has a small window of opportunity to have > GetForegroundWindow return NULL if we call it as we are losing activation > because the user switches to another app at that time... This would have the > side effect of having the elevation prompt not be shown in the foreground, which > I think is OK.. This is mysterious. I've seen apps that do this and they appear mysteriously broken. It's only when I notice the elevation prompt blinking in the task bar that I realize the fix. Seems like we should pass a valid HWND through here, unless you can guarantee GetForegroundWindow() will always return valid non-NULL.
OK, I'm working on a new version where the API now receives an HWND, so the google_update.* files have been renamed google_update_win.*, since they were already only building and used on Windows. There were a few #if defined(OS_WIN) that I removed from the header file since the file is not used elsewhere anymore (it used to be included in chromeos, but not anymore)... So the caller will be responsible to find a proper HWND, and the only caller will now be the version updater for windows, which should be able to get it via GetActiveWindow while running on the UI thread... And Ben, you didn't answer my proposal to only remove the about box in another CL, is this OK with you, or you would rather see the old code removal as part of the same CL? Now that I'm doing much more cleanup that I originally was, I feel I might as well do that too... I wanted to avoid it to keep the CL small, but it's too late for that now... ;-) On Tue, Jul 10, 2012 at 11:10 AM, <ben@chromium.org> wrote: > On 2012/07/10 00:20:08, MAD wrote: > >> Yes, but I think that's OK, it's just that the elevation prompt will be >> shown >> > on > >> top of that other app... But maybe it's not what we want? >> > > And note that this also has a small window of opportunity to have >> GetForegroundWindow return NULL if we call it as we are losing activation >> because the user switches to another app at that time... This would have >> the >> side effect of having the elevation prompt not be shown in the foreground, >> > which > >> I think is OK.. >> > > This is mysterious. I've seen apps that do this and they appear > mysteriously > broken. It's only when I notice the elevation prompt blinking in the task > bar > that I realize the fix. Seems like we should pass a valid HWND through > here, > unless you can guarantee GetForegroundWindow() will always return valid > non-NULL. > > > http://codereview.chromium.**org/10698106/<http://codereview.chromium.org/106... >
How about this now? And James, did I properly addressed/responded to your comments? This new version ensures that the GoogleUpdate class only gets built on Windows, and thus can now receive an HWND directly. This HWND it gets from the webui help page is found by enumerating the visible windows associated to the UI thread (I tried GetActiveWindow and it returned NULL for some reason). This ensures that a proper Chrome window is used, as opposed to the previous GetForegroundWindow which could return windows from other apps. This has the side effect that if the user switches the focus from Chrome to another app, then the UAC will not be shown, it will just flash in the taskbar. Let me know what you think... BYE MAD
LGTM with nits. http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:65: // Make sure we have a valid window handle. nit: Do a find on all 'we' and rewrite those sentences to remove the pronoun. http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:118: scoped_ptr<Version> installed_version_; nit: Document member variables.
Thanks James... Comments updated... Ben? Are you OK with the latest changes? Note that the caveat of not getting the elevation prompt in the foreground when the user switches focus to another app is the behavior we currently have... So unless you want to change that, I think we are OK with this solution now... BYE MAD... http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:65: // Make sure we have a valid window handle. On 2012/07/10 20:52:34, James Hawkins wrote: > nit: Do a find on all 'we' and rewrite those sentences to remove the pronoun. Done. http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10698106/d... chrome/browser/ui/webui/help/version_updater_win.cc:118: scoped_ptr<Version> installed_version_; On 2012/07/10 20:52:34, James Hawkins wrote: > nit: Document member variables. Done.
https://chromiumcodereview.appspot.com/10698106/diff/1019/chrome/browser/ui/w... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://chromiumcodereview.appspot.com/10698106/diff/1019/chrome/browser/ui/w... chrome/browser/ui/webui/help/version_updater_win.cc:153: UpdateStatus(UPGRADE_CHECK_STARTED, GOOGLE_UPDATE_NO_ERROR, string16()); HWND parent = NULL; GetElevationParent(&parent); google_updater_->CheckForUpdater(false, parent);
> HWND parent = NULL; > GetElevationParent(&parent); > google_updater_->CheckForUpdater(false, parent); Or just google_updater_->CheckForUpdater(false, GetElevationParent());
Agreed? Thanks! BYE MAD... http://codereview.chromium.org/10698106/diff/1019/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://codereview.chromium.org/10698106/diff/1019/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_win.cc:153: UpdateStatus(UPGRADE_CHECK_STARTED, GOOGLE_UPDATE_NO_ERROR, string16()); On 2012/07/11 16:11:01, Ben Goodger (Google) wrote: > HWND parent = NULL; > GetElevationParent(&parent); > google_updater_->CheckForUpdater(false, parent); Good idee... Done!
Ping? This is my last day before two weeks of vacation... And then a week in MTV for the Chrome UI Summit! If I can't commit it early today, I'll have to pass the puck to someone else... Thanks! BYE MAD...
http://codereview.chromium.org/10698106/diff/13004/chrome/browser/ui/webui/he... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://codereview.chromium.org/10698106/diff/13004/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:280: if (window_ != NULL) it seems like you don't want this to be a member var. what if this window is hidden or closed or otherwise becomes unsuitable? This function can just always find the relevant window and return it. I don't think this gets called very often.
Thanks! Good point, addressed... Anything else? BYE MAD... http://codereview.chromium.org/10698106/diff/13004/chrome/browser/ui/webui/he... File chrome/browser/ui/webui/help/version_updater_win.cc (right): http://codereview.chromium.org/10698106/diff/13004/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_win.cc:280: if (window_ != NULL) On 2012/07/13 15:04:55, Ben Goodger (Google) wrote: > it seems like you don't want this to be a member var. what if this window is > hidden or closed or otherwise becomes unsuitable? This function can just always > find the relevant window and return it. I don't think this gets called very > often. Done.
lgtm
On 2012/07/13 15:38:37, Ben Goodger (Google) wrote: > lgtm Cool, thanks... CQ'ing! BYE MAD...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/10698106/13010
Failed to apply patch for chrome/browser/google/google_update_win.h: While running patch -p1 --forward --force; patching file chrome/browser/google/google_update_win.h Hunk #1 FAILED at 2. Hunk #2 succeeded at 28 (offset -1 lines). Hunk #3 succeeded at 94 (offset -1 lines). Hunk #4 succeeded at 106 (offset -1 lines). Hunk #5 succeeded at 138 (offset -1 lines). 1 out of 5 hunks FAILED -- saving rejects to file chrome/browser/google/google_update_win.h.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/10698106/11018
Presubmit check for 10698106-11018 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** License must match: .*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n Found a bad license header in these files: chrome/browser/ui/webui/help/version_updater_win.cc Presubmit checks took 1.3s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/10698106/2037
Try job failure for 10698106-2037 (retry) on linux_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/10698106/2037
Try job failure for 10698106-2037 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Darn... I renamed google_update.* to google_update_win.* because it's only used on windows, but a new dependency has just been added in ChromeOS for a single value: GOOGLE_UPDATE_ERROR_UPDATING... :-( Could we do away without this inclusion? Since ChromeOS doesn't use GoogleUpdate anymore, can it get this value from somewhere else? On Fri, Jul 13, 2012 at 1:42 PM, <commit-bot@chromium.org> wrote: > Try job failure for 10698106-2037 (retry) on linux_chromeos for step > "compile" > (clobber build). > It's a second try, previously, step "compile" failed. > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=linux_**chromeos&number=26624<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=26624> > > > https://chromiumcodereview.**appspot.com/10698106/<https://chromiumcodereview... >
I just noticed there is a TODO above this line... Maybe having a temporary solution to use the hard coded number as opposed to the enum would be OK? James? Would you be OK if I had that change to the CL? On Fri, Jul 13, 2012 at 2:48 PM, Marc-Andre Decoste <mad@chromium.org>wrote: > Darn... I renamed google_update.* to google_update_win.* because it's only > used on windows, but a new dependency has just been added in ChromeOS for a > single value: GOOGLE_UPDATE_ERROR_UPDATING... :-( > > Could we do away without this inclusion? Since ChromeOS doesn't use > GoogleUpdate anymore, can it get this value from somewhere else? > > On Fri, Jul 13, 2012 at 1:42 PM, <commit-bot@chromium.org> wrote: > >> Try job failure for 10698106-2037 (retry) on linux_chromeos for step >> "compile" >> (clobber build). >> It's a second try, previously, step "compile" failed. >> http://build.chromium.org/p/**tryserver.chromium/** >> buildstatus?builder=linux_**chromeos&number=26624<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=26624> >> >> >> https://chromiumcodereview.**appspot.com/10698106/<https://chromiumcodereview... >> > >
Oups... Adding proper committer who made the change now... * derat@chromium.org* For some reason, I th On Fri, Jul 13, 2012 at 2:52 PM, Marc-Andre Decoste <mad@chromium.org>wrote: > I just noticed there is a TODO above this line... > Maybe having a temporary solution to use the hard coded number as opposed > to the enum would be OK? > > James? Would you be OK if I had that change to the CL? > > > On Fri, Jul 13, 2012 at 2:48 PM, Marc-Andre Decoste <mad@chromium.org>wrote: > >> Darn... I renamed google_update.* to google_update_win.* because it's >> only used on windows, but a new dependency has just been added in ChromeOS >> for a single value: GOOGLE_UPDATE_ERROR_UPDATING... :-( >> >> Could we do away without this inclusion? Since ChromeOS doesn't use >> GoogleUpdate anymore, can it get this value from somewhere else? >> >> On Fri, Jul 13, 2012 at 1:42 PM, <commit-bot@chromium.org> wrote: >> >>> Try job failure for 10698106-2037 (retry) on linux_chromeos for step >>> "compile" >>> (clobber build). >>> It's a second try, previously, step "compile" failed. >>> http://build.chromium.org/p/**tryserver.chromium/** >>> buildstatus?builder=linux_**chromeos&number=26624<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=26624> >>> >>> >>> https://chromiumcodereview.**appspot.com/10698106/<https://chromiumcodereview... >>> >> >> >
Here's the change James, is this OK with you? CC+ derat@ Thanks! BYE MAD...
Take 3... with the proper .org (as opposed to .orgm) now... :-/ On Fri, Jul 13, 2012 at 2:58 PM, Marc-Andre Decoste <mad@google.com> wrote: > Oups... Adding proper committer who made the change now... * > derat@chromium.org* > > For some reason, I th > > > On Fri, Jul 13, 2012 at 2:52 PM, Marc-Andre Decoste <mad@chromium.org>wrote: > >> I just noticed there is a TODO above this line... >> Maybe having a temporary solution to use the hard coded number as opposed >> to the enum would be OK? >> >> James? Would you be OK if I had that change to the CL? >> >> >> On Fri, Jul 13, 2012 at 2:48 PM, Marc-Andre Decoste <mad@chromium.org>wrote: >> >>> Darn... I renamed google_update.* to google_update_win.* because it's >>> only used on windows, but a new dependency has just been added in ChromeOS >>> for a single value: GOOGLE_UPDATE_ERROR_UPDATING... :-( >>> >>> Could we do away without this inclusion? Since ChromeOS doesn't use >>> GoogleUpdate anymore, can it get this value from somewhere else? >>> >>> On Fri, Jul 13, 2012 at 1:42 PM, <commit-bot@chromium.org> wrote: >>> >>>> Try job failure for 10698106-2037 (retry) on linux_chromeos for step >>>> "compile" >>>> (clobber build). >>>> It's a second try, previously, step "compile" failed. >>>> http://build.chromium.org/p/**tryserver.chromium/** >>>> buildstatus?builder=linux_**chromeos&number=26624<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=26624> >>>> >>>> >>>> https://chromiumcodereview.**appspot.com/10698106/<https://chromiumcodereview... >>>> >>> >>> >> >
https://chromiumcodereview.appspot.com/10698106/diff/12009/chrome/browser/ui/... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): https://chromiumcodereview.appspot.com/10698106/diff/12009/chrome/browser/ui/... chrome/browser/ui/webui/help/version_updater_chromeos.cc:103: message = l10n_util::GetStringFUTF16Int(IDS_UPGRADE_ERROR, 7); this is fine with me, but mind adding a comment like: // GOOGLE_UPDATE_ERROR_UPDATING from // chrome/browser/google/google_update.h above this line so it's more clear where the number came from?
OK, how about this instead? Thanks! BYE MAD... http://codereview.chromium.org/10698106/diff/12009/chrome/browser/ui/webui/he... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): http://codereview.chromium.org/10698106/diff/12009/chrome/browser/ui/webui/he... chrome/browser/ui/webui/help/version_updater_chromeos.cc:103: message = l10n_util::GetStringFUTF16Int(IDS_UPGRADE_ERROR, 7); On 2012/07/13 19:27:56, Daniel Erat wrote: > this is fine with me, but mind adding a comment like: > > // GOOGLE_UPDATE_ERROR_UPDATING from > // chrome/browser/google/google_update.h > > above this line so it's more clear where the number came from? Done.
http://codereview.chromium.org/10698106/diff/32/chrome/browser/ui/webui/help/... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): http://codereview.chromium.org/10698106/diff/32/chrome/browser/ui/webui/help/... chrome/browser/ui/webui/help/version_updater_chromeos.cc:27: #define GOOGLE_UPDATE_ERROR_UPDATING 7 nit: const int in an anon namespace instead of #define? but if this error code enum is windows-only, i'm less convinced that it even makes sense to try to use it on chrome os. maybe 0 should just be used below instead.
Agreed. Makes more sense to me too. I thought you needed to use this specific value... Will upload a new version soon... BYE MAD... sent from Mobile Answering Device! Le 13 juil. 2012 15:45, <derat@chromium.org> a écrit : > > http://codereview.chromium.**org/10698106/diff/32/chrome/** > browser/ui/webui/help/version_**updater_chromeos.cc<http://codereview.chromium.org/10698106/diff/32/chrome/browser/ui/webui/help/version_updater_chromeos.cc> > File chrome/browser/ui/webui/help/**version_updater_chromeos.cc (right): > > http://codereview.chromium.**org/10698106/diff/32/chrome/** > browser/ui/webui/help/version_**updater_chromeos.cc#newcode27<http://codereview.chromium.org/10698106/diff/32/chrome/browser/ui/webui/help/version_updater_chromeos.cc#newcode27> > chrome/browser/ui/webui/help/**version_updater_chromeos.cc:**27: #define > GOOGLE_UPDATE_ERROR_UPDATING 7 > nit: const int in an anon namespace instead of #define? > > but if this error code enum is windows-only, i'm less convinced that it > even makes sense to try to use it on chrome os. maybe 0 should just be > used below instead. > > http://codereview.chromium.**org/10698106/<http://codereview.chromium.org/106... >
Like this?...
LGTM for version_updater_chromeos.cc. Thanks!
On 2012/07/13 20:06:33, Daniel Erat wrote: > LGTM for version_updater_chromeos.cc. Thanks! Thank YOU... CQ'ing again... Fingers crossed... :-) BYE MAD
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/10698106/6039
Try job failure for 10698106-6039 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Anybody wants to double check the changes I needed to do to adapt to an API change in the intall_util class? Thanks! Will CQ later tonight... BYE MAD!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/10698106/49001
Change committed as 146738 |
