Fixing the hiding error bubble animation bug on second opening of create user dialog.

Fixing the hiding error bubble animation bug on second opening of create user dialog.

This bug was introduced with the fix of adding SUs with the same name. The problem is that the callback that is called when SU are refreshed does not know whether it is called after a new user is created. This causes the dialog to think that it needs to create the error bubble. For some reason the overlay is not closed after the creation of account is done, so the solution is to expose the creationInProgress, so the callback does not proceed. This requires the onSuccess callback to not change the state (otherwise the receiveSupervisedUsers callback continues to be called and cause the error).
BUG=575134
Committed: https://crrev.com/5ca1e81c7db5a0d5fb0028ccbb0277a85112b0ac
Cr-Commit-Position: refs/heads/master@{#369314}

[atanasova, 2016-01-07 14:11:44]

Description was changed from

==========
Fixing the hiding error bubble animation bug on second opening of create user
dialog.

This bug was introduced with the fix of adding SUs with the same name. The
problem is that the callback that is called when SU are refreshed does not know
whether it is called after a new user is created. This causes the dialog to
think that it needs to create the error bubble. For some reason the overlay is
not closed after the creation of account is done, so the solution is to expose
the creationInProgress, so the callback does not proceed. This requires the
onSuccess callback to not change the state (otherwise the receiveSupervisedUsers
callback continues to be called and cause the error).
BUG=575134
==========

to

==========
Fixing the hiding error bubble animation bug on second opening of create user
dialog.

This bug was introduced with the fix of adding SUs with the same name. The
problem is that the callback that is called when SU are refreshed does not know
whether it is called after a new user is created. This causes the dialog to
think that it needs to create the error bubble. For some reason the overlay is
not closed after the creation of account is done, so the solution is to expose
the creationInProgress, so the callback does not proceed. This requires the
onSuccess callback to not change the state (otherwise the receiveSupervisedUsers
callback continues to be called and cause the error).
BUG=575134
==========

[atanasova, 2016-01-07 14:11:44]

atanasova@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-01-07 14:42:48]

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:762:
this.updateCreateInProgress_(false);
Why remove this?

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:374: // Because the
callback is called after the supervised user is created
Hm, this kinda makes it sound like that's the only situation in which it is
called. Maybe something like:
During the creation of a SU, this gets called with a list including the
just-created SU. Ignore, to prevent the "already exists" bubble from showing up.

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:414: if
($('supervised-user-import-existing')) {
Why is this necessary?

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:704: * We do not want
to show the errorBubble if a profile is being created.
Please describe what the method does, not what it's used for (that is bound to
become outdated!)
Returns whether a profile creation is currently in progress. ?

[atanasova, 2016-01-07 15:03:10]

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:762:
this.updateCreateInProgress_(false);
On 2016/01/07 14:42:48, Marc Treib wrote:
> Why remove this?

In the description I tried to explain it, but I noticed a weird behavior. In
theory based on this method the overlay should have been closed before we show
the confirm overlay. However it is not closed and I can see it in the html and
the callback is still called. What happens is that we set the update in progress
to false, but the callback keeps getting called which causes the error to
appear. This variable is only used in two places - in the callback and when
checking whether the checkbox for creating SU is disabled. Since when opening
the dialog again this value will be reset, there should not be a problem with
not setting it at this point.

A thing I could not figure out is why the overlay is not actually closed.

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:374: // Because the
callback is called after the supervised user is created
On 2016/01/07 14:42:48, Marc Treib wrote:
> Hm, this kinda makes it sound like that's the only situation in which it is
> called. Maybe something like:
> During the creation of a SU, this gets called with a list including the
> just-created SU. Ignore, to prevent the "already exists" bubble from showing
up.

Done.

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:414: if
($('supervised-user-import-existing')) {
On 2016/01/07 14:42:48, Marc Treib wrote:
> Why is this necessary?

If we display the error without the import option, this element does not exist
and it causes a null error.

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:704: * We do not want
to show the errorBubble if a profile is being created.
On 2016/01/07 14:42:48, Marc Treib wrote:
> Please describe what the method does, not what it's used for (that is bound to
> become outdated!)
> Returns whether a profile creation is currently in progress. ?

Done.

[Marc Treib, 2016-01-07 16:11:23]

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:762:
this.updateCreateInProgress_(false);
On 2016/01/07 15:03:10, atanasova wrote:
> On 2016/01/07 14:42:48, Marc Treib wrote:
> > Why remove this?
> 
> In the description I tried to explain it, but I noticed a weird behavior. In
> theory based on this method the overlay should have been closed before we show
> the confirm overlay. However it is not closed and I can see it in the html and
> the callback is still called. What happens is that we set the update in
progress
> to false, but the callback keeps getting called which causes the error to
> appear. This variable is only used in two places - in the callback and when
> checking whether the checkbox for creating SU is disabled. Since when opening
> the dialog again this value will be reset, there should not be a problem with
> not setting it at this point.
> 
> A thing I could not figure out is why the overlay is not actually closed.

Hm. I guess the callback being called does not necessarily mean that the overlay
is still open? Is it actually still visible?

updateCreateInProgress_ does more than just set the variable. Are you sure
nothing of that is relevant here?
Also, I dislike that onError (above) still calls it but onSuccess now doesn't.
Can the call in onError be removed as well?

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:414: if
($('supervised-user-import-existing')) {
On 2016/01/07 15:03:10, atanasova wrote:
> On 2016/01/07 14:42:48, Marc Treib wrote:
> > Why is this necessary?
> 
> If we display the error without the import option, this element does not exist
> and it causes a null error.

Ah, so it's a pre-existing problem? Okay then, carry on! :)

[atanasova, 2016-01-07 16:22:44]

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/1564063002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:762:
this.updateCreateInProgress_(false);
On 2016/01/07 16:11:23, Marc Treib wrote:
> On 2016/01/07 15:03:10, atanasova wrote:
> > On 2016/01/07 14:42:48, Marc Treib wrote:
> > > Why remove this?
> > 
> > In the description I tried to explain it, but I noticed a weird behavior. In
> > theory based on this method the overlay should have been closed before we
show
> > the confirm overlay. However it is not closed and I can see it in the html
and
> > the callback is still called. What happens is that we set the update in
> progress
> > to false, but the callback keeps getting called which causes the error to
> > appear. This variable is only used in two places - in the callback and when
> > checking whether the checkbox for creating SU is disabled. Since when
opening
> > the dialog again this value will be reset, there should not be a problem
with
> > not setting it at this point.
> > 
> > A thing I could not figure out is why the overlay is not actually closed.
> 
> Hm. I guess the callback being called does not necessarily mean that the
overlay
> is still open? Is it actually still visible?
> 
> updateCreateInProgress_ does more than just set the variable. Are you sure
> nothing of that is relevant here?
> Also, I dislike that onError (above) still calls it but onSuccess now doesn't.
> Can the call in onError be removed as well?

It is needed in the onError method, since then we will not close the overlay and
need to be able to do changes to the name/checkbox/buttons.

The rest of the things that are being set in the method are all part of the
overlay (correct me if I am wrong on this point). So during the onSuccess part
we either open a createConfirmOverlay which does not care about those things or
simply open a new window with that user (non SU case). I _think_ both those
cases will not care about the update (since it simply makes all the things
enabled again).

[atanasova, 2016-01-07 16:45:09]

Changes after offline discussion

[Marc Treib, 2016-01-07 17:04:16]

LGTM

https://codereview.chromium.org/1564063002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1564063002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:376: // from showing
up if the overlay is already hidden.
nit: Now the comment doesn't really correspond to what we're doing anymore.
"After a supervised user has been created and the dialog has been hidden, this
gets called again ...", something like that?

[atanasova, 2016-01-07 17:08:40]

atanasova@chromium.org changed reviewers:
+ pamg@google.com

[atanasova, 2016-01-07 17:08:40]

https://codereview.chromium.org/1564063002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1564063002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:376: // from showing
up if the overlay is already hidden.
On 2016/01/07 17:04:15, Marc Treib wrote:
> nit: Now the comment doesn't really correspond to what we're doing anymore.
> "After a supervised user has been created and the dialog has been hidden, this
> gets called again ...", something like that?

Done.

[Pam (message me for reviews), 2016-01-11 18:27:02]

pam@chromium.org changed reviewers:
+ pam@chromium.org
- pamg@google.com

[Pam (message me for reviews), 2016-01-11 18:32:37]

https://codereview.chromium.org/1564063002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1564063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:378: if
(PageManager.getTopmostVisiblePage().pageDiv != this.pageDiv)
Do we actually need to skip this if the dialog is open but not on top? If not,
then checking visibility directly would be more robust. If I remember correctly,

this.visible

should do it: that also returns false if the dialog is in the process of fading
out (see page.js).

[atanasova, 2016-01-13 16:55:07]

https://codereview.chromium.org/1564063002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1564063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:378: if
(PageManager.getTopmostVisiblePage().pageDiv != this.pageDiv)
On 2016/01/11 18:32:37, Pam (also PM for reviews) wrote:
> Do we actually need to skip this if the dialog is open but not on top? If not,
> then checking visibility directly would be more robust. If I remember
correctly,
> 
> this.visible
> 
> should do it: that also returns false if the dialog is in the process of
fading
> out (see page.js).

Unfortunately, if we do not check on reopening the dialog the hiding animation
is visible, since we go through this function once the user is created. This
causes the error bubble to be shown, so on reopening there is a hiding
animation. I am replacing the complicated line with your solution.

[Pam (message me for reviews), 2016-01-13 22:05:10]

LGTM!

https://codereview.chromium.org/1564063002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1564063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:378: if
(PageManager.getTopmostVisiblePage().pageDiv != this.pageDiv)
On 2016/01/13 16:55:07, atanasova wrote:
> On 2016/01/11 18:32:37, Pam (also PM for reviews) wrote:
> > Do we actually need to skip this if the dialog is open but not on top? If
not,
> > then checking visibility directly would be more robust. If I remember
> correctly,
> > 
> > this.visible
> > 
> > should do it: that also returns false if the dialog is in the process of
> fading
> > out (see page.js).
> 
> Unfortunately, if we do not check on reopening the dialog the hiding animation
> is visible, since we go through this function once the user is created. This
> causes the error bubble to be shown, so on reopening there is a hiding
> animation. I am replacing the complicated line with your solution.

Sorry, I must have been unclear. I see that we need to check on reopening the
dialog. The difference between what you had and what I suggested is that the new
code will run the function (not return early) if  the dialog is open (visible),
but there's another overlay open on top of it (it's not the topmost). I can't
think of a case where that matters, but wanted to be sure you also couldn't.

[atanasova, 2016-01-13 22:08:52]

https://codereview.chromium.org/1564063002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1564063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:378: if
(PageManager.getTopmostVisiblePage().pageDiv != this.pageDiv)
On 2016/01/13 22:05:09, Pam (also PM for reviews) wrote:
> On 2016/01/13 16:55:07, atanasova wrote:
> > On 2016/01/11 18:32:37, Pam (also PM for reviews) wrote:
> > > Do we actually need to skip this if the dialog is open but not on top? If
> not,
> > > then checking visibility directly would be more robust. If I remember
> > correctly,
> > > 
> > > this.visible
> > > 
> > > should do it: that also returns false if the dialog is in the process of
> > fading
> > > out (see page.js).
> > 
> > Unfortunately, if we do not check on reopening the dialog the hiding
animation
> > is visible, since we go through this function once the user is created. This
> > causes the error bubble to be shown, so on reopening there is a hiding
> > animation. I am replacing the complicated line with your solution.
> 
> Sorry, I must have been unclear. I see that we need to check on reopening the
> dialog. The difference between what you had and what I suggested is that the
new
> code will run the function (not return early) if  the dialog is open
(visible),
> but there's another overlay open on top of it (it's not the topmost). I can't
> think of a case where that matters, but wanted to be sure you also couldn't.

I tested it out and the problem was not there using you solution. I also tried
to think of a case when that could be a problem, but I think it should work this
way.

[atanasova, 2016-01-13 22:08:55]

The CQ bit was checked by atanasova@chromium.org

[atanasova, 2016-01-13 22:08:56]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1564063002/#ps100001
(title: "Merge with master")

[commit-bot: I haz the power, 2016-01-13 22:11:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1564063002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1564063002/100001

[commit-bot: I haz the power, 2016-01-14 00:18:46]

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

[commit-bot: I haz the power, 2016-01-14 00:18:47]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[atanasova, 2016-01-14 00:51:48]

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

[commit-bot: I haz the power, 2016-01-14 00:56:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1564063002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1564063002/100001

[commit-bot: I haz the power, 2016-01-14 02:17:42]

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

[commit-bot: I haz the power, 2016-01-14 02:17:42]

Dry run: This issue passed the CQ dry run.

[atanasova, 2016-01-14 02:33:55]

The CQ bit was checked by atanasova@chromium.org

[commit-bot: I haz the power, 2016-01-14 02:40:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1564063002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1564063002/100001

[commit-bot: I haz the power, 2016-01-14 03:46:28]

Description was changed from

==========
Fixing the hiding error bubble animation bug on second opening of create user
dialog.

This bug was introduced with the fix of adding SUs with the same name. The
problem is that the callback that is called when SU are refreshed does not know
whether it is called after a new user is created. This causes the dialog to
think that it needs to create the error bubble. For some reason the overlay is
not closed after the creation of account is done, so the solution is to expose
the creationInProgress, so the callback does not proceed. This requires the
onSuccess callback to not change the state (otherwise the receiveSupervisedUsers
callback continues to be called and cause the error).
BUG=575134
==========

to

==========
Fixing the hiding error bubble animation bug on second opening of create user
dialog.

This bug was introduced with the fix of adding SUs with the same name. The
problem is that the callback that is called when SU are refreshed does not know
whether it is called after a new user is created. This causes the dialog to
think that it needs to create the error bubble. For some reason the overlay is
not closed after the creation of account is done, so the solution is to expose
the creationInProgress, so the callback does not proceed. This requires the
onSuccess callback to not change the state (otherwise the receiveSupervisedUsers
callback continues to be called and cause the error).
BUG=575134
==========

[commit-bot: I haz the power, 2016-01-14 03:46:29]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-14 03:48:41]

Description was changed from

==========
Fixing the hiding error bubble animation bug on second opening of create user
dialog.

This bug was introduced with the fix of adding SUs with the same name. The
problem is that the callback that is called when SU are refreshed does not know
whether it is called after a new user is created. This causes the dialog to
think that it needs to create the error bubble. For some reason the overlay is
not closed after the creation of account is done, so the solution is to expose
the creationInProgress, so the callback does not proceed. This requires the
onSuccess callback to not change the state (otherwise the receiveSupervisedUsers
callback continues to be called and cause the error).
BUG=575134
==========

to

==========
Fixing the hiding error bubble animation bug on second opening of create user
dialog.

This bug was introduced with the fix of adding SUs with the same name. The
problem is that the callback that is called when SU are refreshed does not know
whether it is called after a new user is created. This causes the dialog to
think that it needs to create the error bubble. For some reason the overlay is
not closed after the creation of account is done, so the solution is to expose
the creationInProgress, so the callback does not proceed. This requires the
onSuccess callback to not change the state (otherwise the receiveSupervisedUsers
callback continues to be called and cause the error).
BUG=575134

Committed: https://crrev.com/5ca1e81c7db5a0d5fb0028ccbb0277a85112b0ac
Cr-Commit-Position: refs/heads/master@{#369314}
==========

[commit-bot: I haz the power, 2016-01-14 03:48:42]

Patchset 6 (id:??) landed as
https://crrev.com/5ca1e81c7db5a0d5fb0028ccbb0277a85112b0ac
Cr-Commit-Position: refs/heads/master@{#369314}