Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
192573003:
After a supervised user has been imported, close overlays. (Closed)
|
Created:
6 years, 9 months ago by Adrian Kuegel Modified:
6 years, 9 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, dbeam+watch-options_chromium.org, tfarina, pam+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAfter a supervised user has been imported, close overlays.
When successfully importing a supervised user, close both
the supervised user import overlay and the create new
profile overlay.
BUG=343723
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255967
Patch Set 1 #
Total comments: 4
Patch Set 2 : Close all overlays. #
Total comments: 2
Patch Set 3 : Simplify. #
Messages
Total messages: 15 (0 generated)
Bernhard, can you please review this CL? This is another thing I forgot to change when moving the import overlay to a higher level.
https://codereview.chromium.org/192573003/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/managed_user_import.js (right): https://codereview.chromium.org/192573003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/managed_user_import.js:231: OptionsPage.closeOverlay(); It seems somewhat brittle that we close the two frontmost overlays without knowing what they are (and implicitly assuming that the second one is the create profile overlay). If what we want to achieve here is simply that we go back to the topmost page, we could add a method OptionsPage.closeAllOverlays(). WDYT?
https://codereview.chromium.org/192573003/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/managed_user_import.js (right): https://codereview.chromium.org/192573003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/managed_user_import.js:231: OptionsPage.closeOverlay(); On 2014/03/10 10:25:19, Bernhard Bauer wrote: > It seems somewhat brittle that we close the two frontmost overlays without > knowing what they are (and implicitly assuming that the second one is the create > profile overlay). > > If what we want to achieve here is simply that we go back to the topmost page, > we could add a method OptionsPage.closeAllOverlays(). WDYT? Yes, I think that is the functionality we actually want here, and I was looking for such a method. I guess I will have to write it :)
https://codereview.chromium.org/192573003/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/managed_user_import.js (right): https://codereview.chromium.org/192573003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/managed_user_import.js:231: OptionsPage.closeOverlay(); On 2014/03/10 10:28:52, Adrian Kuegel wrote: > On 2014/03/10 10:25:19, Bernhard Bauer wrote: > > It seems somewhat brittle that we close the two frontmost overlays without > > knowing what they are (and implicitly assuming that the second one is the > create > > profile overlay). > > > > If what we want to achieve here is simply that we go back to the topmost page, > > we could add a method OptionsPage.closeAllOverlays(). WDYT? > > Yes, I think that is the functionality we actually want here, and I was looking > for such a method. I guess I will have to write it :) Yesplz :) It shouldn't be too difficult; just closeOverlay() while there is an overlay page visible.
https://codereview.chromium.org/192573003/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/managed_user_import.js (right): https://codereview.chromium.org/192573003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/managed_user_import.js:231: OptionsPage.closeOverlay(); On 2014/03/10 10:37:36, Bernhard Bauer wrote: > On 2014/03/10 10:28:52, Adrian Kuegel wrote: > > On 2014/03/10 10:25:19, Bernhard Bauer wrote: > > > It seems somewhat brittle that we close the two frontmost overlays without > > > knowing what they are (and implicitly assuming that the second one is the > > create > > > profile overlay). > > > > > > If what we want to achieve here is simply that we go back to the topmost > page, > > > we could add a method OptionsPage.closeAllOverlays(). WDYT? > > > > Yes, I think that is the functionality we actually want here, and I was > looking > > for such a method. I guess I will have to write it :) > > Yesplz :) > > It shouldn't be too difficult; just closeOverlay() while there is an overlay > page visible. Done.
https://codereview.chromium.org/192573003/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/options_page.js (right): https://codereview.chromium.org/192573003/diff/20001/chrome/browser/resources... chrome/browser/resources/options/options_page.js:354: while (overlay) { This could be simplified to: while (this.isOverlayVisible_()) { this.closeOverlay(); }
https://codereview.chromium.org/192573003/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/options_page.js (right): https://codereview.chromium.org/192573003/diff/20001/chrome/browser/resources... chrome/browser/resources/options/options_page.js:354: while (overlay) { On 2014/03/10 10:56:17, Bernhard Bauer wrote: > This could be simplified to: > > while (this.isOverlayVisible_()) { > this.closeOverlay(); > } Done.
LGTM!
The CQ bit was checked by akuegel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/192573003/30001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/192573003/30001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/192573003/30001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/192573003/30001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/192573003/30001
Message was sent while issue was closed.
Change committed as 255967 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
