Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2944713003:
After signin token check failed, show force reauth dialog and start window closing countdown. (Closed)
DescriptionAfter signin token check failed, show force reauth dialog and start window closing countdown.
BUG=642059
Review-Url: https://codereview.chromium.org/2944713003
Cr-Commit-Position: refs/heads/master@{#482668}
Committed: https://chromium.googlesource.com/chromium/src/+/9a740797201d02dea291dfc83c8abb01f26fbc53
Patch Set 1 #Patch Set 2 : fixup #
Total comments: 8
Patch Set 3 : cr #
Total comments: 1
Patch Set 4 : bridge #Patch Set 5 : rebase from master #
Total comments: 6
Patch Set 6 : cr #
Total comments: 4
Patch Set 7 : delegate #
Total comments: 2
Patch Set 8 : rename #Patch Set 9 : move CloseDialog to ~Impl #
Total comments: 2
Patch Set 10 : nit #Messages
Total messages: 93 (71 generated)
Description was changed from ========== After signin token check failed, show force reauth dialog and start window closing countdown. BUG= ========== to ========== After signin token check failed, show force reauth dialog and start window closing countdown. BUG=642059 ==========
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zmin@chromium.org changed reviewers: + rogerta@chromium.org
Hi Roger, Can you take a look this CL please? Owen
lgtm with comments below https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... File chrome/browser/signin/force_signin_verifier.cc (right): https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... chrome/browser/signin/force_signin_verifier.cc:34: const int kWindowClosingShortDelayInSecond = 30; // I I think the comments here are incomplete. Better to put the comment above each constant instead of the right,especially when they are long. https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... chrome/browser/signin/force_signin_verifier.cc:129: base::TimeDelta::FromSeconds(kNoDelaySignoutInSecond)) { Can you add a comment explaining the if condition? Also, where does 3 seconds come from? Is this possibly racy? https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... File chrome/browser/signin/force_signin_verifier.h (right): https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... chrome/browser/signin/force_signin_verifier.h:78: void CloseWindows(); Nit: rename function to CloseAllBrowserWindows() and then you don't need the comment. https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... chrome/browser/signin/force_signin_verifier.h:98: // The widget of dialog. Comments here are not very descriptive. I'd remove line 98 and reword the comment at line 96 to say something like these members are used to implement a countdown timer that automatically closes the profile if the user has not reauth'ed in the allotted time.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... File chrome/browser/signin/force_signin_verifier.cc (right): https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... chrome/browser/signin/force_signin_verifier.cc:34: const int kWindowClosingShortDelayInSecond = 30; // I On 2017/06/20 14:17:51, Roger Tawa wrote: > I think the comments here are incomplete. Better to put the comment above each > constant instead of the right,especially when they are long. Done. https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... chrome/browser/signin/force_signin_verifier.cc:129: base::TimeDelta::FromSeconds(kNoDelaySignoutInSecond)) { On 2017/06/20 14:17:51, Roger Tawa wrote: > Can you add a comment explaining the if condition? Also, where does 3 seconds > come from? Is this possibly racy? I have updated the comments above and rename the variables to make the logic here more clear. Based on the metrics, 99% of gaia ping can be finished in 3 seconds. User will get a short countdown if ping returns in 3 seconds. Once the reauth done, the ForceSigninVerifier will be recreated and the countdown will be stopped automatically. However, there're still some other potential race conditions. So I added some code to cover them: 1) If signout has been called somewhere else and window has been closed, stop the countdown to avoid double sign out. 2) If AuthInProcess is true, do not close window to interrupt the signin. If it fails later, then the signin process should take care of the signout after that. Note that, if user is still typing on the reauth gaia dialog, window will be closed. However, if user press 'Signin again' button when countdown is less than 60 seconds, windows will be closed before reauth dialog displayed. https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... File chrome/browser/signin/force_signin_verifier.h (right): https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... chrome/browser/signin/force_signin_verifier.h:78: void CloseWindows(); On 2017/06/20 14:17:52, Roger Tawa wrote: > Nit: rename function to CloseAllBrowserWindows() and then you don't need the > comment. Done. https://codereview.chromium.org/2944713003/diff/40001/chrome/browser/signin/f... chrome/browser/signin/force_signin_verifier.h:98: // The widget of dialog. On 2017/06/20 14:17:52, Roger Tawa wrote: > Comments here are not very descriptive. I'd remove line 98 and reword the > comment at line 96 to say something like these members are used to implement a > countdown timer that automatically closes the profile if the user has not > reauth'ed in the allotted time. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zmin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by zmin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zmin@chromium.org
zmin@chromium.org changed reviewers: + sky@chromium.org
+sky for chrome/browser/signin/DEPS
https://codereview.chromium.org/2944713003/diff/60001/chrome/browser/signin/DEPS File chrome/browser/signin/DEPS (right): https://codereview.chromium.org/2944713003/diff/60001/chrome/browser/signin/D... chrome/browser/signin/DEPS:7: "+chrome/browser/ui/views/profiles/forced_reauthentication_dialog.h", In general we do not want chrome/browser code referring to platform specific implementations. Why do you need to refer to a views implementation here?
On 2017/06/21 00:29:54, sky wrote: > https://codereview.chromium.org/2944713003/diff/60001/chrome/browser/signin/DEPS > File chrome/browser/signin/DEPS (right): > > https://codereview.chromium.org/2944713003/diff/60001/chrome/browser/signin/D... > chrome/browser/signin/DEPS:7: > "+chrome/browser/ui/views/profiles/forced_reauthentication_dialog.h", > In general we do not want chrome/browser code referring to platform specific > implementations. Why do you need to refer to a views implementation here? The dialog is located in chrome/browser/ui/view because it's view related. The trigger of this dialog is located in chrome/browser/ because it's associated with Profile and SigninClient I'm not sure how to send message from chrome/browser to chrome/browser/ui/views/ I have checked some other automatically poped up dialogs and they are using NOTIFICATION. But I don't know whether it's a good idea to create NOTIFICATIONs just for one dialog. Especially this is only behind enterprise policy on desktop. Do you know if there is any other way to do so?
On 2017/06/21 00:48:09, zmin wrote: > On 2017/06/21 00:29:54, sky wrote: > > > https://codereview.chromium.org/2944713003/diff/60001/chrome/browser/signin/DEPS > > File chrome/browser/signin/DEPS (right): > > > > > https://codereview.chromium.org/2944713003/diff/60001/chrome/browser/signin/D... > > chrome/browser/signin/DEPS:7: > > "+chrome/browser/ui/views/profiles/forced_reauthentication_dialog.h", > > In general we do not want chrome/browser code referring to platform specific > > implementations. Why do you need to refer to a views implementation here? > > The dialog is located in chrome/browser/ui/view because it's view related. > The trigger of this dialog is located in chrome/browser/ because it's associated > with Profile and SigninClient > I'm not sure how to send message from chrome/browser to chrome/browser/ui/views/ > I have checked some other automatically poped up dialogs and they are using > NOTIFICATION. > But I don't know whether it's a good idea to create NOTIFICATIONs just for one > dialog. Especially this is only behind enterprise policy on desktop. > > Do you know if there is any other way to do so? Typically you create a platform neutral interface that has an implementation in each of the platform specific directories. For example, BrowserWindow is the platform neutral interface and BrowserView is an implementation of it.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/06/21 14:50:31, sky wrote: > On 2017/06/21 00:48:09, zmin wrote: > > On 2017/06/21 00:29:54, sky wrote: > > > > > > https://codereview.chromium.org/2944713003/diff/60001/chrome/browser/signin/DEPS > > > File chrome/browser/signin/DEPS (right): > > > > > > > > > https://codereview.chromium.org/2944713003/diff/60001/chrome/browser/signin/D... > > > chrome/browser/signin/DEPS:7: > > > "+chrome/browser/ui/views/profiles/forced_reauthentication_dialog.h", > > > In general we do not want chrome/browser code referring to platform specific > > > implementations. Why do you need to refer to a views implementation here? > > > > The dialog is located in chrome/browser/ui/view because it's view related. > > The trigger of this dialog is located in chrome/browser/ because it's > associated > > with Profile and SigninClient > > I'm not sure how to send message from chrome/browser to > chrome/browser/ui/views/ > > I have checked some other automatically poped up dialogs and they are using > > NOTIFICATION. > > But I don't know whether it's a good idea to create NOTIFICATIONs just for one > > dialog. Especially this is only behind enterprise policy on desktop. > > > > Do you know if there is any other way to do so? > > Typically you create a platform neutral interface that has an implementation in > each of the platform specific directories. For example, BrowserWindow is the > platform neutral interface and BrowserView is an implementation of it. Thanks for the advice. So I added Show/HideForcedReauthenticationDialog function in browser_dialogs.h as a bridge to avoid the DEPS change. I also added a std::map in forced_reauthentication_dialog.cc to track all dialog instances. So that dialog can be closed when HideForcedReauthenticationDialog is called. It introduces change on: chrome/browser/ui/browser_dialogs.h chrome/browser/ui/views/profiles/forced_reauthentication_dialog.cc Please take a look. Owen
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2944713003/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2944713003/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:169: // Show/Hide the ForcedReauthenticationDialog for |profile|. Please document *all* paremeters. https://codereview.chromium.org/2944713003/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:170: void ShowForcedReauthenticationDialog( Can this return an object that is used to delete/close the dialog rather than having to maintain a map? https://codereview.chromium.org/2944713003/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:173: const base::TimeDelta& countdown_duration); We typically don't use a const ref for TimeDelta, but pass by value.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2944713003/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2944713003/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:169: // Show/Hide the ForcedReauthenticationDialog for |profile|. On 2017/06/22 03:28:14, sky wrote: > Please document *all* paremeters. Done. https://codereview.chromium.org/2944713003/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:170: void ShowForcedReauthenticationDialog( On 2017/06/22 03:28:14, sky wrote: > Can this return an object that is used to delete/close the dialog rather than > having to maintain a map? Done. https://codereview.chromium.org/2944713003/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:173: const base::TimeDelta& countdown_duration); On 2017/06/22 03:28:14, sky wrote: > We typically don't use a const ref for TimeDelta, but pass by value. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2944713003/diff/140001/chrome/browser/signin/... File chrome/browser/signin/force_signin_verifier.cc (right): https://codereview.chromium.org/2944713003/diff/140001/chrome/browser/signin/... chrome/browser/signin/force_signin_verifier.cc:15: #include "ui/views/widget/widget.h" This code shouldn't use views either. Remember, keep the platform specific code in the platform specific directories. https://codereview.chromium.org/2944713003/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2944713003/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:177: views::Widget* ShowForcedReauthenticationDialog( Remember, no platform specific ui code here. Also, please merge with with the ifdef two lines above.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #7 (id:180001) has been deleted
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2944713003/diff/140001/chrome/browser/signin/... File chrome/browser/signin/force_signin_verifier.cc (right): https://codereview.chromium.org/2944713003/diff/140001/chrome/browser/signin/... chrome/browser/signin/force_signin_verifier.cc:15: #include "ui/views/widget/widget.h" On 2017/06/22 14:52:02, sky wrote: > This code shouldn't use views either. Remember, keep the platform specific code > in the platform specific directories. Ok, I have created a separate class wrap the dialog instance for show/close operator. https://codereview.chromium.org/2944713003/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2944713003/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:177: views::Widget* ShowForcedReauthenticationDialog( On 2017/06/22 14:52:02, sky wrote: > Remember, no platform specific ui code here. > Also, please merge with with the ifdef two lines above. And I have moved the new class/function Declarations to its own file.
https://codereview.chromium.org/2944713003/diff/200001/chrome/browser/ui/forc... File chrome/browser/ui/forced_reauthentication_dialog_delegate.h (right): https://codereview.chromium.org/2944713003/diff/200001/chrome/browser/ui/forc... chrome/browser/ui/forced_reauthentication_dialog_delegate.h:20: class ForcedReauthenticationDialogDelegate { Much better, thanks. It isn't clear why this has Delegate in the name. There is no delegation here. How about ForcedReauthenticationDialog as that is what this is, right? You can name the views one ForcedReauthenticationDialogViews. Also, make this pure virtual with a create function. That way you don't need the dialog_ member as it's handled by the appropriate implementation. https://codereview.chromium.org/2944713003/diff/200001/chrome/browser/ui/forc... chrome/browser/ui/forced_reauthentication_dialog_delegate.h:33: void CloseDialog(); Is there a reason for the explicit close? AFAICT this is only called from the destructor, so that if you make the destructor implicitly close the dialog you don't need this.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/23 17:06:04, sky wrote: > https://codereview.chromium.org/2944713003/diff/200001/chrome/browser/ui/forc... > File chrome/browser/ui/forced_reauthentication_dialog_delegate.h (right): > > https://codereview.chromium.org/2944713003/diff/200001/chrome/browser/ui/forc... > chrome/browser/ui/forced_reauthentication_dialog_delegate.h:20: class > ForcedReauthenticationDialogDelegate { > Much better, thanks. > It isn't clear why this has Delegate in the name. There is no delegation here. > How about ForcedReauthenticationDialog as that is what this is, right? You can > name the views one ForcedReauthenticationDialogViews. Done. > Also, make this pure virtual with a create function. That way you don't need the > dialog_ member as it's handled by the appropriate implementation. Done. > > https://codereview.chromium.org/2944713003/diff/200001/chrome/browser/ui/forc... > chrome/browser/ui/forced_reauthentication_dialog_delegate.h:33: void > CloseDialog(); > Is there a reason for the explicit close? AFAICT this is only called from the > destructor, so that if you make the destructor implicitly close the dialog you > don't need this. CloseDialog function is used to dismiss the dialog from chrome/browser. (When user ignores the dialog and sign in again from another browser window) Since the dialog_view owns itself, I create this function instead of calling its destructor directly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On Sun, Jun 25, 2017 at 4:37 PM, <zmin@chromium.org> wrote: > On 2017/06/23 17:06:04, sky wrote: > > > https://codereview.chromium.org/2944713003/diff/200001/ > chrome/browser/ui/forced_reauthentication_dialog_delegate.h > > File chrome/browser/ui/forced_reauthentication_dialog_delegate.h > (right): > > > > > https://codereview.chromium.org/2944713003/diff/200001/ > chrome/browser/ui/forced_reauthentication_dialog_delegate.h#newcode20 > > chrome/browser/ui/forced_reauthentication_dialog_delegate.h:20: class > > ForcedReauthenticationDialogDelegate { > > Much better, thanks. > > It isn't clear why this has Delegate in the name. There is no delegation > here. > > How about ForcedReauthenticationDialog as that is what this is, right? > You can > > name the views one ForcedReauthenticationDialogViews. > Done. > > > Also, make this pure virtual with a create function. That way you don't > need > the > > dialog_ member as it's handled by the appropriate implementation. > Done. > > > > > https://codereview.chromium.org/2944713003/diff/200001/ > chrome/browser/ui/forced_reauthentication_dialog_delegate.h#newcode33 > > chrome/browser/ui/forced_reauthentication_dialog_delegate.h:33: void > > CloseDialog(); > > Is there a reason for the explicit close? AFAICT this is only called > from the > > destructor, so that if you make the destructor implicitly close the > dialog you > > don't need this. > CloseDialog function is used to dismiss the dialog from chrome/browser. > (When > user ignores the dialog and sign in again from another browser window) > Since the dialog_view owns itself, I create this function instead of > calling its > destructor directly. > Is there a place where you call CloseDialog outside of the destructor? > > > > > > > https://codereview.chromium.org/2944713003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2944713003/diff/240001/chrome/browser/ui/forc... File chrome/browser/ui/forced_reauthentication_dialog.h (right): https://codereview.chromium.org/2944713003/diff/240001/chrome/browser/ui/forc... chrome/browser/ui/forced_reauthentication_dialog.h:11: #include "base/memory/weak_ptr.h" This include isn't used. https://codereview.chromium.org/2944713003/diff/240001/chrome/browser/ui/forc... chrome/browser/ui/forced_reauthentication_dialog.h:25: virtual ~ForcedReauthenticationDialog() = 0; Typically we inline these. That way you don't have to put the definition in each platform.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2944713003/#ps260001 (title: "nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1498584743518160,
"parent_rev": "715cc9a29f6d78cf129f319a6f0b028a961fc5ed", "commit_rev":
"9a740797201d02dea291dfc83c8abb01f26fbc53"}
Message was sent while issue was closed.
Description was changed from ========== After signin token check failed, show force reauth dialog and start window closing countdown. BUG=642059 ========== to ========== After signin token check failed, show force reauth dialog and start window closing countdown. BUG=642059 Review-Url: https://codereview.chromium.org/2944713003 Cr-Commit-Position: refs/heads/master@{#482668} Committed: https://chromium.googlesource.com/chromium/src/+/9a740797201d02dea291dfc83c8a... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://chromium.googlesource.com/chromium/src/+/9a740797201d02dea291dfc83c8a... |
