Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10446091:
Prompt user before restarting System Preferences. (Closed)
|
Created:
8 years, 6 months ago by Lambros Modified:
8 years, 6 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrompt user before restarting System Preferences.
When Chromoting Host components have been upgraded on Mac whilst
the pref-pane is still loaded, prompt the user before restarting
System Preferences.
The user has the opportunity to cancel, in case they were doing
something in a different pref-pane. In that case, the
Chromoting pref-pane should be completely disabled, and shouldn't
process any new config data until it is restarted.
This is a followup CL to http://codereview.chromium.org/10413065/
BUG=129226
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141583
Patch Set 1 #Patch Set 2 : Send notification if user cancels the "restart" alert #
Total comments: 20
Patch Set 3 : Update comments #
Total comments: 2
Patch Set 4 : Rename variable and move check out of readNewConfig #
Total comments: 2
Patch Set 5 : Fix if.. brace style #Messages
Total messages: 11 (0 generated)
Doesn't this mean that if the user clicks Cancel then the web-app will be stuck waiting for a notification that will never come, and the remoting pref pane will be forever disabled? I think a better flow would be to send the "failed" notification in this case, and revert the preferences pane back to to the main screen. In fact, there's a lot of overlap between this and crbug.com/129616 (send a failure notification if the pref pane is exited without completion).
On 2012/05/31 00:18:29, Jamie wrote: > Doesn't this mean that if the user clicks Cancel then the web-app will be stuck > waiting for a notification that will never come, and the remoting pref pane will > be forever disabled? The notification comes eventually, when the pane gets restarted. But you're right - that's too long to leave the web-app spinning. > I think a better flow would be to send the "failed" > notification in this case, and revert the preferences pane back to to the main > screen. Done (except for sending System Preferences to its top-level view - I can't find any API for "closing" or "deselecting" a pref-pane, so we probably shouldn't attempt it).
http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... File remoting/host/me2me_preference_pane.h (right): http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.h:101: // leaving the config file untouched. Is this second sentence still correct? Since cancelling the "please restart" prompt now sends a "failed" message back to the web-app, I don't think it's necessary to retain the config file that we would have read had there been no version mismatch. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:544: // from a previous call to this method. Please clarify that it is only the check that is skipped here, not the consequences of the check failing. In other words, if |is_pane_disabled_| is true, then we know there's a version mismatch--there's no need to recheck. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:567: is_pane_disabled_ = YES; This is a bit confusing. I guess the pane is disabled at this point, but only because the sheet is open. My understanding of the purpose of this variable was that it indicates that the pane is disabled because the user clicked Cancel. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:574: // the window is created. Under what circumstances can this happen? Presumably, only when called from readNewConfig, but can that be called before there's a window? Maybe a race between didSelect and the distributed notification? http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:587: [cancel_button setKeyEquivalent:@"\e"]; No key equivalent for OK? http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:622: if (access(file.c_str(), F_OK) != 0) Is this check needed? Can't we just let remove fail? http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:626: [self notifyPlugin:kUpdateFailedNotificationName]; I think this needs to be moved above the early returns. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:632: // dynamic reloading of pref-panes. Is there any reason to expect that this will happen? If not, I think I'd remove the comment.
http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... File remoting/host/me2me_preference_pane.h (right): http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.h:101: // leaving the config file untouched. On 2012/06/01 18:53:26, Jamie wrote: > Is this second sentence still correct? Yes. Cancelling the "please restart" prompt now sends a "failed" message back to the web-app AND deletes the config file. The config file must be kept if |is_pane_disabled_| is YES, because the user might not have chosen OK yet on the warning that has popped up or is about to be popped up. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:544: // from a previous call to this method. On 2012/06/01 18:53:26, Jamie wrote: > Please clarify that it is only the check that is skipped here, not the > consequences of the check failing. In other words, if |is_pane_disabled_| is > true, then we know there's a version mismatch--there's no need to recheck. Done. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:567: is_pane_disabled_ = YES; On 2012/06/01 18:53:26, Jamie wrote: > This is a bit confusing. I guess the pane is disabled at this point, but only > because the sheet is open. My understanding of the purpose of this variable was > that it indicates that the pane is disabled because the user clicked Cancel. It indicates that the pane is disabled because version-mismatch was detected, even before the user has chosen OK or Cancel. The variable, if YES, prevents the config file being deleted in the meantime. If the user should choose Cancel, then we delete the file and send the UpdateFailed notification. So the variable has a dual purpose - it causes the controls to be greyed-out, and it also prevents the config file from being deleted (until the user hits Cancel). I've updated the comment on that variable. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:574: // the window is created. On 2012/06/01 18:53:26, Jamie wrote: > Under what circumstances can this happen? Presumably, only when called from > readNewConfig, but can that be called before there's a window? Maybe a race > between didSelect and the distributed notification? Open the pref-pane, close it (leaving System Preferences on its main view), then send it a notification. The bundle will be loaded into memory, with the pref-pane invisible. Then this function gets called from the notification-handler, and |window| will be nil. Similarly, |window| is nil if this function gets called from willSelect (since willSelect happens just before the window becomes visible). That's why I needed to add a |didSelect| handler in this CL. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:587: [cancel_button setKeyEquivalent:@"\e"]; On 2012/06/01 18:53:26, Jamie wrote: > No key equivalent for OK? I think the first button already becomes the default button. Attempting to change its key risks breakage. The Cancel button, on the other hand, doesn't trigger when I press Esc, so I have to set it manually. The same thing is done elsewhere, for example, chrome/browser/mac/install_from_dmg.mm http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:622: if (access(file.c_str(), F_OK) != 0) On 2012/06/01 18:53:26, Jamie wrote: > Is this check needed? Can't we just let remove fail? I don't think we want to send an UpdateFailed notification if there's no config file (this is the logic in |willSelect| and I think we should keep consistency). There might not be any harm in sending the notification anyway, but it's an abuse of the name "UpdateFailed" if there was no "update" as such. Off-topic for this CL, but I think there's a case for getting rid of "UpdateFailed", and sending a simple "PaneClosed" notification every time the pane is closed. The web-app knows whether it has launched the pref-pane, so if the web-app sees "PaneClosed" before anything else, it can infer the requested operation failed (whether it be setting a new PIN or disabling the host). http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:626: [self notifyPlugin:kUpdateFailedNotificationName]; On 2012/06/01 18:53:26, Jamie wrote: > I think this needs to be moved above the early returns. Same as above. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.mm:632: // dynamic reloading of pref-panes. On 2012/06/01 18:53:26, Jamie wrote: > Is there any reason to expect that this will happen? If not, I think I'd remove > the comment. No :) Done.
http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... File remoting/host/me2me_preference_pane.h (right): http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.h:71: BOOL is_pane_disabled_; Now that I understand better what's going on here, I think a better name for this would be something like |confirm_restart_pending_or_canceled_|. The fact that the pane is disabled is just one consequence of being in this state. As your comment below describes, there are other consequences. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.h:101: // leaving the config file untouched. On 2012/06/05 17:57:39, Lambros wrote: > On 2012/06/01 18:53:26, Jamie wrote: > > Is this second sentence still correct? > Yes. Cancelling the "please restart" prompt now sends a "failed" message back > to the web-app AND deletes the config file. > > The config file must be kept if |is_pane_disabled_| is YES, because the user > might not have chosen OK yet on the warning that has popped up or is about to be > popped up. Perhaps it would be better not to call this method in that case, then you wouldn't need this comment. Right now this function doesn't quite do what its name suggests, which is always a potential source of confusion. http://codereview.chromium.org/10446091/diff/12001/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10446091/diff/12001/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:293: } As my previous remark states, I don't think either of these checks is correct with the function named the way it is.
http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... File remoting/host/me2me_preference_pane.h (right): http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.h:71: BOOL is_pane_disabled_; On 2012/06/06 16:26:49, Jamie wrote: > Now that I understand better what's going on here, I think a better name for > this would be something like |confirm_restart_pending_or_canceled_|. The fact > that the pane is disabled is just one consequence of being in this state. As > your comment below describes, there are other consequences. Done. http://codereview.chromium.org/10446091/diff/6003/remoting/host/me2me_prefere... remoting/host/me2me_preference_pane.h:101: // leaving the config file untouched. On 2012/06/06 16:26:49, Jamie wrote: > On 2012/06/05 17:57:39, Lambros wrote: > > On 2012/06/01 18:53:26, Jamie wrote: > > > Is this second sentence still correct? > > Yes. Cancelling the "please restart" prompt now sends a "failed" message back > > to the web-app AND deletes the config file. > > > > The config file must be kept if |is_pane_disabled_| is YES, because the user > > might not have chosen OK yet on the warning that has popped up or is about to > be > > popped up. > > Perhaps it would be better not to call this method in that case, then you > wouldn't need this comment. Right now this function doesn't quite do what its > name suggests, which is always a potential source of confusion. Done. http://codereview.chromium.org/10446091/diff/12001/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10446091/diff/12001/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:293: } On 2012/06/06 16:26:49, Jamie wrote: > As my previous remark states, I don't think either of these checks is correct > with the function named the way it is. Done.
lgtm with one nit. http://codereview.chromium.org/10446091/diff/16001/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10446091/diff/16001/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:184: if (!restart_pending_or_canceled_) Nit: You've used a mix of braced and unbraced one-line ifs in this CL. My preference is for the former, but stick to whatever is used elsewhere in this file for consistency.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/10446091/2...
Change committed as 141583
http://codereview.chromium.org/10446091/diff/16001/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10446091/diff/16001/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:184: if (!restart_pending_or_canceled_) On 2012/06/07 20:41:35, Jamie wrote: > Nit: You've used a mix of braced and unbraced one-line ifs in this CL. My > preference is for the former, but stick to whatever is used elsewhere in this > file for consistency. Done. |
