Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769383002:
Firefox overlay is seen opened even if Import Overlay is Cancelled.
|
Created:
3 years, 9 months ago by nikhil.sahni Modified:
3 years, 8 months ago Reviewers:
Dan Beam, Shanmuga Pandi, Elliot Glaysher, ranjitkan, msw, Elly Fong-Jones, nyerramilli, Srirama, Ilya Sherman CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFirefox overlay is seen opened even if Import Overlay is Cancelled.
Closing the Firefox Overlay If Import Bookmarks Overlay is closed
by checking if the firefox Overlay is existing then close it.
BUG=672428
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #Patch Set 2 : Firefox overlay is seen opened even if Import Overlay is Cancelled. #Patch Set 3 : Firefox overlay is seen opened even if Import Overlay is Cancelled. #
Total comments: 2
Patch Set 4 : 'Close Firefox before importing' overlay is seen opened even if 'Import bookmarks and settings' ove… #Patch Set 5 : Fixed the Review Comments #
Total comments: 33
Patch Set 6 : Fixing Review Comments #
Total comments: 12
Patch Set 7 : Incorportaed Review Comments. #
Total comments: 10
Patch Set 8 : Addressing Review Comments #Patch Set 9 : Fixing Review Comments #Patch Set 10 : Added Checks for all the Exit possibilites for your consideration #
Total comments: 3
Patch Set 11 : Implemented Widget Observer and Addressed Review Comments. #
Total comments: 16
Patch Set 12 : Fixed Review Comments #
Total comments: 9
Patch Set 13 : Review Comments Addressed #Patch Set 14 : Addressing Review Comments #
Total comments: 1
Messages
Total messages: 37 (11 generated)
Description was changed from ========== 'Close Firefox before importing' overlay is seen opened even if 'Import bookmarks and settings' overlay is closed. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 ========== to ========== 'Close Firefox before importing' overlay is seen opened even if 'Import bookmarks and settings' overlay is closed. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== 'Close Firefox before importing' overlay is seen opened even if 'Import bookmarks and settings' overlay is closed. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== 'Close Firefox before importing' overlay is seen opened even if 'Import bookmarks and settings' overlay is closed. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation R=SamsungPeerReview ==========
nikhil.sahni@samsung.com changed reviewers: + shanmuga.m@samsung.com, srirama.m@samsung.com
peer review looks goog. Address the nit and adjust the commit message to be shorter (preferably not more than 80char) https://codereview.chromium.org/2769383002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2769383002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:38: on-close="closeDialog_" ignore-popstate> nit: give indentation
Description was changed from ========== 'Close Firefox before importing' overlay is seen opened even if 'Import bookmarks and settings' overlay is closed. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation R=SamsungPeerReview ========== to ========== Firefox overlay is seen opened even if Import Overlay is Cancelled. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation R=SamsungPeerReview ==========
Description was changed from ========== 'Close Firefox before importing' overlay is seen opened even if 'Import bookmarks and settings' overlay is closed. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation R=SamsungPeerReview ========== to ========== Firefox overlay is seen opened even if Import Overlay is Cancelled. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation R=SamsungPeerReview ==========
Description was changed from ========== 'Close Firefox before importing' overlay is seen opened even if 'Import bookmarks and settings' overlay is closed. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation R=SamsungPeerReview ========== to ========== Firefox overlay is seen opened even if Import Overlay is Cancelled. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation R=SamsungPeerReview ==========
https://codereview.chromium.org/2769383002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2769383002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:38: on-close="closeDialog_" ignore-popstate> On 2017/03/24 09:33:03, Srirama wrote: > nit: give indentation Done.
Description was changed from ========== Firefox overlay is seen opened even if Import Overlay is Cancelled. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation R=SamsungPeerReview ========== to ========== Firefox overlay is seen opened even if Import Overlay is Cancelled. Closing the Firefox Overlay If Import Bookmarks Overlay is closed by checking if the firefox Overlay is existing then close it. BUG=672428 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
nikhil.sahni@samsung.com changed reviewers: + nyerramilli@chromium.org, ranjitkan@chromium.org
isherman@chromium.org changed reviewers: + isherman@chromium.org
Hi Nikhil, thank you for the CL! It mostly looks pretty good! I have a bunch of nit-picky style guide comments for you; but ignoring those, there are only a couple of really important notes on functionality. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/importer... File chrome/browser/importer/importer_lock_dialog.h (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/importer... chrome/browser/importer/importer_lock_dialog.h:18: void HideImportLockDialog(); Please add documentation for this function. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/importer... chrome/browser/importer/importer_lock_dialog.h:18: void HideImportLockDialog(); nit: Please leave a blank line before and after this function declaration. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_browser_proxy.js (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:58: importFromBookmarksFile: function() {}, nit: Please leave a blank line after this one. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:60: * Close the Dialog if it is Opened. nit: s/Dialog/dialog; s/Opened/opened. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:61: */ nit: Please fix up the indentation of the asterisks in this comment. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:62: closeDialog: function() {}, nit: This should probably be named "closeImportDialog", so that it's clearer which dialog this refers to, when the event is handled on the JavaScript side. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:63: nit: Please remove this blank line. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:93: chrome.send('closeDialog'); nit: Likewise, this should be named 'closeImportDialog'. (Actually, does this close the *import lock* dialog? If so, it should probably be named 'closeImportLockDialog'.) https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:132: this.browserProxy_.closeDialog(); This line is confusing: One dialog is already being closed, and here you're sending a message to close a different dialog... but that's not very obvious from context. Please add a comment here to explain. (Having a more specific method name than "closeDialog()" will also help.) https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:33: void HideImportLockDialog() { You'll need to also implement this method for Mac, within chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:39: views::Widget* ImportLockDialogView::m_widget = NULL; nit: s/NULL/nullptr https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:43: if (m_widget) { It looks like the widget can be destroyed without m_widget ever being updated. I think you'll need to register a WidgetObserver that can listen for OnWidgetDestroying() and reset this pointer to null. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:45: m_widget->CloseNow(); Hmm, is it actually necessary to call both of these methods? I'm not very familiar with Views, but it seems a bit surprising that you'd need both. It looks like the Close() method should do what you need. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/importer/import_lock_dialog_view.h (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.h:43: static views::Widget* m_widget; nit: Please move this to the implementation file, name it g_widget, and document it. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_import_data_handler.cc (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_import_data_handler.cc:74: "closeDialog", Again, you'll want a more specific name, that explicitly mentions importing.
Hi Ilya Sherman, Thanks for your review Comments, but am stuck with the Cocoa Implementation. Please have a look at my latest submission. Thanks again for your guidance. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/importer... File chrome/browser/importer/importer_lock_dialog.h (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/importer... chrome/browser/importer/importer_lock_dialog.h:18: void HideImportLockDialog(); On 2017/03/27 15:21:52, Ilya Sherman wrote: > Please add documentation for this function. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/importer... chrome/browser/importer/importer_lock_dialog.h:18: void HideImportLockDialog(); On 2017/03/27 15:21:52, Ilya Sherman wrote: > Please add documentation for this function. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_browser_proxy.js (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:58: importFromBookmarksFile: function() {}, On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:60: * Close the Dialog if it is Opened. On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: s/Dialog/dialog; s/Opened/opened. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:61: */ On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Please fix up the indentation of the asterisks in this comment. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:62: closeDialog: function() {}, On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: This should probably be named "closeImportDialog", so that it's clearer > which dialog this refers to, when the event is handled on the JavaScript side. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:63: On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Please remove this blank line. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:93: chrome.send('closeDialog'); On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Likewise, this should be named 'closeImportDialog'. (Actually, does this > close the *import lock* dialog? If so, it should probably be named > 'closeImportLockDialog'.) Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:132: this.browserProxy_.closeDialog(); On 2017/03/27 15:21:52, Ilya Sherman wrote: > This line is confusing: One dialog is already being closed, and here you're > sending a message to close a different dialog... but that's not very obvious > from context. Please add a comment here to explain. (Having a more specific > method name than "closeDialog()" will also help.) Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:33: void HideImportLockDialog() { On 2017/03/27 15:21:52, Ilya Sherman wrote: > You'll need to also implement this method for Mac, within > chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm. Hi Ilya Sherman, Thanks for pointing this out , I have implemented this function in import_lock_dialog_cocoa.mm but implementing it I am facing problem. I made the lock_alert as global variable and trying to access the same in HideImportLockDialog but getting Error:declaration requires an exit-time destructor. I am not aware of Mac programming , I tried to implement seeing some examples. Please help me on this. Thanks https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:39: views::Widget* ImportLockDialogView::m_widget = NULL; On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: s/NULL/nullptr Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:43: if (m_widget) { On 2017/03/27 15:21:52, Ilya Sherman wrote: > It looks like the widget can be destroyed without m_widget ever being updated. > I think you'll need to register a WidgetObserver that can listen for > OnWidgetDestroying() and reset this pointer to null. Here I guess there won't be any issue as even if it is called without calling ShowImportLockDialog m_widget will be NULL so further API execution will not happen. and ShowImportLockDialog cannot be called multiple times as the button gets Disabled in this Scenario untill Hide happens. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:45: m_widget->CloseNow(); On 2017/03/27 15:21:52, Ilya Sherman wrote: > Hmm, is it actually necessary to call both of these methods? I'm not very > familiar with Views, but it seems a bit surprising that you'd need both. It > looks like the Close() method should do what you need. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/importer/import_lock_dialog_view.h (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.h:43: static views::Widget* m_widget; On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Please move this to the implementation file, name it g_widget, and document > it. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_import_data_handler.cc (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_import_data_handler.cc:74: "closeDialog", On 2017/03/27 15:21:52, Ilya Sherman wrote: > Again, you'll want a more specific name, that explicitly mentions importing. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_import_data_handler.cc:74: "closeDialog", On 2017/03/27 15:21:52, Ilya Sherman wrote: > Again, you'll want a more specific name, that explicitly mentions importing. Done.
Hi Ilya Sherman, Thanks for your review Comments, but am stuck with the Cocoa Implementation. Please have a look at my latest submission. Thanks again for your guidance. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/importer... File chrome/browser/importer/importer_lock_dialog.h (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/importer... chrome/browser/importer/importer_lock_dialog.h:18: void HideImportLockDialog(); On 2017/03/27 15:21:52, Ilya Sherman wrote: > Please add documentation for this function. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/importer... chrome/browser/importer/importer_lock_dialog.h:18: void HideImportLockDialog(); On 2017/03/27 15:21:52, Ilya Sherman wrote: > Please add documentation for this function. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_browser_proxy.js (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:58: importFromBookmarksFile: function() {}, On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:60: * Close the Dialog if it is Opened. On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: s/Dialog/dialog; s/Opened/opened. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:61: */ On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Please fix up the indentation of the asterisks in this comment. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:62: closeDialog: function() {}, On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: This should probably be named "closeImportDialog", so that it's clearer > which dialog this refers to, when the event is handled on the JavaScript side. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:63: On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Please remove this blank line. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_browser_proxy.js:93: chrome.send('closeDialog'); On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Likewise, this should be named 'closeImportDialog'. (Actually, does this > close the *import lock* dialog? If so, it should probably be named > 'closeImportLockDialog'.) Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:132: this.browserProxy_.closeDialog(); On 2017/03/27 15:21:52, Ilya Sherman wrote: > This line is confusing: One dialog is already being closed, and here you're > sending a message to close a different dialog... but that's not very obvious > from context. Please add a comment here to explain. (Having a more specific > method name than "closeDialog()" will also help.) Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:33: void HideImportLockDialog() { On 2017/03/27 15:21:52, Ilya Sherman wrote: > You'll need to also implement this method for Mac, within > chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm. Hi Ilya Sherman, Thanks for pointing this out , I have implemented this function in import_lock_dialog_cocoa.mm but implementing it I am facing problem. I made the lock_alert as global variable and trying to access the same in HideImportLockDialog but getting Error:declaration requires an exit-time destructor. I am not aware of Mac programming , I tried to implement seeing some examples. Please help me on this. Thanks https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:39: views::Widget* ImportLockDialogView::m_widget = NULL; On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: s/NULL/nullptr Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:43: if (m_widget) { On 2017/03/27 15:21:52, Ilya Sherman wrote: > It looks like the widget can be destroyed without m_widget ever being updated. > I think you'll need to register a WidgetObserver that can listen for > OnWidgetDestroying() and reset this pointer to null. Here I guess there won't be any issue as even if it is called without calling ShowImportLockDialog m_widget will be NULL so further API execution will not happen. and ShowImportLockDialog cannot be called multiple times as the button gets Disabled in this Scenario untill Hide happens. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:45: m_widget->CloseNow(); On 2017/03/27 15:21:52, Ilya Sherman wrote: > Hmm, is it actually necessary to call both of these methods? I'm not very > familiar with Views, but it seems a bit surprising that you'd need both. It > looks like the Close() method should do what you need. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/importer/import_lock_dialog_view.h (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.h:43: static views::Widget* m_widget; On 2017/03/27 15:21:52, Ilya Sherman wrote: > nit: Please move this to the implementation file, name it g_widget, and document > it. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_import_data_handler.cc (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_import_data_handler.cc:74: "closeDialog", On 2017/03/27 15:21:52, Ilya Sherman wrote: > Again, you'll want a more specific name, that explicitly mentions importing. Done. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_import_data_handler.cc:74: "closeDialog", On 2017/03/27 15:21:52, Ilya Sherman wrote: > Again, you'll want a more specific name, that explicitly mentions importing. Done.
Thanks, Nikhil! https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:43: if (m_widget) { On 2017/03/28 11:22:00, nikhil.sahni wrote: > On 2017/03/27 15:21:52, Ilya Sherman wrote: > > It looks like the widget can be destroyed without m_widget ever being updated. > > > I think you'll need to register a WidgetObserver that can listen for > > OnWidgetDestroying() and reset this pointer to null. > > Here I guess there won't be any issue as even if it is called without calling > ShowImportLockDialog m_widget will be NULL so further API execution will not > happen. and ShowImportLockDialog cannot be called multiple times as the button > gets Disabled in this Scenario untill Hide happens. At a minimum, you'll want to reset g_widget to null after closing it. But, what happens if the user goes through the normal flow, and upon seeing that dialog, closes Firefox? I *think* the dialog will be destroyed after the user presses "Continue". Then, at some later time, the code will call HideImportLockDialog(), which will reach this code path. This code should not try to close the dialog if it has already been destroyed. Am I correct that this is currently an issue? If not, what am I misunderstanding? https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/importe... File chrome/browser/importer/importer_lock_dialog.h (right): https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/importe... chrome/browser/importer/importer_lock_dialog.h:20: // warning dialog opened by using ShowImportLockDialog function nit: "This function is called by an ImporterHost, and closes the Firefox profile warning dialog." https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:133: //if it is shown on the screen. nit: Thanks for adding a comment here. I'd rephrase it something along the lines of: "It's possible for the user to close the import dialog while a sub-dialog is still showing. Make sure to close that sub-dialog as well." https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm (right): https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm:43: lock_alert.reset(); I just tested this on my Mac, and I couldn't actually reproduce the original bug. AFAICT, while the sheet is showing, there is no way to close the parent dialog. So, you could probably actually leave this method body empty, with a comment explaining that there is no need for this code path on Mac. https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:40: // using method CreateDialogWidget. nit: I'd simplify this comment to "The currently open dialog widget, or null if no dialog is currently open." https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:41: static views::Widget* g_widget = nullptr; nit: Please move this to an anonymous namespace. https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:46: g_widget->CloseNow(); I'm pretty sure, based on reading the documentation, that you should call Close() rather than CloseNow(). Do you have a reason for calling CloseNow() instead?
Have addressed the Review Comments. Thanks for your suggestions ,it helped a lot. https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:43: if (m_widget) { On 2017/03/29 06:29:02, Ilya Sherman wrote: > On 2017/03/28 11:22:00, nikhil.sahni wrote: > > On 2017/03/27 15:21:52, Ilya Sherman wrote: > > > It looks like the widget can be destroyed without m_widget ever being > updated. > > > > > I think you'll need to register a WidgetObserver that can listen for > > > OnWidgetDestroying() and reset this pointer to null. > > > > Here I guess there won't be any issue as even if it is called without calling > > ShowImportLockDialog m_widget will be NULL so further API execution will not > > happen. and ShowImportLockDialog cannot be called multiple times as the button > > gets Disabled in this Scenario untill Hide happens. > > At a minimum, you'll want to reset g_widget to null after closing it. But, what > happens if the user goes through the normal flow, and upon seeing that dialog, > closes Firefox? I *think* the dialog will be destroyed after the user presses > "Continue". Then, at some later time, the code will call > HideImportLockDialog(), which will reach this code path. This code should not > try to close the dialog if it has already been destroyed. Am I correct that > this is currently an issue? If not, what am I misunderstanding? yes you are right this case needs to be handled, but no need of Observer as you already have ImportLockDialogView::Accept function which gets called on pressing Continue as you pointed out , there I am setting g_widget = nullptr . https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/importe... File chrome/browser/importer/importer_lock_dialog.h (right): https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/importe... chrome/browser/importer/importer_lock_dialog.h:20: // warning dialog opened by using ShowImportLockDialog function On 2017/03/29 06:29:02, Ilya Sherman wrote: > nit: "This function is called by an ImporterHost, and closes the Firefox profile > warning dialog." Done. https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:133: //if it is shown on the screen. On 2017/03/29 06:29:02, Ilya Sherman wrote: > nit: Thanks for adding a comment here. I'd rephrase it something along the lines > of: > > "It's possible for the user to close the import dialog while a sub-dialog is > still showing. Make sure to close that sub-dialog as well." Done. https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm (right): https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm:43: lock_alert.reset(); On 2017/03/29 06:29:02, Ilya Sherman wrote: > I just tested this on my Mac, and I couldn't actually reproduce the original > bug. AFAICT, while the sheet is showing, there is no way to close the parent > dialog. So, you could probably actually leave this method body empty, with a > comment explaining that there is no need for this code path on Mac. Done. https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:40: // using method CreateDialogWidget. On 2017/03/29 06:29:02, Ilya Sherman wrote: > nit: I'd simplify this comment to "The currently open dialog widget, or null if > no dialog is currently open." Done. https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:41: static views::Widget* g_widget = nullptr; On 2017/03/29 06:29:02, Ilya Sherman wrote: > nit: Please move this to an anonymous namespace. Done. https://codereview.chromium.org/2769383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:46: g_widget->CloseNow(); On 2017/03/29 06:29:02, Ilya Sherman wrote: > I'm pretty sure, based on reading the documentation, that you should call > Close() rather than CloseNow(). Do you have a reason for calling CloseNow() > instead? Done.
https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:133: //still showing. Make sure to close that sub-dialog as well. nit: Please make sure there's a space following "//", and update the text wrapping -- the first line is currently one character too long. https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm (right): https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm:41: // Details. nit: I'd rephrase this a bit to something like "This code path is not needed for Mac, as the user must explicitly close the sheet before any situation can arise in which we'd need to close it programmatically. See https://crbug.com/672428 for more details. (Of note, "Mac" is used for the OS, "MAC" is used for "MAC addresses".) https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:48: g_widget->Close(); Should you set g_widget to nullptr here? https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:100: g_widget = nullptr; What about if the user cancels the dialog instead? I think it's safer to listen for the notification, in case there's any other way for the dialog to be closed, than trying to catch every possible individual code path.
Hi Ilya, Please help me on the comments. Thanks https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:133: //still showing. Make sure to close that sub-dialog as well. On 2017/03/30 22:14:32, Ilya Sherman wrote: > nit: Please make sure there's a space following "//", and update the text > wrapping -- the first line is currently one character too long. Done. https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm (right): https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm:41: // Details. On 2017/03/30 22:14:32, Ilya Sherman wrote: > nit: I'd rephrase this a bit to something like "This code path is not needed for > Mac, as the user must explicitly close the sheet before any situation can arise > in which we'd need to close it programmatically. See https://crbug.com/672428 > for more details. (Of note, "Mac" is used for the OS, "MAC" is used for "MAC > addresses".) Done. https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:48: g_widget->Close(); On 2017/03/30 22:14:32, Ilya Sherman wrote: > Should you set g_widget to nullptr here? Done. https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:100: g_widget = nullptr; On 2017/03/30 22:14:32, Ilya Sherman wrote: > What about if the user cancels the dialog instead? I think it's safer to listen > for the notification, in case there's any other way for the dialog to be closed, > than trying to catch every possible individual code path. In case of CANCEL even the IMPORT Dialog gets closed as import is Cancelled so in this case there is no possibility for the user to explicitly cancel that. I also studied views::WidgetObserver class, I can implement soemthing like below void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { g_widget = nullptr; } but have to do AddObserver here , I am stuck as where to do that here . Can you please guide me on this, thanks
Added Patch to cover all the possibilities of Exit for your kind consideration. Thanks https://codereview.chromium.org/2769383002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:73: g_widget = nullptr; Even Destructor is called on Exiting so added check here as well. https://codereview.chromium.org/2769383002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:103: g_widget = nullptr; On Pressing "Continue" this function gets called so setting to nullptr here as well. https://codereview.chromium.org/2769383002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:110: g_widget = nullptr; On pressing "Skip Import" and "Cross Button of the Lock Dialog" this function gets called as it is mapped with CANCEL so for safety making it nullptr here as well.
Thanks, Nikhil, I think this is getting really close! https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:100: g_widget = nullptr; On 2017/03/31 15:21:25, nikhil.sahni wrote: > On 2017/03/30 22:14:32, Ilya Sherman wrote: > > What about if the user cancels the dialog instead? I think it's safer to > listen > > for the notification, in case there's any other way for the dialog to be > closed, > > than trying to catch every possible individual code path. > > In case of CANCEL even the IMPORT Dialog gets closed as import is Cancelled so > in this case > there is no possibility for the user to explicitly cancel that. > > I also studied views::WidgetObserver class, > I can implement soemthing like below > > void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { > g_widget = nullptr; > } > > but have to do AddObserver here , I am stuck as where to do that here . > > Can you please guide me on this, thanks Yes, I think implementing ImportLockDialogView::OnWidgetDestroying() is the best approach. Please implement it as void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { g_widget = nullptr; widget->RemoveObserver(this); } You can call g_widget->AddObserver() as part of the ImportLockDialogView::Show() function. To do so, you'll need to capture the "new ImportLockDialogView(callback)" in a local variable. If you implement this approach, you then don't need any of the "g_widget = nullptr" lines below, which is a lot less fragile to edge cases and code refactoring.
Hi Ilya, Implemented the Widget Observer and it works well. Thanks for your help on this. Please let me know your review comments. Thanks https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:100: g_widget = nullptr; On 2017/04/04 05:39:52, Ilya Sherman wrote: > On 2017/03/31 15:21:25, nikhil.sahni wrote: > > On 2017/03/30 22:14:32, Ilya Sherman wrote: > > > What about if the user cancels the dialog instead? I think it's safer to > > listen > > > for the notification, in case there's any other way for the dialog to be > > closed, > > > than trying to catch every possible individual code path. > > > > In case of CANCEL even the IMPORT Dialog gets closed as import is Cancelled so > > in this case > > there is no possibility for the user to explicitly cancel that. > > > > I also studied views::WidgetObserver class, > > I can implement soemthing like below > > > > void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { > > g_widget = nullptr; > > } > > > > but have to do AddObserver here , I am stuck as where to do that here . > > > > Can you please guide me on this, thanks > > Yes, I think implementing ImportLockDialogView::OnWidgetDestroying() is the best > approach. Please implement it as > > void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { > g_widget = nullptr; > widget->RemoveObserver(this); > } > > You can call g_widget->AddObserver() as part of the ImportLockDialogView::Show() > function. To do so, you'll need to capture the "new > ImportLockDialogView(callback)" in a local variable. > > If you implement this approach, you then don't need any of the "g_widget = > nullptr" lines below, which is a lot less fragile to edge cases and code > refactoring. Done.
On 2017/04/04 09:21:41, nikhil.sahni wrote: > Hi Ilya, > > Implemented the Widget Observer and it works well. > Thanks for your help on this. > > Please let me know your review comments. > > Thanks > > https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): > > https://codereview.chromium.org/2769383002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/importer/import_lock_dialog_view.cc:100: g_widget = > nullptr; > On 2017/04/04 05:39:52, Ilya Sherman wrote: > > On 2017/03/31 15:21:25, nikhil.sahni wrote: > > > On 2017/03/30 22:14:32, Ilya Sherman wrote: > > > > What about if the user cancels the dialog instead? I think it's safer to > > > listen > > > > for the notification, in case there's any other way for the dialog to be > > > closed, > > > > than trying to catch every possible individual code path. > > > > > > In case of CANCEL even the IMPORT Dialog gets closed as import is Cancelled > so > > > in this case > > > there is no possibility for the user to explicitly cancel that. > > > > > > I also studied views::WidgetObserver class, > > > I can implement soemthing like below > > > > > > void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { > > > g_widget = nullptr; > > > } > > > > > > but have to do AddObserver here , I am stuck as where to do that here . > > > > > > Can you please guide me on this, thanks > > > > Yes, I think implementing ImportLockDialogView::OnWidgetDestroying() is the > best > > approach. Please implement it as > > > > void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { > > g_widget = nullptr; > > widget->RemoveObserver(this); > > } > > > > You can call g_widget->AddObserver() as part of the > ImportLockDialogView::Show() > > function. To do so, you'll need to capture the "new > > ImportLockDialogView(callback)" in a local variable. > > > > If you implement this approach, you then don't need any of the "g_widget = > > nullptr" lines below, which is a lot less fragile to edge cases and code > > refactoring. > > Done.
Thanks -- looks great! I think this should be good to go after a final set of nits are addressed. Thanks again for the contribution to Chromium, and for your patience in working with me on this! https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:40: // using method CreateDialogWidget. nit: Please move this comment to be within the namespace, just above the g_widget declaration. And, please simplify it a bit to something like "A reference to the currently active widget backing the import lock dialog, or null if the dialog is not open." https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:43: } nit: Please move this to the top of the file, just below the "using" declaration on line 23 (well, please also leave a blank line following the using declaration =]) https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:49: g_widget = nullptr; nit: This line should now be unnecessary, since the observer should handle it. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:51: } nit: Please move this to be below the Show() function, to match the order of ShowImportLockDialog and HideImportLockDIalog. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:56: ImportLockDialogView* import_lock_dialog_view = Optional nit: I'd name this simply "view" or "dialog_view". https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:61: g_widget->AddObserver(import_lock_dialog_view); nit: Please add the observer before showing the widget, just to be a bit safer. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:78: void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { nit: Please also add DCHECK_EQ(widget, g_widget); https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.h (right): https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.h:43: // views::WidgetObserver overrides: nit: Please follow the pattern used above, and simply write "// views::WidgetObserver:" here.
Addressed the review comments. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:40: // using method CreateDialogWidget. On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please move this comment to be within the namespace, just above the > g_widget declaration. And, please simplify it a bit to something like "A > reference to the currently active widget backing the import lock dialog, or null > if the dialog is not open." Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:43: } On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please move this to the top of the file, just below the "using" declaration > on line 23 (well, please also leave a blank line following the using declaration > =]) Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:49: g_widget = nullptr; On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: This line should now be unnecessary, since the observer should handle it. Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:51: } On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please move this to be below the Show() function, to match the order of > ShowImportLockDialog and HideImportLockDIalog. Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:56: ImportLockDialogView* import_lock_dialog_view = On 2017/04/04 16:47:13, Ilya Sherman wrote: > Optional nit: I'd name this simply "view" or "dialog_view". Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:61: g_widget->AddObserver(import_lock_dialog_view); On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please add the observer before showing the widget, just to be a bit safer. Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:78: void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please also add DCHECK_EQ(widget, g_widget); Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.h (right): https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.h:43: // views::WidgetObserver overrides: On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please follow the pattern used above, and simply write "// > views::WidgetObserver:" here. Done.
Addressed the review comments. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:40: // using method CreateDialogWidget. On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please move this comment to be within the namespace, just above the > g_widget declaration. And, please simplify it a bit to something like "A > reference to the currently active widget backing the import lock dialog, or null > if the dialog is not open." Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:43: } On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please move this to the top of the file, just below the "using" declaration > on line 23 (well, please also leave a blank line following the using declaration > =]) Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:49: g_widget = nullptr; On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: This line should now be unnecessary, since the observer should handle it. Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:51: } On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please move this to be below the Show() function, to match the order of > ShowImportLockDialog and HideImportLockDIalog. Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:56: ImportLockDialogView* import_lock_dialog_view = On 2017/04/04 16:47:13, Ilya Sherman wrote: > Optional nit: I'd name this simply "view" or "dialog_view". Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:61: g_widget->AddObserver(import_lock_dialog_view); On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please add the observer before showing the widget, just to be a bit safer. Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:78: void ImportLockDialogView::OnWidgetDestroying(views::Widget* widget) { On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please also add DCHECK_EQ(widget, g_widget); Done. https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.h (right): https://codereview.chromium.org/2769383002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.h:43: // views::WidgetObserver overrides: On 2017/04/04 16:47:13, Ilya Sherman wrote: > nit: Please follow the pattern used above, and simply write "// > views::WidgetObserver:" here. Done.
Thanks, Nikhil! This CL LGTM % some nits: https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:25: namespace { nit: Please leave a blank line after this one. https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:27: // using method CreateDialogWidget. nit: Please re-wrap this comment to 80-col. https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:27: // using method CreateDialogWidget. Please simplify this comment a bit to something like // A reference to the currently active widget backing the import lock dialog, or // null if the dialog is not open. https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:28: static views::Widget* g_widget = nullptr; nit: Please leave a blank line after this one. https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:49: g_widget = views::DialogDelegate::CreateDialogWidget(import_lock_dialog_view, Please update the variable name import_lock_dialog_view used here to dialog_view.
Fixed nits, Thank you https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right): https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:25: namespace { On 2017/04/05 06:15:29, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:27: // using method CreateDialogWidget. On 2017/04/05 06:15:29, Ilya Sherman wrote: > Please simplify this comment a bit to something like > > // A reference to the currently active widget backing the import lock dialog, or > // null if the dialog is not open. Done. https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:28: static views::Widget* g_widget = nullptr; On 2017/04/05 06:15:29, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/2769383002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/importer/import_lock_dialog_view.cc:49: g_widget = views::DialogDelegate::CreateDialogWidget(import_lock_dialog_view, On 2017/04/05 06:15:29, Ilya Sherman wrote: > Please update the variable name import_lock_dialog_view used here to > dialog_view. Done.
isherman@chromium.org changed reviewers: + dbeam@chromium.org, ellyjones@chromium.org, msw@chromium.org
+Dan for //chrome/browser/resources/settings/* and //chrome/browser/ui/webui/settings/* +Mike for //chrome/browser/ui/views/* +Elly for //chrome/browser/ui/cocoa/*
https://codereview.chromium.org/2769383002/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm (right): https://codereview.chromium.org/2769383002/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/importer/import_lock_dialog_cocoa.mm:42: // programmatically. See https://crbug.com/672428 for more details. Views should probably instead use a tab or browser-modal dialog to prevent interaction with the import dialog while the lock dialog is showing? That would make Views match Cocoa in behavior, and make the weird sub-dialog closing logic unnecessary. All you should need for browser window modality is to pass the |parent| to CreateDialogWidget and override ImportLockDialogView::GetModalType() to return ui::MODAL_TYPE_WINDOW. For tab modality, see users of constrained_window::ShowWebModalDialogViews
Thanks Ilya and Msw for your comments and time.
I tried to check the way what "Msw" suggested:
1)Adding parent while creation
views::DialogDelegate::CreateDialogWidget(
new ImportLockDialogView(callback), NULL, parent)->Show();
2) Overriden ImportLockDialogView::GetModalType() to return
ui::MODAL_TYPE_WINDOW.ui::ModalType
ImportLockDialogView::GetModalType() const {
return ui::MODAL_TYPE_WINDOW;
}
But Issue is still happening and does not solve after these changes , can you
please guide me if I am doing anything wrong.
chrome/browser/ui/cocoa/* lgtm
On 2017/04/06 07:24:59, nikhil.sahni wrote:
> Thanks Ilya and Msw for your comments and time.
>
> I tried to check the way what "Msw" suggested:
>
> 1)Adding parent while creation
> views::DialogDelegate::CreateDialogWidget(
> new ImportLockDialogView(callback), NULL, parent)->Show();
>
> 2) Overriden ImportLockDialogView::GetModalType() to return
> ui::MODAL_TYPE_WINDOW.ui::ModalType
>
> ImportLockDialogView::GetModalType() const {
> return ui::MODAL_TYPE_WINDOW;
> }
>
> But Issue is still happening and does not solve after these changes , can you
> please guide me if I am doing anything wrong.
I'm not sure exactly what you're missing. Study the bookmark edit dialog, it
does the right thing:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/bookmarks/bookma...
Hi MSW/Ilya,
I further checked on this and I get below logs:
25306:25306:0411/121536.479824:ERROR:import_lock_dialog_view.cc(93)] NIKHIL
ImportLockDialogView::GetModalType() function called.
After this I get this log :
[25306:25306:0411/121536.479883:ERROR:desktop_window_tree_host_x11.cc(1148)] Not
implemented reached in virtual void
views::DesktopWindowTreeHostX11::InitModalType(ui::ModalType)
and when I went into the function I can see the implementation is missing , Is
This The Reason It is not Working here ?
void DesktopWindowTreeHostX11::InitModalType(ui::ModalType modal_type) {
switch (modal_type) {
case ui::MODAL_TYPE_NONE:
break;
default:
// TODO(erg): Figure out under what situations |modal_type| isn't
// none. The comment in desktop_native_widget_aura.cc suggests that this
// is rare.
NOTIMPLEMENTED();
}
}
Do we need to implement this functionality or if any other suggestion.
msw@chromium.org changed reviewers: + erg@chromium.org
On 2017/04/11 06:50:30, nikhil.sahni wrote:
> Hi MSW/Ilya,
>
> I further checked on this and I get below logs:
>
> 25306:25306:0411/121536.479824:ERROR:import_lock_dialog_view.cc(93)] NIKHIL
> ImportLockDialogView::GetModalType() function called.
>
> After this I get this log :
>
> [25306:25306:0411/121536.479883:ERROR:desktop_window_tree_host_x11.cc(1148)]
Not
> implemented reached in virtual void
> views::DesktopWindowTreeHostX11::InitModalType(ui::ModalType)
>
> and when I went into the function I can see the implementation is missing , Is
> This The Reason It is not Working here ?
>
> void DesktopWindowTreeHostX11::InitModalType(ui::ModalType modal_type) {
> switch (modal_type) {
> case ui::MODAL_TYPE_NONE:
> break;
> default:
> // TODO(erg): Figure out under what situations |modal_type| isn't
> // none. The comment in desktop_native_widget_aura.cc suggests that this
> // is rare.
> NOTIMPLEMENTED();
> }
> }
It's hard to tell what's wrong without seeing your code... I think it should be
going through NativeWidgetAura, not
DesktopNativeWidgetAura/DesktopWindowTreeHostX11 for a window-modal dialog that
is a child of the browser window (ie. ensure the dialog is parented correctly).
You should try to mimic the edit bookmark dialog, which does exactly what I'm
suggesting and works fine on Linux. +CC erg in case I'm mistaken. If
window-modality still isn't working, perhaps look at components/web_modal for
making a tab-modal dialog, which would also be appropriate in this case.
|
