Issue 2862653002: If force-sign-in policy is enabled, popup warning dialog before window closing if auth token becom…

zmin

zmin@chromium.org changed reviewers: + sky@chromium.org

3 years, 7 months ago (2017-05-03 19:29:04 UTC) #1

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-03 19:34:38 UTC) #3

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/1

3 years, 7 months ago (2017-05-03 19:35:19 UTC) #4

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-03 20:03:44 UTC) #5

commit-bot: I haz the power

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_compile_dbg_ng/builds/410154)

3 years, 7 months ago (2017-05-03 20:03:45 UTC) #6

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-03 20:15:29 UTC) #7

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/20001

3 years, 7 months ago (2017-05-03 20:15:47 UTC) #8

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-03 20:41:30 UTC) #9

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/66327)

3 years, 7 months ago (2017-05-03 20:41:31 UTC) #10

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-03 21:04:38 UTC) #12

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/40001

3 years, 7 months ago (2017-05-03 21:05:04 UTC) #13

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-03 21:21:19 UTC) #14

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/375954)

3 years, 7 months ago (2017-05-03 21:21:20 UTC) #15

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-03 21:33:03 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/60001

3 years, 7 months ago (2017-05-03 21:34:24 UTC) #18

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-03 21:54:46 UTC) #19

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/331685) linux_chromium_chromeos_ozone_rel_ng on ...

3 years, 7 months ago (2017-05-03 21:54:47 UTC) #20

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-03 22:31:31 UTC) #22

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/80001

3 years, 7 months ago (2017-05-03 22:32:34 UTC) #23

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-03 23:45:37 UTC) #24

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-03 23:45:38 UTC) #25

sky

https://codereview.chromium.org/2862653002/diff/80001/chrome/app/chromium_strings.grd File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/2862653002/diff/80001/chrome/app/chromium_strings.grd#newcode761 chrome/app/chromium_strings.grd:761: <message name="IDS_ENTERPRISE_FORCE_SIGNOUT_CLOSE_CONFIRM" desc="Text of the button to cofirm close ...

3 years, 7 months ago (2017-05-04 03:45:12 UTC) #26

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-04 23:13:29 UTC) #27

zmin

https://codereview.chromium.org/2862653002/diff/80001/chrome/app/chromium_strings.grd File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/2862653002/diff/80001/chrome/app/chromium_strings.grd#newcode761 chrome/app/chromium_strings.grd:761: <message name="IDS_ENTERPRISE_FORCE_SIGNOUT_CLOSE_CONFIRM" desc="Text of the button to cofirm close ...

3 years, 7 months ago (2017-05-04 23:13:53 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/100001

3 years, 7 months ago (2017-05-04 23:14:17 UTC) #29

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-04 23:36:38 UTC) #30

commit-bot: I haz the power

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_compile_dbg_ng/builds/411308) mac_chromium_rel_ng on ...

3 years, 7 months ago (2017-05-04 23:36:39 UTC) #31

sky

https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views/profiles/force_signout_dialog.cc File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right): https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode38 chrome/browser/ui/views/profiles/force_signout_dialog.cc:38: bool IsBrowserBelongedToProfile(Browser* browser, Profile* profile) { On 2017/05/04 23:13:52, ...

3 years, 7 months ago (2017-05-05 00:00:10 UTC) #32

https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):

https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/force_signout_dialog.cc:38: bool
IsBrowserBelongedToProfile(Browser* browser, Profile* profile) {
On 2017/05/04 23:13:52, zmin wrote:
> On 2017/05/04 03:45:11, sky wrote:
> > How about naming this IsMatchingBrowser and include the tab_strip_model
check
> > here too? 
> Done.
> Two questions on the implementation:
> > . Do you really want to incognito windows here? It seems weird to anchor to
an
> > incognito window?
> > . Are you sure you don't want to look at the type? Anchoring to popups could
> be
> > weird.
> > . Don't you want to check visibility of the window?
> 
> The purpose the dialog is warning the window closing. So
> Visibility, yes I'll check the visibility. it seems no harm to close if all
> windows are invisible.

Be careful there. You should verify you get the right thing if the window is
minimized. I would say in that case you should force the window to be shown and
then show the dialog.

> Incognito and popups. I agree it's kinda weird. But since they will be closed
> later as well, I think it make sense to show the warning msg before.

https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
app_modal::AppModalDialog(
On 2017/05/04 23:13:52, zmin wrote:
> On 2017/05/04 03:45:11, sky wrote:
> > Why are you using app_modal_dialog here? I understand that you want app
modal,
> > but you needn't use the AppModal class for that. Using the Appmodal class
ties
> > you to WebContents, which I'm not sure you want.
> 
> No, I don't want to be tied to WebContents. However, what i want is this:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...
> It prevent user open other browser window when the dialog is being displayed.
> And right now I only found AppModalDialog class can do so with
> AppModalDialogQueue. User can still use other chrome window when a normal
window
> modal dialog is opened.
> 

Also, user could still interact with non-browser windows in Chrome. Are you sure
you want that?

https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
app_modal::AppModalDialog(
On 2017/05/04 23:13:52, zmin wrote:
> On 2017/05/04 03:45:11, sky wrote:
> > Why are you using app_modal_dialog here? I understand that you want app
modal,
> > but you needn't use the AppModal class for that. Using the Appmodal class
ties
> > you to WebContents, which I'm not sure you want.
> 
> No, I don't want to be tied to WebContents. However, what i want is this:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...
> It prevent user open other browser window when the dialog is being displayed.
> And right now I only found AppModalDialog class can do so with
> AppModalDialogQueue. User can still use other chrome window when a normal
window
> modal dialog is opened.
> 

Also, what happens if the browser you are anchored to ends up closing? This
could happen for a number of reasons.

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-05 22:20:46 UTC) #33

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/120001

3 years, 7 months ago (2017-05-05 22:22:04 UTC) #34

zmin

I have tested the dialog on Mac. The dialog itself looks good. But it seems ...

3 years, 7 months ago (2017-05-05 22:22:57 UTC) #35

I have tested the dialog on Mac. The dialog itself looks good. But it seems that
the app modal dialog doesn't block other browser window on Mac.

I think it's not what I expected. So i remove Mac support for now.

On 2017/05/05 00:00:10, sky wrote:
>
https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views...
> File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):
> 
>
https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views...
> chrome/browser/ui/views/profiles/force_signout_dialog.cc:38: bool
> IsBrowserBelongedToProfile(Browser* browser, Profile* profile) {
> On 2017/05/04 23:13:52, zmin wrote:
> > On 2017/05/04 03:45:11, sky wrote:
> > > How about naming this IsMatchingBrowser and include the tab_strip_model
> check
> > > here too? 
> > Done.
> > Two questions on the implementation:
> > > . Do you really want to incognito windows here? It seems weird to anchor
to
> an
> > > incognito window?
> > > . Are you sure you don't want to look at the type? Anchoring to popups
could
> > be
> > > weird.
> > > . Don't you want to check visibility of the window?
> > 
> > The purpose the dialog is warning the window closing. So
> > Visibility, yes I'll check the visibility. it seems no harm to close if all
> > windows are invisible.
> 
> Be careful there. You should verify you get the right thing if the window is
> minimized. I would say in that case you should force the window to be shown
and
> then show the dialog.

I have tested that, this IsVisible function returns True for a minimized window.

> 
> > Incognito and popups. I agree it's kinda weird. But since they will be
closed
> > later as well, I think it make sense to show the warning msg before.
> 
>
https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views...
> chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> app_modal::AppModalDialog(
> On 2017/05/04 23:13:52, zmin wrote:
> > On 2017/05/04 03:45:11, sky wrote:
> > > Why are you using app_modal_dialog here? I understand that you want app
> modal,
> > > but you needn't use the AppModal class for that. Using the Appmodal class
> ties
> > > you to WebContents, which I'm not sure you want.
> > 
> > No, I don't want to be tied to WebContents. However, what i want is this:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...
> > It prevent user open other browser window when the dialog is being
displayed.
> > And right now I only found AppModalDialog class can do so with
> > AppModalDialogQueue. User can still use other chrome window when a normal
> window
> > modal dialog is opened.
> > 
> 
> Also, user could still interact with non-browser windows in Chrome. Are you
sure
> you want that?

Yes, I think it's good enough. I don't think it's necessary to block a
non-browser window because user can't use them to browser the internet.

> 
>
https://codereview.chromium.org/2862653002/diff/80001/chrome/browser/ui/views...
> chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> app_modal::AppModalDialog(
> On 2017/05/04 23:13:52, zmin wrote:
> > On 2017/05/04 03:45:11, sky wrote:
> > > Why are you using app_modal_dialog here? I understand that you want app
> modal,
> > > but you needn't use the AppModal class for that. Using the Appmodal class
> ties
> > > you to WebContents, which I'm not sure you want.
> > 
> > No, I don't want to be tied to WebContents. However, what i want is this:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...
> > It prevent user open other browser window when the dialog is being
displayed.
> > And right now I only found AppModalDialog class can do so with
> > AppModalDialogQueue. User can still use other chrome window when a normal
> window
> > modal dialog is opened.
> > 
> 
> Also, what happens if the browser you are anchored to ends up closing? This
> could happen for a number of reasons.

In general, there is always a default behavior(cancel) when dialog is closed.
Because the dialog instance is stored in the browser process. So i guess it will
always block all browser windows unless the browser process is terminated. In
which cases, it doesn't matter anyway.

sky

Sorry to ask this now, but is there a design doc for this? I'm wondering ...

3 years, 7 months ago (2017-05-05 22:32:20 UTC) #36

Sorry to ask this now, but is there a design doc for this? I'm wondering
why we are trying to block usage at all instead of presenting the warning
timer and forcing closing if the timer expires?

On Fri, May 5, 2017 at 3:22 PM, <zmin@chromium.org> wrote:

> I have tested the dialog on Mac. The dialog itself looks good. But it
> seems that
> the app modal dialog doesn't block other browser window on Mac.
>
> I think it's not what I expected. So i remove Mac support for now.
>
>
> On 2017/05/05 00:00:10, sky wrote:
> >
> https://codereview.chromium.org/2862653002/diff/80001/
> chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):
> >
> >
> https://codereview.chromium.org/2862653002/diff/80001/
> chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode38
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc:38: bool
> > IsBrowserBelongedToProfile(Browser* browser, Profile* profile) {
> > On 2017/05/04 23:13:52, zmin wrote:
> > > On 2017/05/04 03:45:11, sky wrote:
> > > > How about naming this IsMatchingBrowser and include the
> tab_strip_model
> > check
> > > > here too?
> > > Done.
> > > Two questions on the implementation:
> > > > . Do you really want to incognito windows here? It seems weird to
> anchor
> to
> > an
> > > > incognito window?
> > > > . Are you sure you don't want to look at the type? Anchoring to
> popups
> could
> > > be
> > > > weird.
> > > > . Don't you want to check visibility of the window?
> > >
> > > The purpose the dialog is warning the window closing. So
> > > Visibility, yes I'll check the visibility. it seems no harm to close
> if all
> > > windows are invisible.
> >
> > Be careful there. You should verify you get the right thing if the
> window is
> > minimized. I would say in that case you should force the window to be
> shown
> and
> > then show the dialog.
>
> I have tested that, this IsVisible function returns True for a minimized
> window.
>
> >
> > > Incognito and popups. I agree it's kinda weird. But since they will be
> closed
> > > later as well, I think it make sense to show the warning msg before.
> >
> >
> https://codereview.chromium.org/2862653002/diff/80001/
> chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode100
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > app_modal::AppModalDialog(
> > On 2017/05/04 23:13:52, zmin wrote:
> > > On 2017/05/04 03:45:11, sky wrote:
> > > > Why are you using app_modal_dialog here? I understand that you want
> app
> > modal,
> > > > but you needn't use the AppModal class for that. Using the Appmodal
> class
> > ties
> > > > you to WebContents, which I'm not sure you want.
> > >
> > > No, I don't want to be tied to WebContents. However, what i want is
> this:
> > >
> >
> https://cs.chromium.org/chromium/src/chrome/browser/
> ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > It prevent user open other browser window when the dialog is being
> displayed.
> > > And right now I only found AppModalDialog class can do so with
> > > AppModalDialogQueue. User can still use other chrome window when a
> normal
> > window
> > > modal dialog is opened.
> > >
> >
> > Also, user could still interact with non-browser windows in Chrome. Are
> you
> sure
> > you want that?
>
> Yes, I think it's good enough. I don't think it's necessary to block a
> non-browser window because user can't use them to browser the internet.
>
> >
> >
> https://codereview.chromium.org/2862653002/diff/80001/
> chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode100
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > app_modal::AppModalDialog(
> > On 2017/05/04 23:13:52, zmin wrote:
> > > On 2017/05/04 03:45:11, sky wrote:
> > > > Why are you using app_modal_dialog here? I understand that you want
> app
> > modal,
> > > > but you needn't use the AppModal class for that. Using the Appmodal
> class
> > ties
> > > > you to WebContents, which I'm not sure you want.
> > >
> > > No, I don't want to be tied to WebContents. However, what i want is
> this:
> > >
> >
> https://cs.chromium.org/chromium/src/chrome/browser/
> ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > It prevent user open other browser window when the dialog is being
> displayed.
> > > And right now I only found AppModalDialog class can do so with
> > > AppModalDialogQueue. User can still use other chrome window when a
> normal
> > window
> > > modal dialog is opened.
> > >
> >
> > Also, what happens if the browser you are anchored to ends up closing?
> This
> > could happen for a number of reasons.
>
> In general, there is always a default behavior(cancel) when dialog is
> closed.
> Because the dialog instance is stored in the browser process. So i guess
> it will
> always block all browser windows unless the browser process is terminated.
> In
> which cases, it doesn't matter anyway.
>
>
>
> https://codereview.chromium.org/2862653002/
>

-- 
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.

zmin

On 2017/05/05 22:32:20, sky wrote: > Sorry to ask this now, but is there a ...

3 years, 7 months ago (2017-05-05 22:42:05 UTC) #37

On 2017/05/05 22:32:20, sky wrote:
> Sorry to ask this now, but is there a design doc for this? I'm wondering
> why we are trying to block usage at all instead of presenting the warning
> timer and forcing closing if the timer expires?
Yes:
https://docs.google.com/document/d/1cfDuV6n7puoWV2cMVI_LXlY8TOh1_jNulVPHyGjXW...

User may leave his or her desk when the dialog is poped. That's why the timer is
started only after the dialog is confirmed/canceled.


> On Fri, May 5, 2017 at 3:22 PM, <mailto:zmin@chromium.org> wrote:
> 
> > I have tested the dialog on Mac. The dialog itself looks good. But it
> > seems that
> > the app modal dialog doesn't block other browser window on Mac.
> >
> > I think it's not what I expected. So i remove Mac support for now.
> >
> >
> > On 2017/05/05 00:00:10, sky wrote:
> > >
> > https://codereview.chromium.org/2862653002/diff/80001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > > File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):
> > >
> > >
> > https://codereview.chromium.org/2862653002/diff/80001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode38
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:38: bool
> > > IsBrowserBelongedToProfile(Browser* browser, Profile* profile) {
> > > On 2017/05/04 23:13:52, zmin wrote:
> > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > How about naming this IsMatchingBrowser and include the
> > tab_strip_model
> > > check
> > > > > here too?
> > > > Done.
> > > > Two questions on the implementation:
> > > > > . Do you really want to incognito windows here? It seems weird to
> > anchor
> > to
> > > an
> > > > > incognito window?
> > > > > . Are you sure you don't want to look at the type? Anchoring to
> > popups
> > could
> > > > be
> > > > > weird.
> > > > > . Don't you want to check visibility of the window?
> > > >
> > > > The purpose the dialog is warning the window closing. So
> > > > Visibility, yes I'll check the visibility. it seems no harm to close
> > if all
> > > > windows are invisible.
> > >
> > > Be careful there. You should verify you get the right thing if the
> > window is
> > > minimized. I would say in that case you should force the window to be
> > shown
> > and
> > > then show the dialog.
> >
> > I have tested that, this IsVisible function returns True for a minimized
> > window.
> >
> > >
> > > > Incognito and popups. I agree it's kinda weird. But since they will be
> > closed
> > > > later as well, I think it make sense to show the warning msg before.
> > >
> > >
> > https://codereview.chromium.org/2862653002/diff/80001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode100
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > > app_modal::AppModalDialog(
> > > On 2017/05/04 23:13:52, zmin wrote:
> > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > Why are you using app_modal_dialog here? I understand that you want
> > app
> > > modal,
> > > > > but you needn't use the AppModal class for that. Using the Appmodal
> > class
> > > ties
> > > > > you to WebContents, which I'm not sure you want.
> > > >
> > > > No, I don't want to be tied to WebContents. However, what i want is
> > this:
> > > >
> > >
> > https://cs.chromium.org/chromium/src/chrome/browser/
> > ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > > It prevent user open other browser window when the dialog is being
> > displayed.
> > > > And right now I only found AppModalDialog class can do so with
> > > > AppModalDialogQueue. User can still use other chrome window when a
> > normal
> > > window
> > > > modal dialog is opened.
> > > >
> > >
> > > Also, user could still interact with non-browser windows in Chrome. Are
> > you
> > sure
> > > you want that?
> >
> > Yes, I think it's good enough. I don't think it's necessary to block a
> > non-browser window because user can't use them to browser the internet.
> >
> > >
> > >
> > https://codereview.chromium.org/2862653002/diff/80001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode100
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > > app_modal::AppModalDialog(
> > > On 2017/05/04 23:13:52, zmin wrote:
> > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > Why are you using app_modal_dialog here? I understand that you want
> > app
> > > modal,
> > > > > but you needn't use the AppModal class for that. Using the Appmodal
> > class
> > > ties
> > > > > you to WebContents, which I'm not sure you want.
> > > >
> > > > No, I don't want to be tied to WebContents. However, what i want is
> > this:
> > > >
> > >
> > https://cs.chromium.org/chromium/src/chrome/browser/
> > ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > > It prevent user open other browser window when the dialog is being
> > displayed.
> > > > And right now I only found AppModalDialog class can do so with
> > > > AppModalDialogQueue. User can still use other chrome window when a
> > normal
> > > window
> > > > modal dialog is opened.
> > > >
> > >
> > > Also, what happens if the browser you are anchored to ends up closing?
> > This
> > > could happen for a number of reasons.
> >
> > In general, there is always a default behavior(cancel) when dialog is
> > closed.
> > Because the dialog instance is stored in the browser process. So i guess
> > it will
> > always block all browser windows unless the browser process is terminated.
> > In
> > which cases, it doesn't matter anyway.
> >
> >
> >
> > https://codereview.chromium.org/2862653002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

sky

Thanks! So, I think if you want true app-modal you need to use another mechanism. ...

3 years, 7 months ago (2017-05-05 22:45:08 UTC) #38

Thanks!
So, I think if you want true app-modal you need to use another mechanism.
The mechanism you are using here is not necessarily app modal. It's only
modal against browsers. For examples, apps don't hit the code path you
mentioned.

  -Scott

On Fri, May 5, 2017 at 3:42 PM, <zmin@chromium.org> wrote:

> On 2017/05/05 22:32:20, sky wrote:
> > Sorry to ask this now, but is there a design doc for this? I'm wondering
> > why we are trying to block usage at all instead of presenting the warning
> > timer and forcing closing if the timer expires?
> Yes:
> https://docs.google.com/document/d/1cfDuV6n7puoWV2cMVI_LXlY8TOh1_
> jNulVPHyGjXW94/edit#heading=h.id227xaj6wxy
>
> User may leave his or her desk when the dialog is poped. That's why the
> timer is
> started only after the dialog is confirmed/canceled.
>
>
>
> > On Fri, May 5, 2017 at 3:22 PM, <mailto:zmin@chromium.org> wrote:
> >
> > > I have tested the dialog on Mac. The dialog itself looks good. But it
> > > seems that
> > > the app modal dialog doesn't block other browser window on Mac.
> > >
> > > I think it's not what I expected. So i remove Mac support for now.
> > >
> > >
> > > On 2017/05/05 00:00:10, sky wrote:
> > > >
> > > https://codereview.chromium.org/2862653002/diff/80001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > > > File chrome/browser/ui/views/profiles/force_signout_dialog.cc
> (right):
> > > >
> > > >
> > > https://codereview.chromium.org/2862653002/diff/80001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode38
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:38: bool
> > > > IsBrowserBelongedToProfile(Browser* browser, Profile* profile) {
> > > > On 2017/05/04 23:13:52, zmin wrote:
> > > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > > How about naming this IsMatchingBrowser and include the
> > > tab_strip_model
> > > > check
> > > > > > here too?
> > > > > Done.
> > > > > Two questions on the implementation:
> > > > > > . Do you really want to incognito windows here? It seems weird to
> > > anchor
> > > to
> > > > an
> > > > > > incognito window?
> > > > > > . Are you sure you don't want to look at the type? Anchoring to
> > > popups
> > > could
> > > > > be
> > > > > > weird.
> > > > > > . Don't you want to check visibility of the window?
> > > > >
> > > > > The purpose the dialog is warning the window closing. So
> > > > > Visibility, yes I'll check the visibility. it seems no harm to
> close
> > > if all
> > > > > windows are invisible.
> > > >
> > > > Be careful there. You should verify you get the right thing if the
> > > window is
> > > > minimized. I would say in that case you should force the window to be
> > > shown
> > > and
> > > > then show the dialog.
> > >
> > > I have tested that, this IsVisible function returns True for a
> minimized
> > > window.
> > >
> > > >
> > > > > Incognito and popups. I agree it's kinda weird. But since they
> will be
> > > closed
> > > > > later as well, I think it make sense to show the warning msg
> before.
> > > >
> > > >
> > > https://codereview.chromium.org/2862653002/diff/80001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode100
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > > > app_modal::AppModalDialog(
> > > > On 2017/05/04 23:13:52, zmin wrote:
> > > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > > Why are you using app_modal_dialog here? I understand that you
> want
> > > app
> > > > modal,
> > > > > > but you needn't use the AppModal class for that. Using the
> Appmodal
> > > class
> > > > ties
> > > > > > you to WebContents, which I'm not sure you want.
> > > > >
> > > > > No, I don't want to be tied to WebContents. However, what i want is
> > > this:
> > > > >
> > > >
> > > https://cs.chromium.org/chromium/src/chrome/browser/
> > > ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > > > It prevent user open other browser window when the dialog is being
> > > displayed.
> > > > > And right now I only found AppModalDialog class can do so with
> > > > > AppModalDialogQueue. User can still use other chrome window when a
> > > normal
> > > > window
> > > > > modal dialog is opened.
> > > > >
> > > >
> > > > Also, user could still interact with non-browser windows in Chrome.
> Are
> > > you
> > > sure
> > > > you want that?
> > >
> > > Yes, I think it's good enough. I don't think it's necessary to block a
> > > non-browser window because user can't use them to browser the internet.
> > >
> > > >
> > > >
> > > https://codereview.chromium.org/2862653002/diff/80001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode100
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > > > app_modal::AppModalDialog(
> > > > On 2017/05/04 23:13:52, zmin wrote:
> > > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > > Why are you using app_modal_dialog here? I understand that you
> want
> > > app
> > > > modal,
> > > > > > but you needn't use the AppModal class for that. Using the
> Appmodal
> > > class
> > > > ties
> > > > > > you to WebContents, which I'm not sure you want.
> > > > >
> > > > > No, I don't want to be tied to WebContents. However, what i want is
> > > this:
> > > > >
> > > >
> > > https://cs.chromium.org/chromium/src/chrome/browser/
> > > ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > > > It prevent user open other browser window when the dialog is being
> > > displayed.
> > > > > And right now I only found AppModalDialog class can do so with
> > > > > AppModalDialogQueue. User can still use other chrome window when a
> > > normal
> > > > window
> > > > > modal dialog is opened.
> > > > >
> > > >
> > > > Also, what happens if the browser you are anchored to ends up
> closing?
> > > This
> > > > could happen for a number of reasons.
> > >
> > > In general, there is always a default behavior(cancel) when dialog is
> > > closed.
> > > Because the dialog instance is stored in the browser process. So i
> guess
> > > it will
> > > always block all browser windows unless the browser process is
> terminated.
> > > In
> > > which cases, it doesn't matter anyway.
> > >
> > >
> > >
> > > https://codereview.chromium.org/2862653002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2862653002/
>

-- 
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.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-05 23:20:46 UTC) #39

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/446720)

3 years, 7 months ago (2017-05-05 23:20:47 UTC) #40

zmin

Is there any true app-modal dialog in chromium? Because this is the only dialog class ...

3 years, 7 months ago (2017-05-05 23:40:27 UTC) #41

Is there any true app-modal dialog in chromium? Because this is the only dialog
class I found when search AppModal in chromium.
If not, I think it's ok not to block app now. I can use other way to block app
if necessary.

On 2017/05/05 22:45:08, sky wrote:
> Thanks!
> So, I think if you want true app-modal you need to use another mechanism.
> The mechanism you are using here is not necessarily app modal. It's only
> modal against browsers. For examples, apps don't hit the code path you
> mentioned.
> 
>   -Scott
> 
> On Fri, May 5, 2017 at 3:42 PM, <mailto:zmin@chromium.org> wrote:
> 
> > On 2017/05/05 22:32:20, sky wrote:
> > > Sorry to ask this now, but is there a design doc for this? I'm wondering
> > > why we are trying to block usage at all instead of presenting the warning
> > > timer and forcing closing if the timer expires?
> > Yes:
> > https://docs.google.com/document/d/1cfDuV6n7puoWV2cMVI_LXlY8TOh1_
> > jNulVPHyGjXW94/edit#heading=h.id227xaj6wxy
> >
> > User may leave his or her desk when the dialog is poped. That's why the
> > timer is
> > started only after the dialog is confirmed/canceled.
> >
> >
> >
> > > On Fri, May 5, 2017 at 3:22 PM, <mailto:zmin@chromium.org> wrote:
> > >
> > > > I have tested the dialog on Mac. The dialog itself looks good. But it
> > > > seems that
> > > > the app modal dialog doesn't block other browser window on Mac.
> > > >
> > > > I think it's not what I expected. So i remove Mac support for now.
> > > >
> > > >
> > > > On 2017/05/05 00:00:10, sky wrote:
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/80001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > > > > File chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > (right):
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/80001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode38
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:38: bool
> > > > > IsBrowserBelongedToProfile(Browser* browser, Profile* profile) {
> > > > > On 2017/05/04 23:13:52, zmin wrote:
> > > > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > > > How about naming this IsMatchingBrowser and include the
> > > > tab_strip_model
> > > > > check
> > > > > > > here too?
> > > > > > Done.
> > > > > > Two questions on the implementation:
> > > > > > > . Do you really want to incognito windows here? It seems weird to
> > > > anchor
> > > > to
> > > > > an
> > > > > > > incognito window?
> > > > > > > . Are you sure you don't want to look at the type? Anchoring to
> > > > popups
> > > > could
> > > > > > be
> > > > > > > weird.
> > > > > > > . Don't you want to check visibility of the window?
> > > > > >
> > > > > > The purpose the dialog is warning the window closing. So
> > > > > > Visibility, yes I'll check the visibility. it seems no harm to
> > close
> > > > if all
> > > > > > windows are invisible.
> > > > >
> > > > > Be careful there. You should verify you get the right thing if the
> > > > window is
> > > > > minimized. I would say in that case you should force the window to be
> > > > shown
> > > > and
> > > > > then show the dialog.
> > > >
> > > > I have tested that, this IsVisible function returns True for a
> > minimized
> > > > window.
> > > >
> > > > >
> > > > > > Incognito and popups. I agree it's kinda weird. But since they
> > will be
> > > > closed
> > > > > > later as well, I think it make sense to show the warning msg
> > before.
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/80001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode100
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > > > > app_modal::AppModalDialog(
> > > > > On 2017/05/04 23:13:52, zmin wrote:
> > > > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > > > Why are you using app_modal_dialog here? I understand that you
> > want
> > > > app
> > > > > modal,
> > > > > > > but you needn't use the AppModal class for that. Using the
> > Appmodal
> > > > class
> > > > > ties
> > > > > > > you to WebContents, which I'm not sure you want.
> > > > > >
> > > > > > No, I don't want to be tied to WebContents. However, what i want is
> > > > this:
> > > > > >
> > > > >
> > > > https://cs.chromium.org/chromium/src/chrome/browser/
> > > > ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > > > > It prevent user open other browser window when the dialog is being
> > > > displayed.
> > > > > > And right now I only found AppModalDialog class can do so with
> > > > > > AppModalDialogQueue. User can still use other chrome window when a
> > > > normal
> > > > > window
> > > > > > modal dialog is opened.
> > > > > >
> > > > >
> > > > > Also, user could still interact with non-browser windows in Chrome.
> > Are
> > > > you
> > > > sure
> > > > > you want that?
> > > >
> > > > Yes, I think it's good enough. I don't think it's necessary to block a
> > > > non-browser window because user can't use them to browser the internet.
> > > >
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/80001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode100
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > > > > app_modal::AppModalDialog(
> > > > > On 2017/05/04 23:13:52, zmin wrote:
> > > > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > > > Why are you using app_modal_dialog here? I understand that you
> > want
> > > > app
> > > > > modal,
> > > > > > > but you needn't use the AppModal class for that. Using the
> > Appmodal
> > > > class
> > > > > ties
> > > > > > > you to WebContents, which I'm not sure you want.
> > > > > >
> > > > > > No, I don't want to be tied to WebContents. However, what i want is
> > > > this:
> > > > > >
> > > > >
> > > > https://cs.chromium.org/chromium/src/chrome/browser/
> > > > ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > > > > It prevent user open other browser window when the dialog is being
> > > > displayed.
> > > > > > And right now I only found AppModalDialog class can do so with
> > > > > > AppModalDialogQueue. User can still use other chrome window when a
> > > > normal
> > > > > window
> > > > > > modal dialog is opened.
> > > > > >
> > > > >
> > > > > Also, what happens if the browser you are anchored to ends up
> > closing?
> > > > This
> > > > > could happen for a number of reasons.
> > > >
> > > > In general, there is always a default behavior(cancel) when dialog is
> > > > closed.
> > > > Because the dialog instance is stored in the browser process. So i
> > guess
> > > > it will
> > > > always block all browser windows unless the browser process is
> > terminated.
> > > > In
> > > > which cases, it doesn't matter anyway.
> > > >
> > > >
> > > >
> > > > https://codereview.chromium.org/2862653002/
> > > >
> > >
> > > --
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> > https://codereview.chromium.org/2862653002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

sky

On windows I don't believe we have anything app modal. Why is it ok to ...

3 years, 7 months ago (2017-05-07 22:45:54 UTC) #42

On windows I don't believe we have anything app modal.

Why is it ok to ignore app windows and other random dialogs that may be
created?

  -Scott

On Fri, May 5, 2017 at 4:40 PM, <zmin@chromium.org> wrote:

> Is there any true app-modal dialog in chromium? Because this is the only
> dialog
> class I found when search AppModal in chromium.
> If not, I think it's ok not to block app now. I can use other way to block
> app
> if necessary.
>
> On 2017/05/05 22:45:08, sky wrote:
> > Thanks!
> > So, I think if you want true app-modal you need to use another mechanism.
> > The mechanism you are using here is not necessarily app modal. It's only
> > modal against browsers. For examples, apps don't hit the code path you
> > mentioned.
> >
> > -Scott
> >
> > On Fri, May 5, 2017 at 3:42 PM, <mailto:zmin@chromium.org> wrote:
> >
> > > On 2017/05/05 22:32:20, sky wrote:
> > > > Sorry to ask this now, but is there a design doc for this? I'm
> wondering
> > > > why we are trying to block usage at all instead of presenting the
> warning
> > > > timer and forcing closing if the timer expires?
> > > Yes:
> > > https://docs.google.com/document/d/1cfDuV6n7puoWV2cMVI_LXlY8TOh1_
> > > jNulVPHyGjXW94/edit#heading=h.id227xaj6wxy
> > >
> > > User may leave his or her desk when the dialog is poped. That's why the
> > > timer is
> > > started only after the dialog is confirmed/canceled.
> > >
> > >
> > >
> > > > On Fri, May 5, 2017 at 3:22 PM, <mailto:zmin@chromium.org> wrote:
> > > >
> > > > > I have tested the dialog on Mac. The dialog itself looks good. But
> it
> > > > > seems that
> > > > > the app modal dialog doesn't block other browser window on Mac.
> > > > >
> > > > > I think it's not what I expected. So i remove Mac support for now.
> > > > >
> > > > >
> > > > > On 2017/05/05 00:00:10, sky wrote:
> > > > > >
> > > > > https://codereview.chromium.org/2862653002/diff/80001/
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > > > > > File chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > > (right):
> > > > > >
> > > > > >
> > > > > https://codereview.chromium.org/2862653002/diff/80001/
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode38
> > > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:38:
> bool
> > > > > > IsBrowserBelongedToProfile(Browser* browser, Profile* profile) {
> > > > > > On 2017/05/04 23:13:52, zmin wrote:
> > > > > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > > > > How about naming this IsMatchingBrowser and include the
> > > > > tab_strip_model
> > > > > > check
> > > > > > > > here too?
> > > > > > > Done.
> > > > > > > Two questions on the implementation:
> > > > > > > > . Do you really want to incognito windows here? It seems
> weird to
> > > > > anchor
> > > > > to
> > > > > > an
> > > > > > > > incognito window?
> > > > > > > > . Are you sure you don't want to look at the type? Anchoring
> to
> > > > > popups
> > > > > could
> > > > > > > be
> > > > > > > > weird.
> > > > > > > > . Don't you want to check visibility of the window?
> > > > > > >
> > > > > > > The purpose the dialog is warning the window closing. So
> > > > > > > Visibility, yes I'll check the visibility. it seems no harm to
> > > close
> > > > > if all
> > > > > > > windows are invisible.
> > > > > >
> > > > > > Be careful there. You should verify you get the right thing if
> the
> > > > > window is
> > > > > > minimized. I would say in that case you should force the window
> to be
> > > > > shown
> > > > > and
> > > > > > then show the dialog.
> > > > >
> > > > > I have tested that, this IsVisible function returns True for a
> > > minimized
> > > > > window.
> > > > >
> > > > > >
> > > > > > > Incognito and popups. I agree it's kinda weird. But since they
> > > will be
> > > > > closed
> > > > > > > later as well, I think it make sense to show the warning msg
> > > before.
> > > > > >
> > > > > >
> > > > > https://codereview.chromium.org/2862653002/diff/80001/
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.
> cc#newcode100
> > > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > > > > > app_modal::AppModalDialog(
> > > > > > On 2017/05/04 23:13:52, zmin wrote:
> > > > > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > > > > Why are you using app_modal_dialog here? I understand that
> you
> > > want
> > > > > app
> > > > > > modal,
> > > > > > > > but you needn't use the AppModal class for that. Using the
> > > Appmodal
> > > > > class
> > > > > > ties
> > > > > > > > you to WebContents, which I'm not sure you want.
> > > > > > >
> > > > > > > No, I don't want to be tied to WebContents. However, what i
> want is
> > > > > this:
> > > > > > >
> > > > > >
> > > > > https://cs.chromium.org/chromium/src/chrome/browser/
> > > > > ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > > > > > It prevent user open other browser window when the dialog is
> being
> > > > > displayed.
> > > > > > > And right now I only found AppModalDialog class can do so with
> > > > > > > AppModalDialogQueue. User can still use other chrome window
> when a
> > > > > normal
> > > > > > window
> > > > > > > modal dialog is opened.
> > > > > > >
> > > > > >
> > > > > > Also, user could still interact with non-browser windows in
> Chrome.
> > > Are
> > > > > you
> > > > > sure
> > > > > > you want that?
> > > > >
> > > > > Yes, I think it's good enough. I don't think it's necessary to
> block a
> > > > > non-browser window because user can't use them to browser the
> internet.
> > > > >
> > > > > >
> > > > > >
> > > > > https://codereview.chromium.org/2862653002/diff/80001/
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.
> cc#newcode100
> > > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:100: :
> > > > > > app_modal::AppModalDialog(
> > > > > > On 2017/05/04 23:13:52, zmin wrote:
> > > > > > > On 2017/05/04 03:45:11, sky wrote:
> > > > > > > > Why are you using app_modal_dialog here? I understand that
> you
> > > want
> > > > > app
> > > > > > modal,
> > > > > > > > but you needn't use the AppModal class for that. Using the
> > > Appmodal
> > > > > class
> > > > > > ties
> > > > > > > > you to WebContents, which I'm not sure you want.
> > > > > > >
> > > > > > > No, I don't want to be tied to WebContents. However, what i
> want is
> > > > > this:
> > > > > > >
> > > > > >
> > > > > https://cs.chromium.org/chromium/src/chrome/browser/
> > > > > ui/views/frame/browser_view.cc?sq=package:chromium&dr&l=1584
> > > > > > > It prevent user open other browser window when the dialog is
> being
> > > > > displayed.
> > > > > > > And right now I only found AppModalDialog class can do so with
> > > > > > > AppModalDialogQueue. User can still use other chrome window
> when a
> > > > > normal
> > > > > > window
> > > > > > > modal dialog is opened.
> > > > > > >
> > > > > >
> > > > > > Also, what happens if the browser you are anchored to ends up
> > > closing?
> > > > > This
> > > > > > could happen for a number of reasons.
> > > > >
> > > > > In general, there is always a default behavior(cancel) when dialog
> is
> > > > > closed.
> > > > > Because the dialog instance is stored in the browser process. So i
> > > guess
> > > > > it will
> > > > > always block all browser windows unless the browser process is
> > > terminated.
> > > > > In
> > > > > which cases, it doesn't matter anyway.
> > > > >
> > > > >
> > > > >
> > > > > https://codereview.chromium.org/2862653002/
> > > > >
> > > >
> > > > --
> > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > >
> > >
> > > https://codereview.chromium.org/2862653002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2862653002/
>

-- 
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.

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-08 19:42:51 UTC) #43

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/140001

3 years, 7 months ago (2017-05-08 19:43:45 UTC) #44

zmin

On 2017/05/07 22:45:54, sky wrote: > On windows I don't believe we have anything app ...

3 years, 7 months ago (2017-05-08 20:25:52 UTC) #45

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-08 21:29:59 UTC) #46

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/447986)

3 years, 7 months ago (2017-05-08 21:30:00 UTC) #47

sky

Fair enough. My concern then is that you are using the existing appmodal code, which ...

3 years, 7 months ago (2017-05-08 21:48:45 UTC) #48

chromium-reviews

Yes, I agree tie this dialog to a WebContents is a weird logic. However, app_modal ...

3 years, 7 months ago (2017-05-08 22:37:47 UTC) #49

Yes, I agree tie this dialog to a WebContents is a weird logic. However,
app_modal dialog is the only dialog I found that will prevent other browser
windows being activated. For normal window type modal dialog, user can
ignore the dialog by opening a new browser window.

I think for now, implement this appmodal dialog means I can reuse the code
related to browser window activation. And I think it's only used to find
the Browser instance outside the dialog. For long term, I think we might
need to either create another app_modal dialog associated with Browser
instead of WebContents. Or decouple the WebContent from app_modal dialog
and move it to javascript_app_modal_dialog

On Mon, May 8, 2017 at 5:48 PM, Scott Violet <sky@chromium.org> wrote:

> Fair enough. My concern then is that you are using the existing appmodal
> code, which is intended for use with a WebContents. That doesn't make sense
> in the context you are trying to use it.
>
> On Mon, May 8, 2017 at 1:25 PM, <zmin@chromium.org> wrote:
>
>> On 2017/05/07 22:45:54, sky wrote:
>> > On windows I don't believe we have anything app modal.
>> >
>> > Why is it ok to ignore app windows and other random dialogs that may be
>> > created?
>>
>> It will be awesome if there is a true app modal dialog. However,
>> I don't feel it's necessary to create a one here.
>>
>> Compare to the browser window, the thing user can do with a app window or
>> a
>> random dialog is limited.
>> In the other word, a user have to try very hard to escape the policies
>> and break
>> the force sign in system. Then he/she get a small
>> part of Chrome.
>>
>> That's why I think it's not a critical issue for now so that it won't
>> block the
>> launch of the project. And once the feature is online, it's also
>> more easy for me to find a proper solution here with user feedback.
>>
>>
>>
>>
>>
>> https://codereview.chromium.org/2862653002/
>>
>
>

-- 
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.

sky

You are using code that happens to provide functionality close to what you want, even ...

3 years, 7 months ago (2017-05-08 22:54:52 UTC) #50

You are using code that happens to provide functionality close to what you
want, even though it was designed for something else and hasn't different
constraints. That isn't the right thing for long term maintainability. Your
code should *not* be tied to extracting a WebContents from a Browser.

  -Scott

On Mon, May 8, 2017 at 3:37 PM, Owen Min <zmin@google.com> wrote:

> Yes, I agree tie this dialog to a WebContents is a weird logic. However,
> app_modal dialog is the only dialog I found that will prevent other browser
> windows being activated. For normal window type modal dialog, user can
> ignore the dialog by opening a new browser window.
>
> I think for now, implement this appmodal dialog means I can reuse the code
> related to browser window activation. And I think it's only used to find
> the Browser instance outside the dialog. For long term, I think we might
> need to either create another app_modal dialog associated with Browser
> instead of WebContents. Or decouple the WebContent from app_modal dialog
> and move it to javascript_app_modal_dialog
>
> On Mon, May 8, 2017 at 5:48 PM, Scott Violet <sky@chromium.org> wrote:
>
>> Fair enough. My concern then is that you are using the existing appmodal
>> code, which is intended for use with a WebContents. That doesn't make sense
>> in the context you are trying to use it.
>>
>> On Mon, May 8, 2017 at 1:25 PM, <zmin@chromium.org> wrote:
>>
>>> On 2017/05/07 22:45:54, sky wrote:
>>> > On windows I don't believe we have anything app modal.
>>> >
>>> > Why is it ok to ignore app windows and other random dialogs that may be
>>> > created?
>>>
>>> It will be awesome if there is a true app modal dialog. However,
>>> I don't feel it's necessary to create a one here.
>>>
>>> Compare to the browser window, the thing user can do with a app window
>>> or a
>>> random dialog is limited.
>>> In the other word, a user have to try very hard to escape the policies
>>> and break
>>> the force sign in system. Then he/she get a small
>>> part of Chrome.
>>>
>>> That's why I think it's not a critical issue for now so that it won't
>>> block the
>>> launch of the project. And once the feature is online, it's also
>>> more easy for me to find a proper solution here with user feedback.
>>>
>>>
>>>
>>>
>>>
>>> https://codereview.chromium.org/2862653002/
>>>
>>
>>
>

-- 
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.

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-09 22:34:53 UTC) #51

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/160001

3 years, 7 months ago (2017-05-09 22:36:42 UTC) #52

zmin

On 2017/05/08 22:54:52, sky wrote: > You are using code that happens to provide functionality ...

3 years, 7 months ago (2017-05-09 22:36:48 UTC) #53

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-09 23:12:22 UTC) #54

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/264333) ios-simulator on ...

3 years, 7 months ago (2017-05-09 23:12:23 UTC) #55

sky

+avi for app_modal below. zmin: don't go back to app_modal until you here from Avi. ...

3 years, 7 months ago (2017-05-10 15:43:26 UTC) #57

zmin

On 2017/05/10 15:43:26, sky wrote: > +avi for app_modal below. > zmin: don't go back ...

3 years, 7 months ago (2017-05-10 18:33:28 UTC) #58

Avi (use Gerrit)

The notion of extending app_modal and using app-modal dialogs does not sit well with me ...

3 years, 7 months ago (2017-05-10 20:05:05 UTC) #59

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-10 21:40:13 UTC) #60

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/180001

3 years, 7 months ago (2017-05-10 21:41:27 UTC) #61

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/265317) android_clang_dbg_recipe on ...

3 years, 7 months ago (2017-05-10 21:46:44 UTC) #63

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-10 21:53:14 UTC) #64

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/200001

3 years, 7 months ago (2017-05-10 21:54:01 UTC) #65

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-10 22:12:24 UTC) #66

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/293934)

3 years, 7 months ago (2017-05-10 22:12:25 UTC) #67

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-10 22:35:32 UTC) #69

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/220001

3 years, 7 months ago (2017-05-10 22:36:56 UTC) #70

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-11 00:51:11 UTC) #71

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/441293)

3 years, 7 months ago (2017-05-11 00:51:12 UTC) #72

sky

Ok, thanks Avi. I'll remove you as a reviewer as this won't use app_modal.

3 years, 7 months ago (2017-05-12 13:13:27 UTC) #73

sky

Description was changed from ========== If force-sign-in policy is enabled, popup warning dialog before window ...

3 years, 7 months ago (2017-05-12 13:13:43 UTC) #74

sky

https://codereview.chromium.org/2862653002/diff/220001/chrome/browser/ui/views/frame/browser_view.cc File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2862653002/diff/220001/chrome/browser/ui/views/frame/browser_view.cc#newcode1587 chrome/browser/ui/views/frame/browser_view.cc:1587: if ((!queue->active_dialog() || !queue->active_dialog()->native_dialog() || Continually adding dependencies from ...

3 years, 7 months ago (2017-05-12 13:15:16 UTC) #76

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-15 21:56:42 UTC) #77

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/240001

3 years, 7 months ago (2017-05-15 21:57:25 UTC) #78

zmin

On 2017/05/12 13:15:16, sky wrote: > https://codereview.chromium.org/2862653002/diff/220001/chrome/browser/ui/views/frame/browser_view.cc > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/2862653002/diff/220001/chrome/browser/ui/views/frame/browser_view.cc#newcode1587 > ...

3 years, 7 months ago (2017-05-15 21:58:36 UTC) #79

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-15 22:18:05 UTC) #80

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/297104)

3 years, 7 months ago (2017-05-15 22:18:06 UTC) #81

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-15 22:50:08 UTC) #83

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/260001

3 years, 7 months ago (2017-05-15 22:51:35 UTC) #84

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-16 02:25:19 UTC) #85

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/427373)

3 years, 7 months ago (2017-05-16 02:25:20 UTC) #86

sky

https://codereview.chromium.org/2862653002/diff/260001/chrome/browser/ui/views/browser_modal_dialog.cc File chrome/browser/ui/views/browser_modal_dialog.cc (right): https://codereview.chromium.org/2862653002/diff/260001/chrome/browser/ui/views/browser_modal_dialog.cc#newcode30 chrome/browser/ui/views/browser_modal_dialog.cc:30: if (it != dialogs_.end()) { no {} https://codereview.chromium.org/2862653002/diff/260001/chrome/browser/ui/views/browser_modal_dialog.h File ...

3 years, 7 months ago (2017-05-16 12:40:33 UTC) #87

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-16 22:11:44 UTC) #88

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/280001

3 years, 7 months ago (2017-05-16 22:13:48 UTC) #89

zmin

https://codereview.chromium.org/2862653002/diff/260001/chrome/browser/ui/views/browser_modal_dialog.cc File chrome/browser/ui/views/browser_modal_dialog.cc (right): https://codereview.chromium.org/2862653002/diff/260001/chrome/browser/ui/views/browser_modal_dialog.cc#newcode30 chrome/browser/ui/views/browser_modal_dialog.cc:30: if (it != dialogs_.end()) { On 2017/05/16 12:40:32, sky ...

3 years, 7 months ago (2017-05-16 22:22:02 UTC) #90

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-16 23:27:47 UTC) #91

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/456555)

3 years, 7 months ago (2017-05-16 23:27:48 UTC) #92

sky

https://codereview.chromium.org/2862653002/diff/280001/chrome/app/google_chrome_strings.grd File chrome/app/google_chrome_strings.grd (right): https://codereview.chromium.org/2862653002/diff/280001/chrome/app/google_chrome_strings.grd#newcode766 chrome/app/google_chrome_strings.grd:766: Close all Google Chrome windows and sign in now ...

3 years, 7 months ago (2017-05-17 18:03:18 UTC) #93

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-18 03:28:22 UTC) #94

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/300001

3 years, 7 months ago (2017-05-18 03:29:53 UTC) #95

zmin

https://codereview.chromium.org/2862653002/diff/280001/chrome/app/google_chrome_strings.grd File chrome/app/google_chrome_strings.grd (right): https://codereview.chromium.org/2862653002/diff/280001/chrome/app/google_chrome_strings.grd#newcode766 chrome/app/google_chrome_strings.grd:766: Close all Google Chrome windows and sign in now ...

3 years, 7 months ago (2017-05-18 03:31:18 UTC) #96

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-18 05:50:13 UTC) #97

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-18 05:50:14 UTC) #98

sky

https://codereview.chromium.org/2862653002/diff/280001/chrome/app/google_chrome_strings.grd File chrome/app/google_chrome_strings.grd (right): https://codereview.chromium.org/2862653002/diff/280001/chrome/app/google_chrome_strings.grd#newcode766 chrome/app/google_chrome_strings.grd:766: Close all Google Chrome windows and sign in now ...

3 years, 7 months ago (2017-05-18 16:50:51 UTC) #99

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-05 19:58:36 UTC) #101

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/340001

3 years, 6 months ago (2017-06-05 19:59:11 UTC) #102

zmin

Hi Scott, I have chatted with the UX. And based on our discussion, here is ...

3 years, 6 months ago (2017-06-05 20:23:58 UTC) #104

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-05 20:27:22 UTC) #105

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/360001

3 years, 6 months ago (2017-06-05 20:27:53 UTC) #106

sky

https://codereview.chromium.org/2862653002/diff/360001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2862653002/diff/360001/chrome/app/generated_resources.grd#newcode8108 chrome/app/generated_resources.grd:8108: <message name="IDS_ENTERPRISE_FORCE_SIGNOUT_CLOSE_CONFIRM" desc="Text of the button to confirm close ...

3 years, 6 months ago (2017-06-05 23:43:19 UTC) #107

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-06 01:09:05 UTC) #108

commit-bot: I haz the power

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_android_rel_ng/builds/310927)

3 years, 6 months ago (2017-06-06 01:09:06 UTC) #109

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-06 20:59:19 UTC) #110

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/380001

3 years, 6 months ago (2017-06-06 20:59:47 UTC) #111

zmin

https://codereview.chromium.org/2862653002/diff/360001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2862653002/diff/360001/chrome/app/generated_resources.grd#newcode8108 chrome/app/generated_resources.grd:8108: <message name="IDS_ENTERPRISE_FORCE_SIGNOUT_CLOSE_CONFIRM" desc="Text of the button to confirm close ...

3 years, 6 months ago (2017-06-06 21:00:44 UTC) #112

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-06 22:35:55 UTC) #113

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-06 22:35:56 UTC) #114

sky

https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/views/profiles/force_signout_dialog.cc File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right): https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode198 chrome/browser/ui/views/profiles/force_signout_dialog.cc:198: views::GridLayout* dialog_layout = new views::GridLayout(this); On 2017/06/06 21:00:43, zmin ...

3 years, 6 months ago (2017-06-06 23:10:39 UTC) #115

https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):

https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/force_signout_dialog.cc:198: views::GridLayout*
dialog_layout = new views::GridLayout(this);
On 2017/06/06 21:00:43, zmin wrote:
> On 2017/06/05 23:43:18, sky wrote:
> > Why do you need two GridLayouts?
> 
> dialog_layout is the global layout for the whole dialog.
> 
> The additional layout(prompt_layout) is for the sub-view that contains the
> prompt_label view. The sub-view has different background color, padding and
> border. 
> 
> I just removed the sub-view with its layout and append the label to the dialog
> layout directly.
> 
> Is multiple levels of layout bad for the performance?

No, I just wanted to understand why it was necessary. Often times it isn't.

https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/force_signout_dialog.cc:244: return
close_timer_->desired_run_time() - base::TimeTicks::Now();
On 2017/06/06 21:00:43, zmin wrote:
> On 2017/06/05 23:43:18, sky wrote:
> > To avoid possible weird bugs I recommend making this negative.
> 
> Why negative result will avoid weird bug, is it because the overflow when
> desired_run_time is less than current time?
> 
> How about I just make sure it always return non-negative number?

Sorry, my comment was really bad here. I meant it seems less error prone to
ensure this doesn't go negative. But I think TimeDelta hides that, so ignore
this comment. Sorry.

https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/force_signout_dialog.h (right):

https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/force_signout_dialog.h:28: class
ForceSignoutDialog : public views::DialogDelegateView {
On 2017/06/06 21:00:44, zmin wrote:
> On 2017/06/05 23:43:18, sky wrote:
> > WDYT of naming this ReauthenticateDialog?
> 
> There're lots of different situations requires reauth, how about
> ForceReauthenticateDialog

ForcedReauthenticationDialog?

https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/force_signout_dialog.h:34: base::Timer*
close_timer);
On 2017/06/06 21:00:43, zmin wrote:
> On 2017/06/05 23:43:18, sky wrote:
> > Document ownership. Also, why does the caller need to supply a timer?
> 
> The |close_timer| is used to track the remaining time mainly. 
> It will also be used to 
> 1) Stop the timer if there is no opened browser window as we can sign out user
> without delay.
> 2) Close the dialog if the timer is stopped

I get that you want a close_timer, but why does it need to be passed in? Can't
the dialog own and creates it's own timer?

zmin

On 2017/06/06 23:10:39, sky wrote: > https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/views/profiles/force_signout_dialog.cc > File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right): > > https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode198 > ...

3 years, 6 months ago (2017-06-07 00:00:38 UTC) #116

On 2017/06/06 23:10:39, sky wrote:
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):
> 
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> chrome/browser/ui/views/profiles/force_signout_dialog.cc:198:
views::GridLayout*
> dialog_layout = new views::GridLayout(this);
> On 2017/06/06 21:00:43, zmin wrote:
> > On 2017/06/05 23:43:18, sky wrote:
> > > Why do you need two GridLayouts?
> > 
> > dialog_layout is the global layout for the whole dialog.
> > 
> > The additional layout(prompt_layout) is for the sub-view that contains the
> > prompt_label view. The sub-view has different background color, padding and
> > border. 
> > 
> > I just removed the sub-view with its layout and append the label to the
dialog
> > layout directly.
> > 
> > Is multiple levels of layout bad for the performance?
> 
> No, I just wanted to understand why it was necessary. Often times it isn't.
> 
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> chrome/browser/ui/views/profiles/force_signout_dialog.cc:244: return
> close_timer_->desired_run_time() - base::TimeTicks::Now();
> On 2017/06/06 21:00:43, zmin wrote:
> > On 2017/06/05 23:43:18, sky wrote:
> > > To avoid possible weird bugs I recommend making this negative.
> > 
> > Why negative result will avoid weird bug, is it because the overflow when
> > desired_run_time is less than current time?
> > 
> > How about I just make sure it always return non-negative number?
> 
> Sorry, my comment was really bad here. I meant it seems less error prone to
> ensure this doesn't go negative. But I think TimeDelta hides that, so ignore
> this comment. Sorry.
> 
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> File chrome/browser/ui/views/profiles/force_signout_dialog.h (right):
> 
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> chrome/browser/ui/views/profiles/force_signout_dialog.h:28: class
> ForceSignoutDialog : public views::DialogDelegateView {
> On 2017/06/06 21:00:44, zmin wrote:
> > On 2017/06/05 23:43:18, sky wrote:
> > > WDYT of naming this ReauthenticateDialog?
> > 
> > There're lots of different situations requires reauth, how about
> > ForceReauthenticateDialog
> 
> ForcedReauthenticationDialog?
Sure, I can do that.


>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> chrome/browser/ui/views/profiles/force_signout_dialog.h:34: base::Timer*
> close_timer);
> On 2017/06/06 21:00:43, zmin wrote:
> > On 2017/06/05 23:43:18, sky wrote:
> > > Document ownership. Also, why does the caller need to supply a timer?
> > 
> > The |close_timer| is used to track the remaining time mainly. 
> > It will also be used to 
> > 1) Stop the timer if there is no opened browser window as we can sign out
user
> > without delay.
> > 2) Close the dialog if the timer is stopped
> 
> I get that you want a close_timer, but why does it need to be passed in? Can't
> the dialog own and creates it's own timer?

close_timer(5 minutes countdown) will continue after user close the dialog. 
So at least dialog can't own the timer.

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-07 16:23:25 UTC) #117

zmin

On 2017/06/07 00:00:38, zmin wrote: > On 2017/06/06 23:10:39, sky wrote: > > > https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/views/profiles/force_signout_dialog.cc ...

3 years, 6 months ago (2017-06-07 16:23:59 UTC) #118

On 2017/06/07 00:00:38, zmin wrote:
> On 2017/06/06 23:10:39, sky wrote:
> >
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> > File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):
> > 
> >
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc:198:
> views::GridLayout*
> > dialog_layout = new views::GridLayout(this);
> > On 2017/06/06 21:00:43, zmin wrote:
> > > On 2017/06/05 23:43:18, sky wrote:
> > > > Why do you need two GridLayouts?
> > > 
> > > dialog_layout is the global layout for the whole dialog.
> > > 
> > > The additional layout(prompt_layout) is for the sub-view that contains the
> > > prompt_label view. The sub-view has different background color, padding
and
> > > border. 
> > > 
> > > I just removed the sub-view with its layout and append the label to the
> dialog
> > > layout directly.
> > > 
> > > Is multiple levels of layout bad for the performance?
> > 
> > No, I just wanted to understand why it was necessary. Often times it isn't.
> > 
> >
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc:244: return
> > close_timer_->desired_run_time() - base::TimeTicks::Now();
> > On 2017/06/06 21:00:43, zmin wrote:
> > > On 2017/06/05 23:43:18, sky wrote:
> > > > To avoid possible weird bugs I recommend making this negative.
> > > 
> > > Why negative result will avoid weird bug, is it because the overflow when
> > > desired_run_time is less than current time?
> > > 
> > > How about I just make sure it always return non-negative number?
> > 
> > Sorry, my comment was really bad here. I meant it seems less error prone to
> > ensure this doesn't go negative. But I think TimeDelta hides that, so ignore
> > this comment. Sorry.
> > 
> >
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> > File chrome/browser/ui/views/profiles/force_signout_dialog.h (right):
> > 
> >
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> > chrome/browser/ui/views/profiles/force_signout_dialog.h:28: class
> > ForceSignoutDialog : public views::DialogDelegateView {
> > On 2017/06/06 21:00:44, zmin wrote:
> > > On 2017/06/05 23:43:18, sky wrote:
> > > > WDYT of naming this ReauthenticateDialog?
> > > 
> > > There're lots of different situations requires reauth, how about
> > > ForceReauthenticateDialog
> > 
> > ForcedReauthenticationDialog?
> Sure, I can do that.
Done.
> 
> >
>
https://codereview.chromium.org/2862653002/diff/360001/chrome/browser/ui/view...
> > chrome/browser/ui/views/profiles/force_signout_dialog.h:34: base::Timer*
> > close_timer);
> > On 2017/06/06 21:00:43, zmin wrote:
> > > On 2017/06/05 23:43:18, sky wrote:
> > > > Document ownership. Also, why does the caller need to supply a timer?
> > > 
> > > The |close_timer| is used to track the remaining time mainly. 
> > > It will also be used to 
> > > 1) Stop the timer if there is no opened browser window as we can sign out
> user
> > > without delay.
> > > 2) Close the dialog if the timer is stopped
> > 
> > I get that you want a close_timer, but why does it need to be passed in?
Can't
> > the dialog own and creates it's own timer?
> 
> close_timer(5 minutes countdown) will continue after user close the dialog. 
> So at least dialog can't own the timer.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/400001

3 years, 6 months ago (2017-06-07 16:24:02 UTC) #119

sky

On Tue, Jun 6, 2017 at 5:00 PM, <zmin@chromium.org> wrote: > On 2017/06/06 23:10:39, sky ...

3 years, 6 months ago (2017-06-07 16:51:57 UTC) #120

On Tue, Jun 6, 2017 at 5:00 PM, <zmin@chromium.org> wrote:

> On 2017/06/06 23:10:39, sky wrote:
> >
> https://codereview.chromium.org/2862653002/diff/360001/
> chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):
> >
> >
> https://codereview.chromium.org/2862653002/diff/360001/
> chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode198
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc:198:
> views::GridLayout*
> > dialog_layout = new views::GridLayout(this);
> > On 2017/06/06 21:00:43, zmin wrote:
> > > On 2017/06/05 23:43:18, sky wrote:
> > > > Why do you need two GridLayouts?
> > >
> > > dialog_layout is the global layout for the whole dialog.
> > >
> > > The additional layout(prompt_layout) is for the sub-view that contains
> the
> > > prompt_label view. The sub-view has different background color,
> padding and
> > > border.
> > >
> > > I just removed the sub-view with its layout and append the label to the
> dialog
> > > layout directly.
> > >
> > > Is multiple levels of layout bad for the performance?
> >
> > No, I just wanted to understand why it was necessary. Often times it
> isn't.
> >
> >
> https://codereview.chromium.org/2862653002/diff/360001/
> chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode244
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc:244: return
> > close_timer_->desired_run_time() - base::TimeTicks::Now();
> > On 2017/06/06 21:00:43, zmin wrote:
> > > On 2017/06/05 23:43:18, sky wrote:
> > > > To avoid possible weird bugs I recommend making this negative.
> > >
> > > Why negative result will avoid weird bug, is it because the overflow
> when
> > > desired_run_time is less than current time?
> > >
> > > How about I just make sure it always return non-negative number?
> >
> > Sorry, my comment was really bad here. I meant it seems less error prone
> to
> > ensure this doesn't go negative. But I think TimeDelta hides that, so
> ignore
> > this comment. Sorry.
> >
> >
> https://codereview.chromium.org/2862653002/diff/360001/
> chrome/browser/ui/views/profiles/force_signout_dialog.h
> > File chrome/browser/ui/views/profiles/force_signout_dialog.h (right):
> >
> >
> https://codereview.chromium.org/2862653002/diff/360001/
> chrome/browser/ui/views/profiles/force_signout_dialog.h#newcode28
> > chrome/browser/ui/views/profiles/force_signout_dialog.h:28: class
> > ForceSignoutDialog : public views::DialogDelegateView {
> > On 2017/06/06 21:00:44, zmin wrote:
> > > On 2017/06/05 23:43:18, sky wrote:
> > > > WDYT of naming this ReauthenticateDialog?
> > >
> > > There're lots of different situations requires reauth, how about
> > > ForceReauthenticateDialog
> >
> > ForcedReauthenticationDialog?
> Sure, I can do that.
>
>
> >
> https://codereview.chromium.org/2862653002/diff/360001/
> chrome/browser/ui/views/profiles/force_signout_dialog.h#newcode34
> > chrome/browser/ui/views/profiles/force_signout_dialog.h:34: base::Timer*
> > close_timer);
> > On 2017/06/06 21:00:43, zmin wrote:
> > > On 2017/06/05 23:43:18, sky wrote:
> > > > Document ownership. Also, why does the caller need to supply a timer?
> > >
> > > The |close_timer| is used to track the remaining time mainly.
> > > It will also be used to
> > > 1) Stop the timer if there is no opened browser window as we can sign
> out
> user
> > > without delay.
> > > 2) Close the dialog if the timer is stopped
> >
> > I get that you want a close_timer, but why does it need to be passed in?
> Can't
> > the dialog own and creates it's own timer?
>
> close_timer(5 minutes countdown) will continue after user close the
> dialog.
> So at least dialog can't own the timer.
>

Why does that mean the dialog needs to share the same timer? If the caller
needs to maintain a timer, can't it do that?

  -Scott


>
>
> https://codereview.chromium.org/2862653002/
>

-- 
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.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-07 18:19:56 UTC) #121

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-07 18:19:58 UTC) #122

zmin

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-07 19:09:23 UTC) #123

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/420001

3 years, 6 months ago (2017-06-07 19:10:01 UTC) #124

zmin

On 2017/06/07 16:51:57, sky wrote: > On Tue, Jun 6, 2017 at 5:00 PM, <mailto:zmin@chromium.org> ...

3 years, 6 months ago (2017-06-07 19:17:18 UTC) #125

On 2017/06/07 16:51:57, sky wrote:
> On Tue, Jun 6, 2017 at 5:00 PM, <mailto:zmin@chromium.org> wrote:
> 
> > On 2017/06/06 23:10:39, sky wrote:
> > >
> > https://codereview.chromium.org/2862653002/diff/360001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > > File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):
> > >
> > >
> > https://codereview.chromium.org/2862653002/diff/360001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode198
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:198:
> > views::GridLayout*
> > > dialog_layout = new views::GridLayout(this);
> > > On 2017/06/06 21:00:43, zmin wrote:
> > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > Why do you need two GridLayouts?
> > > >
> > > > dialog_layout is the global layout for the whole dialog.
> > > >
> > > > The additional layout(prompt_layout) is for the sub-view that contains
> > the
> > > > prompt_label view. The sub-view has different background color,
> > padding and
> > > > border.
> > > >
> > > > I just removed the sub-view with its layout and append the label to the
> > dialog
> > > > layout directly.
> > > >
> > > > Is multiple levels of layout bad for the performance?
> > >
> > > No, I just wanted to understand why it was necessary. Often times it
> > isn't.
> > >
> > >
> > https://codereview.chromium.org/2862653002/diff/360001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode244
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:244: return
> > > close_timer_->desired_run_time() - base::TimeTicks::Now();
> > > On 2017/06/06 21:00:43, zmin wrote:
> > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > To avoid possible weird bugs I recommend making this negative.
> > > >
> > > > Why negative result will avoid weird bug, is it because the overflow
> > when
> > > > desired_run_time is less than current time?
> > > >
> > > > How about I just make sure it always return non-negative number?
> > >
> > > Sorry, my comment was really bad here. I meant it seems less error prone
> > to
> > > ensure this doesn't go negative. But I think TimeDelta hides that, so
> > ignore
> > > this comment. Sorry.
> > >
> > >
> > https://codereview.chromium.org/2862653002/diff/360001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.h
> > > File chrome/browser/ui/views/profiles/force_signout_dialog.h (right):
> > >
> > >
> > https://codereview.chromium.org/2862653002/diff/360001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.h#newcode28
> > > chrome/browser/ui/views/profiles/force_signout_dialog.h:28: class
> > > ForceSignoutDialog : public views::DialogDelegateView {
> > > On 2017/06/06 21:00:44, zmin wrote:
> > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > WDYT of naming this ReauthenticateDialog?
> > > >
> > > > There're lots of different situations requires reauth, how about
> > > > ForceReauthenticateDialog
> > >
> > > ForcedReauthenticationDialog?
> > Sure, I can do that.
> >
> >
> > >
> > https://codereview.chromium.org/2862653002/diff/360001/
> > chrome/browser/ui/views/profiles/force_signout_dialog.h#newcode34
> > > chrome/browser/ui/views/profiles/force_signout_dialog.h:34: base::Timer*
> > > close_timer);
> > > On 2017/06/06 21:00:43, zmin wrote:
> > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > Document ownership. Also, why does the caller need to supply a timer?
> > > >
> > > > The |close_timer| is used to track the remaining time mainly.
> > > > It will also be used to
> > > > 1) Stop the timer if there is no opened browser window as we can sign
> > out
> > user
> > > > without delay.
> > > > 2) Close the dialog if the timer is stopped
> > >
> > > I get that you want a close_timer, but why does it need to be passed in?
> > Can't
> > > the dialog own and creates it's own timer?
> >
> > close_timer(5 minutes countdown) will continue after user close the
> > dialog.
> > So at least dialog can't own the timer.
> >
> 
> Why does that mean the dialog needs to share the same timer? If the caller
> needs to maintain a timer, can't it do that?
> 
>   -Scott

I share the timer so that the dialog and caller will share the exactly same
timestamp.

If this is no necessary, I think the dialog shouldn't care about the timer at
all. 
The caller will tell the dialog the duration of countdown. 
Dialog use this duration to display the time remaining and dismiss itself after
that.


> 
> >
> >
> > https://codereview.chromium.org/2862653002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

zmin

On 2017/06/07 19:17:18, zmin wrote: > On 2017/06/07 16:51:57, sky wrote: > > On Tue, ...

3 years, 6 months ago (2017-06-07 19:22:23 UTC) #126

On 2017/06/07 19:17:18, zmin wrote:
> On 2017/06/07 16:51:57, sky wrote:
> > On Tue, Jun 6, 2017 at 5:00 PM, <mailto:zmin@chromium.org> wrote:
> > 
> > > On 2017/06/06 23:10:39, sky wrote:
> > > >
> > > https://codereview.chromium.org/2862653002/diff/360001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > > > File chrome/browser/ui/views/profiles/force_signout_dialog.cc (right):
> > > >
> > > >
> > > https://codereview.chromium.org/2862653002/diff/360001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode198
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:198:
> > > views::GridLayout*
> > > > dialog_layout = new views::GridLayout(this);
> > > > On 2017/06/06 21:00:43, zmin wrote:
> > > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > > Why do you need two GridLayouts?
> > > > >
> > > > > dialog_layout is the global layout for the whole dialog.
> > > > >
> > > > > The additional layout(prompt_layout) is for the sub-view that contains
> > > the
> > > > > prompt_label view. The sub-view has different background color,
> > > padding and
> > > > > border.
> > > > >
> > > > > I just removed the sub-view with its layout and append the label to
the
> > > dialog
> > > > > layout directly.
> > > > >
> > > > > Is multiple levels of layout bad for the performance?
> > > >
> > > > No, I just wanted to understand why it was necessary. Often times it
> > > isn't.
> > > >
> > > >
> > > https://codereview.chromium.org/2862653002/diff/360001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode244
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:244: return
> > > > close_timer_->desired_run_time() - base::TimeTicks::Now();
> > > > On 2017/06/06 21:00:43, zmin wrote:
> > > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > > To avoid possible weird bugs I recommend making this negative.
> > > > >
> > > > > Why negative result will avoid weird bug, is it because the overflow
> > > when
> > > > > desired_run_time is less than current time?
> > > > >
> > > > > How about I just make sure it always return non-negative number?
> > > >
> > > > Sorry, my comment was really bad here. I meant it seems less error prone
> > > to
> > > > ensure this doesn't go negative. But I think TimeDelta hides that, so
> > > ignore
> > > > this comment. Sorry.
> > > >
> > > >
> > > https://codereview.chromium.org/2862653002/diff/360001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.h
> > > > File chrome/browser/ui/views/profiles/force_signout_dialog.h (right):
> > > >
> > > >
> > > https://codereview.chromium.org/2862653002/diff/360001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.h#newcode28
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.h:28: class
> > > > ForceSignoutDialog : public views::DialogDelegateView {
> > > > On 2017/06/06 21:00:44, zmin wrote:
> > > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > > WDYT of naming this ReauthenticateDialog?
> > > > >
> > > > > There're lots of different situations requires reauth, how about
> > > > > ForceReauthenticateDialog
> > > >
> > > > ForcedReauthenticationDialog?
> > > Sure, I can do that.
> > >
> > >
> > > >
> > > https://codereview.chromium.org/2862653002/diff/360001/
> > > chrome/browser/ui/views/profiles/force_signout_dialog.h#newcode34
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.h:34: base::Timer*
> > > > close_timer);
> > > > On 2017/06/06 21:00:43, zmin wrote:
> > > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > > Document ownership. Also, why does the caller need to supply a
timer?
> > > > >
> > > > > The |close_timer| is used to track the remaining time mainly.
> > > > > It will also be used to
> > > > > 1) Stop the timer if there is no opened browser window as we can sign
> > > out
> > > user
> > > > > without delay.
> > > > > 2) Close the dialog if the timer is stopped
> > > >
> > > > I get that you want a close_timer, but why does it need to be passed in?
> > > Can't
> > > > the dialog own and creates it's own timer?
> > >
> > > close_timer(5 minutes countdown) will continue after user close the
> > > dialog.
> > > So at least dialog can't own the timer.
> > >
> > 
> > Why does that mean the dialog needs to share the same timer? If the caller
> > needs to maintain a timer, can't it do that?
> > 
> >   -Scott
> 
> I share the timer so that the dialog and caller will share the exactly same
> timestamp.
> 
> If this is no necessary, I think the dialog shouldn't care about the timer at
> all. 
> The caller will tell the dialog the duration of countdown. 
> Dialog use this duration to display the time remaining and dismiss itself
after
> that.

The duration will be controlled by caller for different situation.
For example, if Chrome was just launched a second ago, there is no need to give
full five minutes
countdown. I have chatted this with the UX.


> 
> > 
> > >
> > >
> > > https://codereview.chromium.org/2862653002/
> > >
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-07 19:24:50 UTC) #127

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/444938)

3 years, 6 months ago (2017-06-07 19:24:51 UTC) #128

sky

Can the consumer supply the duration to the dialog? On Wed, Jun 7, 2017 at ...

3 years, 6 months ago (2017-06-07 20:22:24 UTC) #129

Can the consumer supply the duration to the dialog?

On Wed, Jun 7, 2017 at 12:22 PM, <zmin@chromium.org> wrote:

> On 2017/06/07 19:17:18, zmin wrote:
> > On 2017/06/07 16:51:57, sky wrote:
> > > On Tue, Jun 6, 2017 at 5:00 PM, <mailto:zmin@chromium.org> wrote:
> > >
> > > > On 2017/06/06 23:10:39, sky wrote:
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/360001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc
> > > > > File chrome/browser/ui/views/profiles/force_signout_dialog.cc
> (right):
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/360001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode198
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:198:
> > > > views::GridLayout*
> > > > > dialog_layout = new views::GridLayout(this);
> > > > > On 2017/06/06 21:00:43, zmin wrote:
> > > > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > > > Why do you need two GridLayouts?
> > > > > >
> > > > > > dialog_layout is the global layout for the whole dialog.
> > > > > >
> > > > > > The additional layout(prompt_layout) is for the sub-view that
> contains
> > > > the
> > > > > > prompt_label view. The sub-view has different background color,
> > > > padding and
> > > > > > border.
> > > > > >
> > > > > > I just removed the sub-view with its layout and append the label
> to
> the
> > > > dialog
> > > > > > layout directly.
> > > > > >
> > > > > > Is multiple levels of layout bad for the performance?
> > > > >
> > > > > No, I just wanted to understand why it was necessary. Often times
> it
> > > > isn't.
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/360001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc#newcode244
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.cc:244:
> return
> > > > > close_timer_->desired_run_time() - base::TimeTicks::Now();
> > > > > On 2017/06/06 21:00:43, zmin wrote:
> > > > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > > > To avoid possible weird bugs I recommend making this negative.
> > > > > >
> > > > > > Why negative result will avoid weird bug, is it because the
> overflow
> > > > when
> > > > > > desired_run_time is less than current time?
> > > > > >
> > > > > > How about I just make sure it always return non-negative number?
> > > > >
> > > > > Sorry, my comment was really bad here. I meant it seems less error
> prone
> > > > to
> > > > > ensure this doesn't go negative. But I think TimeDelta hides that,
> so
> > > > ignore
> > > > > this comment. Sorry.
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/360001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.h
> > > > > File chrome/browser/ui/views/profiles/force_signout_dialog.h
> (right):
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/360001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.h#newcode28
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.h:28: class
> > > > > ForceSignoutDialog : public views::DialogDelegateView {
> > > > > On 2017/06/06 21:00:44, zmin wrote:
> > > > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > > > WDYT of naming this ReauthenticateDialog?
> > > > > >
> > > > > > There're lots of different situations requires reauth, how about
> > > > > > ForceReauthenticateDialog
> > > > >
> > > > > ForcedReauthenticationDialog?
> > > > Sure, I can do that.
> > > >
> > > >
> > > > >
> > > > https://codereview.chromium.org/2862653002/diff/360001/
> > > > chrome/browser/ui/views/profiles/force_signout_dialog.h#newcode34
> > > > > chrome/browser/ui/views/profiles/force_signout_dialog.h:34:
> base::Timer*
> > > > > close_timer);
> > > > > On 2017/06/06 21:00:43, zmin wrote:
> > > > > > On 2017/06/05 23:43:18, sky wrote:
> > > > > > > Document ownership. Also, why does the caller need to supply a
> timer?
> > > > > >
> > > > > > The |close_timer| is used to track the remaining time mainly.
> > > > > > It will also be used to
> > > > > > 1) Stop the timer if there is no opened browser window as we can
> sign
> > > > out
> > > > user
> > > > > > without delay.
> > > > > > 2) Close the dialog if the timer is stopped
> > > > >
> > > > > I get that you want a close_timer, but why does it need to be
> passed in?
> > > > Can't
> > > > > the dialog own and creates it's own timer?
> > > >
> > > > close_timer(5 minutes countdown) will continue after user close the
> > > > dialog.
> > > > So at least dialog can't own the timer.
> > > >
> > >
> > > Why does that mean the dialog needs to share the same timer? If the
> caller
> > > needs to maintain a timer, can't it do that?
> > >
> > > -Scott
> >
> > I share the timer so that the dialog and caller will share the exactly
> same
> > timestamp.
> >
> > If this is no necessary, I think the dialog shouldn't care about the
> timer at
> > all.
> > The caller will tell the dialog the duration of countdown.
> > Dialog use this duration to display the time remaining and dismiss itself
> after
> > that.
>
> The duration will be controlled by caller for different situation.
> For example, if Chrome was just launched a second ago, there is no need to
> give
> full five minutes
> countdown. I have chatted this with the UX.
>
>
> >
> > >
> > > >
> > > >
> > > > https://codereview.chromium.org/2862653002/
> > > >
> > >
> > > --
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2862653002/
>

-- 
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.

sky

https://codereview.chromium.org/2862653002/diff/420001/chrome/browser/ui/views/profiles/forced_reauthentication_dialog.cc File chrome/browser/ui/views/profiles/forced_reauthentication_dialog.cc (right): https://codereview.chromium.org/2862653002/diff/420001/chrome/browser/ui/views/profiles/forced_reauthentication_dialog.cc#newcode120 chrome/browser/ui/views/profiles/forced_reauthentication_dialog.cc:120: if (GetTimeRemaining() < base::TimeDelta::FromSeconds(kCloseDirectlyTimer)) { Is this conditional still ...

3 years, 6 months ago (2017-06-07 20:37:59 UTC) #130

zmin

https://codereview.chromium.org/2862653002/diff/420001/chrome/browser/ui/views/profiles/forced_reauthentication_dialog.cc File chrome/browser/ui/views/profiles/forced_reauthentication_dialog.cc (right): https://codereview.chromium.org/2862653002/diff/420001/chrome/browser/ui/views/profiles/forced_reauthentication_dialog.cc#newcode120 chrome/browser/ui/views/profiles/forced_reauthentication_dialog.cc:120: if (GetTimeRemaining() < base::TimeDelta::FromSeconds(kCloseDirectlyTimer)) { On 2017/06/07 20:37:59, sky ...

3 years, 6 months ago (2017-06-07 20:56:49 UTC) #131

sky

LGTM https://codereview.chromium.org/2862653002/diff/440001/chrome/browser/ui/views/profiles/forced_reauthentication_dialog.h File chrome/browser/ui/views/profiles/forced_reauthentication_dialog.h (right): https://codereview.chromium.org/2862653002/diff/440001/chrome/browser/ui/views/profiles/forced_reauthentication_dialog.h#newcode60 chrome/browser/ui/views/profiles/forced_reauthentication_dialog.h:60: base::TimeTicks desired_close_time_; const

3 years, 6 months ago (2017-06-07 23:06:58 UTC) #132

zmin

zmin@chromium.org changed reviewers: + jwd@chromium.org, rogerta@chromium.org

3 years, 6 months ago (2017-06-08 14:56:26 UTC) #133

zmin

+rogerta@ for components/signin/core/browser/signin_metrics.h +jwd@ for tools/metrics/actions/actions.xml

3 years, 6 months ago (2017-06-08 14:56:27 UTC) #134

Roger Tawa OOO till Jul 10th

lgtm components/signin/core/browser/signin_metrics.h

3 years, 6 months ago (2017-06-08 17:49:58 UTC) #135

jwd

LGTM with nit https://codereview.chromium.org/2862653002/diff/440001/tools/metrics/actions/actions.xml File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2862653002/diff/440001/tools/metrics/actions/actions.xml#newcode15400 tools/metrics/actions/actions.xml:15400: Recorded when user sign in again ...

3 years, 6 months ago (2017-06-08 19:39:23 UTC) #136

zmin

The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, sky@chromium.org, jwd@chromium.org Link ...

3 years, 6 months ago (2017-06-08 20:52:32 UTC) #138

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862653002/460001

3 years, 6 months ago (2017-06-08 20:53:14 UTC) #139

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 460001, "attempt_start_ts": 1496955151486040, "parent_rev": "9b40ada69660b42c0261c2830d6f2215d0a6f1f4", "commit_rev": "4dc68137b9724480cf6e055d24cbff75458a114e"}

3 years, 6 months ago (2017-06-08 23:51:43 UTC) #140

commit-bot: I haz the power

Description was changed from ========== If force-sign-in policy is enabled, popup warning dialog before window ...

3 years, 6 months ago (2017-06-08 23:51:57 UTC) #141

commit-bot: I haz the power

3 years, 6 months ago (2017-06-08 23:51:58 UTC) #142

Message was sent while issue was closed.

Committed patchset #16 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/4dc68137b9724480cf6e055d24cb...

Issue 2862653002: If force-sign-in policy is enabled, popup warning dialog before window closing if auth token becom… (Closed)

Description

Patch Set 1 #

Patch Set 2 : fixup #

Patch Set 3 : sky's comments, without browsertest update #

Patch Set 4 : update browser test #

Patch Set 5 : fix #

Patch Set 6 : remove app_modal:: #

Patch Set 7 : fixup and rebase from master #

Patch Set 8 : refactor #

Patch Set 9 : cr #

Patch Set 10 : cr and rebase from master #

Patch Set 11 : UX update #

Patch Set 12 : cr #

Patch Set 13 : rename #

Patch Set 14 : remove timer #

Patch Set 15 : cr #

Patch Set 16 : nit #

Messages