Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9265026:
Implement restart on Idle for Kiosk Mode. (Closed)
|
Created:
8 years, 11 months ago by rkc Modified:
8 years, 9 months ago Reviewers:
Will Drewry, xiyuan, Rick Byers, Evan Stade, flackr, arv (Not doing code reviews), James Hawkins, satorux1 CC:
chromium-reviews, arv (Not doing code reviews), stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, zel, Chris Masone, Will Drewry Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement restart on Idle for Kiosk Mode.
Implement the Kiosk Mode flag and the idle timeout for a logged in user/guest if the flag is enabled.
Currently the timeout to restart is hard coded to 2m total with the dialog showing in the last 20 seconds. If the machine goes active while the dialog is counting down, the restart will be aborted and the state reset.
R=flackr@chromium.org,rbyers@chromium.org,xiyuan@chromium.org
BUG=23363
TEST=Tested with the flag enabled to ensure that guest and user login acts as expected; tested without the flag to ensure regular functionality is not affected.
Patch Set 1 #
Total comments: 45
Patch Set 2 : Renamed kiosk->retail and incorporated review comments. #
Total comments: 28
Patch Set 3 : Review changes. #Patch Set 4 : Merge. #
Total comments: 48
Patch Set 5 : Review comments. #
Total comments: 1
Messages
Total messages: 25 (0 generated)
http://codereview.chromium.org/9265026/diff/1/chrome/browser/chromeos/kiosk_m... File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screen_locker.cc (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/chromeos/kiosk_m... chrome/browser/chromeos/kiosk_mode/kiosk_mode_screen_locker.cc:11: #include "chrome/browser/ui/browser_dialogs.h" nit: sort. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... File chrome/browser/resources/kiosk_mode_logout_dialog.css (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.css:20: border-bottom: 1px solid #bbbbbb; nit: sort. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.css:30: font-family: Arial, Helvetica, sans-serif; nit: sort. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... File chrome/browser/resources/kiosk_mode_logout_dialog.js (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.js:26: $('warning').innerHTML = warningText + ' ' + timeToRestart + 's'; Use template parameter to insert timeToRestart using LocalStrings.getStringF. http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... File chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc:136: nit: Remove extra newline.
http://codereview.chromium.org/9265026/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/9265026/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:5047: <message name="IDS_FLAGS_ENABLE_KIOSK_MODE_NAME" desc="Title for the flag to enable kiosk mode."> nit: put those under <if expr="pp_ifdef('chromeos')"> since they are only used in chromeos. http://codereview.chromium.org/9265026/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:5808: <ph name="LINE_BREAK"><br /><br /></ph> nit: prefer to use <br> rather than <br />. The later is allowed in HTML5 but it is intended for XHTML. http://codereview.chromium.org/9265026/diff/1/chrome/browser/browser_resource... File chrome/browser/browser_resources.grd (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/browser_resource... chrome/browser/browser_resources.grd:76: <include name="IDR_KIOSK_MODE_LOGOUT_DIALOG_HTML" file="resources\kiosk_mode_logout_dialog.html" flattenhtml="true" allowexternalscript="true" type="BINDATA" /> Prefer to let grit to flatten and inline the css and js files so that we have less resource Ids being used. That is, put relative path in html for css/js resources and grit should inline them in and you don't need to define resources for them and manually add them when creating your source. http://codereview.chromium.org/9265026/diff/1/chrome/browser/chromeos/kiosk_m... File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screen_locker.cc (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/chromeos/kiosk_m... chrome/browser/chromeos/kiosk_mode/kiosk_mode_screen_locker.cc:20: namespace { nit: blank line before and after code block in namespace http://codereview.chromium.org/9265026/diff/1/chrome/browser/chromeos/kiosk_m... chrome/browser/chromeos/kiosk_mode/kiosk_mode_screen_locker.cc:21: const int64 kLoginIdleTimeout = 100; // seconds nit: This is not 2m. :) http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... File chrome/browser/resources/kiosk_mode_logout_dialog.html (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.html:10: <script src="chrome://resources/js/cr/ui.js"></script> we probably don't need this. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.html:13: <script src="chrome://kiosk-mode-logout/kiosk_mode_logout_dialog.js"></script> If you want to try grit's inlining, change this to: <script src="kiosk_mode_logout_dialog.js"></script> http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.html:19: <div id="title" class="text" i18n-content="title"></div> nit: 2-space ident http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.html:22: <div id="warning" class="text" i18n-values=".innerHTML:warning"></div> No need to have i18n-values here since you will set the content via js. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... File chrome/browser/resources/kiosk_mode_logout_dialog.js (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.js:12: timeToRestart -= 1; nit: --timeToRestart http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.js:13: $('warning').innerHTML = warningText + ' ' + timeToRestart + 's'; Use LocalStrings.getStringF instead of string concat http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/browser_dialo... File chrome/browser/ui/browser_dialogs.h (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/browser_dialo... chrome/browser/ui/browser_dialogs.h:67: // Shows or close the logout dialog for Kiosk Mode nit: close -> closes and add a "." at the end. http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/browser_dialo... chrome/browser/ui/browser_dialogs.h:69: void CloseKioskModeLogoutDialog(); I saw implementations of those functions for chromeos but not for other platforms. We need to have stub for other platforms or put those under #if defined(CHROMEOS). http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... File chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc:40: namespace { nit: insert blank lines before and after code block in namespace per style guide. namespace { <blank> ... <blank> } // namespace http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc:111: handlers->push_back(handler_); nit: fix indent http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc:128: bool* out_close_dialog) { nit: align with the first arg http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... File chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.h (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.h:53: class KioskModeLogoutDialogHandler : public content::WebUIMessageHandler { Think you can move this into cc file since it should not be referenced by outside.
I've incorporated the review comments; additionally with discussion on the design doc, it was becoming apparent that 'kiosk mode' isn't the term we want to go with but 'retail mode' instead, so changed the code to reflect that. http://codereview.chromium.org/9265026/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/9265026/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:5047: <message name="IDS_FLAGS_ENABLE_KIOSK_MODE_NAME" desc="Title for the flag to enable kiosk mode."> On 2012/01/20 18:23:44, xiyuan wrote: > nit: put those under <if expr="pp_ifdef('chromeos')"> since they are only used > in chromeos. Done. http://codereview.chromium.org/9265026/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:5808: <ph name="LINE_BREAK"><br /><br /></ph> On 2012/01/20 18:23:44, xiyuan wrote: > nit: prefer to use <br> rather than <br />. The later is allowed in HTML5 but it > is intended for XHTML. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/browser_resource... File chrome/browser/browser_resources.grd (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/browser_resource... chrome/browser/browser_resources.grd:76: <include name="IDR_KIOSK_MODE_LOGOUT_DIALOG_HTML" file="resources\kiosk_mode_logout_dialog.html" flattenhtml="true" allowexternalscript="true" type="BINDATA" /> On 2012/01/20 18:23:44, xiyuan wrote: > Prefer to let grit to flatten and inline the css and js files so that we have > less resource Ids being used. > > That is, put relative path in html for css/js resources and grit should inline > them in and you don't need to define resources for them and manually add them > when creating your source. I've changed the way the JS is included to make sure it's inlined - but these resource ID's still seem to be needed here since I am not using a derived data source; using the default ChromeWebUIDataSource doesn't work if we don't add the resource path for the js/css being included, even if flattened. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/chromeos/kiosk_m... File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screen_locker.cc (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/chromeos/kiosk_m... chrome/browser/chromeos/kiosk_mode/kiosk_mode_screen_locker.cc:11: #include "chrome/browser/ui/browser_dialogs.h" On 2012/01/20 16:16:58, flackr wrote: > nit: sort. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/chromeos/kiosk_m... chrome/browser/chromeos/kiosk_mode/kiosk_mode_screen_locker.cc:20: namespace { On 2012/01/20 18:23:44, xiyuan wrote: > nit: blank line before and after code block in namespace Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/chromeos/kiosk_m... chrome/browser/chromeos/kiosk_mode/kiosk_mode_screen_locker.cc:21: const int64 kLoginIdleTimeout = 100; // seconds On 2012/01/20 18:23:44, xiyuan wrote: > nit: This is not 2m. :) This is intended; this is meant to be idle time - dialog countdown time. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... File chrome/browser/resources/kiosk_mode_logout_dialog.css (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.css:20: border-bottom: 1px solid #bbbbbb; On 2012/01/20 16:16:58, flackr wrote: > nit: sort. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.css:30: font-family: Arial, Helvetica, sans-serif; On 2012/01/20 16:16:58, flackr wrote: > nit: sort. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... File chrome/browser/resources/kiosk_mode_logout_dialog.html (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.html:10: <script src="chrome://resources/js/cr/ui.js"></script> On 2012/01/20 18:23:44, xiyuan wrote: > we probably don't need this. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.html:13: <script src="chrome://kiosk-mode-logout/kiosk_mode_logout_dialog.js"></script> On 2012/01/20 18:23:44, xiyuan wrote: > If you want to try grit's inlining, change this to: > <script src="kiosk_mode_logout_dialog.js"></script> Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.html:19: <div id="title" class="text" i18n-content="title"></div> On 2012/01/20 18:23:44, xiyuan wrote: > nit: 2-space ident Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.html:22: <div id="warning" class="text" i18n-values=".innerHTML:warning"></div> On 2012/01/20 18:23:44, xiyuan wrote: > No need to have i18n-values here since you will set the content via js. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... File chrome/browser/resources/kiosk_mode_logout_dialog.js (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.js:12: timeToRestart -= 1; On 2012/01/20 18:23:44, xiyuan wrote: > nit: --timeToRestart Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.js:13: $('warning').innerHTML = warningText + ' ' + timeToRestart + 's'; On 2012/01/20 18:23:44, xiyuan wrote: > Use LocalStrings.getStringF instead of string concat Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/resources/kiosk_... chrome/browser/resources/kiosk_mode_logout_dialog.js:26: $('warning').innerHTML = warningText + ' ' + timeToRestart + 's'; On 2012/01/20 16:16:58, flackr wrote: > Use template parameter to insert timeToRestart using LocalStrings.getStringF. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/browser_dialo... File chrome/browser/ui/browser_dialogs.h (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/browser_dialo... chrome/browser/ui/browser_dialogs.h:67: // Shows or close the logout dialog for Kiosk Mode On 2012/01/20 18:23:44, xiyuan wrote: > nit: close -> closes and add a "." at the end. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/browser_dialo... chrome/browser/ui/browser_dialogs.h:69: void CloseKioskModeLogoutDialog(); Being a very specific ChromeOS dialog, this really doesn't belong here anyway - moved it to chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.h Done. On 2012/01/20 18:23:44, xiyuan wrote: > I saw implementations of those functions for chromeos but not for other > platforms. We need to have stub for other platforms or put those under #if > defined(CHROMEOS). http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... File chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc:40: namespace { On 2012/01/20 18:23:44, xiyuan wrote: > nit: insert blank lines before and after code block in namespace per style > guide. > > namespace { > <blank> > ... > <blank> > } // namespace > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc:111: handlers->push_back(handler_); On 2012/01/20 18:23:44, xiyuan wrote: > nit: fix indent Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc:128: bool* out_close_dialog) { On 2012/01/20 18:23:44, xiyuan wrote: > nit: align with the first arg Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.cc:136: On 2012/01/20 16:16:58, flackr wrote: > nit: Remove extra newline. Done. http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... File chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.h (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/ui/webui/chromeo... chrome/browser/ui/webui/chromeos/kiosk_mode_logout_dialog.h:53: class KioskModeLogoutDialogHandler : public content::WebUIMessageHandler { On 2012/01/20 18:23:44, xiyuan wrote: > Think you can move this into cc file since it should not be referenced by > outside. Done.
LGTM with nits http://codereview.chromium.org/9265026/diff/1/chrome/browser/browser_resource... File chrome/browser/browser_resources.grd (right): http://codereview.chromium.org/9265026/diff/1/chrome/browser/browser_resource... chrome/browser/browser_resources.grd:76: <include name="IDR_KIOSK_MODE_LOGOUT_DIALOG_HTML" file="resources\kiosk_mode_logout_dialog.html" flattenhtml="true" allowexternalscript="true" type="BINDATA" /> On 2012/01/26 03:37:23, Rahul Chaturvedi wrote: > On 2012/01/20 18:23:44, xiyuan wrote: > > Prefer to let grit to flatten and inline the css and js files so that we have > > less resource Ids being used. > > > > That is, put relative path in html for css/js resources and grit should inline > > them in and you don't need to define resources for them and manually add them > > when creating your source. > I've changed the way the JS is included to make sure it's inlined - but these > resource ID's still seem to be needed here since I am not using a derived data > source; using the default ChromeWebUIDataSource doesn't work if we don't add the > resource path for the js/css being included, even if flattened. > > Done. Think it's because you have allowexternalscript="true" and grit does not really flatten the js and css into the html. If grit does everything right, ChromeWebUIDataSource does not need to know the js/css flattened. All you need is to set strings.js with ChromeWebUIDataSource to make i18n stuff work. http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... File chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc (right): http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc:19: nit: one blank line should be enough. http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc:29: public content::NotificationObserver { nit: alignment http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc:81: nit: one blank line. http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... File chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.h (right): http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.h:20: nit: one blank line http://codereview.chromium.org/9265026/diff/10001/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/9265026/diff/10001/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_factory.cc:62: #include "chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h" nit: sort http://codereview.chromium.org/9265026/diff/10001/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_factory.cc:254: if (url.host() == chrome::kChromeUIRetailModeLogoutDialogHost) nit: keep the "if"s sorted on host constant name http://codereview.chromium.org/9265026/diff/10001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/9265026/diff/10001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:532: 'browser/chromeos/retail_mode/retail_mode_screen_locker.h', nit: sort http://codereview.chromium.org/9265026/diff/10001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:3758: 'browser/ui/webui/chromeos/retail_mode_logout_dialog.h', nit: sort http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:1200: const char kEnableRetailMode[] = "enable-retail-mode"; nit: sort http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switch... File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switch... chrome/common/chrome_switches.h:330: extern const char kEnableRetailMode[]; nit: sort http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants... chrome/common/url_constants.cc:59: const char kChromeUIRetailModeLogoutDialogURL[] = nit: sort and should probably put in #if defined(OS_CHROMEOS) section. http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants... chrome/common/url_constants.cc:163: const char kChromeUIRetailModeLogoutDialogHost[] = "retail-mode-logout"; nit: sort and put in #if defined(OS_CHROMEOS) section. http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.h File chrome/common/url_constants.h (right): http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants... chrome/common/url_constants.h:52: extern const char kChromeUIRetailModeLogoutDialogURL[]; nit: sort and put in #if defined(OS_CHROMEOS) section http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants... chrome/common/url_constants.h:149: extern const char kChromeUIRetailModeLogoutDialogHost[]; nit: sort and put in #if defined(OS_CHROMEOS) section
There is not a single test in this CL. Please test your new feature. On Thu, Jan 26, 2012 at 11:34 AM, <xiyuan@chromium.org> wrote: > LGTM with nits > > > > http://codereview.chromium.**org/9265026/diff/1/chrome/** > browser/browser_resources.grd<http://codereview.chromium.org/9265026/diff/1/chrome/browser/browser_resources.grd> > File chrome/browser/browser_**resources.grd (right): > > http://codereview.chromium.**org/9265026/diff/1/chrome/** > browser/browser_resources.grd#**newcode76<http://codereview.chromium.org/9265026/diff/1/chrome/browser/browser_resources.grd#newcode76> > chrome/browser/browser_**resources.grd:76: <include > name="IDR_KIOSK_MODE_LOGOUT_**DIALOG_HTML" > file="resources\kiosk_mode_**logout_dialog.html" flattenhtml="true" > allowexternalscript="true" type="BINDATA" /> > On 2012/01/26 03:37:23, Rahul Chaturvedi wrote: > >> On 2012/01/20 18:23:44, xiyuan wrote: >> > Prefer to let grit to flatten and inline the css and js files so >> > that we have > >> > less resource Ids being used. >> > >> > That is, put relative path in html for css/js resources and grit >> > should inline > >> > them in and you don't need to define resources for them and manually >> > add them > >> > when creating your source. >> I've changed the way the JS is included to make sure it's inlined - >> > but these > >> resource ID's still seem to be needed here since I am not using a >> > derived data > >> source; using the default ChromeWebUIDataSource doesn't work if we >> > don't add the > >> resource path for the js/css being included, even if flattened. >> > > Done. >> > > Think it's because you have allowexternalscript="true" and grit does not > really flatten the js and css into the html. If grit does everything > right, ChromeWebUIDataSource does not need to know the js/css flattened. > All you need is to set strings.js with ChromeWebUIDataSource to make > i18n stuff work. > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > browser/chromeos/retail_mode/**retail_mode_screen_locker.cc<http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc> > File chrome/browser/chromeos/**retail_mode/retail_mode_**screen_locker.cc > (right): > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > browser/chromeos/retail_mode/**retail_mode_screen_locker.cc#**newcode19<http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc#newcode19> > chrome/browser/chromeos/**retail_mode/retail_mode_**screen_locker.cc:19: > nit: one blank line should be enough. > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > browser/chromeos/retail_mode/**retail_mode_screen_locker.cc#**newcode29<http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc#newcode29> > chrome/browser/chromeos/**retail_mode/retail_mode_**screen_locker.cc:29: > public content::NotificationObserver { > nit: alignment > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > browser/chromeos/retail_mode/**retail_mode_screen_locker.cc#**newcode81<http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc#newcode81> > chrome/browser/chromeos/**retail_mode/retail_mode_**screen_locker.cc:81: > nit: one blank line. > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > browser/chromeos/retail_mode/**retail_mode_screen_locker.h<http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.h> > File chrome/browser/chromeos/**retail_mode/retail_mode_**screen_locker.h > (right): > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > browser/chromeos/retail_mode/**retail_mode_screen_locker.h#**newcode20<http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.h#newcode20> > chrome/browser/chromeos/**retail_mode/retail_mode_**screen_locker.h:20: > nit: one blank line > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > browser/ui/webui/chrome_web_**ui_factory.cc<http://codereview.chromium.org/9265026/diff/10001/chrome/browser/ui/webui/chrome_web_ui_factory.cc> > File chrome/browser/ui/webui/**chrome_web_ui_factory.cc (right): > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > browser/ui/webui/chrome_web_**ui_factory.cc#newcode62<http://codereview.chromium.org/9265026/diff/10001/chrome/browser/ui/webui/chrome_web_ui_factory.cc#newcode62> > chrome/browser/ui/webui/**chrome_web_ui_factory.cc:62: #include > "chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.h" > nit: sort > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > browser/ui/webui/chrome_web_**ui_factory.cc#newcode254<http://codereview.chromium.org/9265026/diff/10001/chrome/browser/ui/webui/chrome_web_ui_factory.cc#newcode254> > chrome/browser/ui/webui/**chrome_web_ui_factory.cc:254: if (url.host() == > chrome::**kChromeUIRetailModeLogoutDialo**gHost) > nit: keep the "if"s sorted on host constant name > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > chrome_browser.gypi<http://codereview.chromium.org/9265026/diff/10001/chrome/chrome_browser.gypi> > File chrome/chrome_browser.gypi (right): > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > chrome_browser.gypi#newcode532<http://codereview.chromium.org/9265026/diff/10001/chrome/chrome_browser.gypi#newcode532> > chrome/chrome_browser.gypi:**532: > 'browser/chromeos/retail_mode/**retail_mode_screen_locker.h', > nit: sort > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > chrome_browser.gypi#**newcode3758<http://codereview.chromium.org/9265026/diff/10001/chrome/chrome_browser.gypi#newcode3758> > chrome/chrome_browser.gypi:**3758: > 'browser/ui/webui/chromeos/**retail_mode_logout_dialog.h', > nit: sort > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/chrome_switches.cc<http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switches.cc> > File chrome/common/chrome_switches.**cc (right): > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/chrome_switches.cc#**newcode1200<http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switches.cc#newcode1200> > chrome/common/chrome_switches.**cc:1200: const char kEnableRetailMode[] > = "enable-retail-mode"; > nit: sort > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/chrome_switches.h<http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switches.h> > File chrome/common/chrome_switches.**h (right): > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/chrome_switches.h#**newcode330<http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switches.h#newcode330> > chrome/common/chrome_switches.**h:330: extern const char > kEnableRetailMode[]; > nit: sort > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/url_constants.cc<http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.cc> > File chrome/common/url_constants.cc (right): > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/url_constants.cc#**newcode59<http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.cc#newcode59> > chrome/common/url_constants.**cc:59: const char > kChromeUIRetailModeLogoutDialo**gURL[] = > nit: sort and should probably put in #if defined(OS_CHROMEOS) section. > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/url_constants.cc#**newcode163<http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.cc#newcode163> > chrome/common/url_constants.**cc:163: const char > kChromeUIRetailModeLogoutDialo**gHost[] = "retail-mode-logout"; > nit: sort and put in #if defined(OS_CHROMEOS) section. > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/url_constants.h<http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.h> > File chrome/common/url_constants.h (right): > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/url_constants.h#**newcode52<http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.h#newcode52> > chrome/common/url_constants.h:**52: extern const char > kChromeUIRetailModeLogoutDialo**gURL[]; > nit: sort and put in #if defined(OS_CHROMEOS) section > > http://codereview.chromium.**org/9265026/diff/10001/chrome/** > common/url_constants.h#**newcode149<http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.h#newcode149> > chrome/common/url_constants.h:**149: extern const char > kChromeUIRetailModeLogoutDialo**gHost[]; > nit: sort and put in #if defined(OS_CHROMEOS) section > > http://codereview.chromium.**org/9265026/<http://codereview.chromium.org/9265... >
Review changes incorporated. http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... File chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc (right): http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc:19: On 2012/01/26 19:34:18, xiyuan wrote: > nit: one blank line should be enough. Done. http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc:29: public content::NotificationObserver { On 2012/01/26 19:34:18, xiyuan wrote: > nit: alignment Done. http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.cc:81: On 2012/01/26 19:34:18, xiyuan wrote: > nit: one blank line. Done. http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... File chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.h (right): http://codereview.chromium.org/9265026/diff/10001/chrome/browser/chromeos/ret... chrome/browser/chromeos/retail_mode/retail_mode_screen_locker.h:20: On 2012/01/26 19:34:18, xiyuan wrote: > nit: one blank line Done. http://codereview.chromium.org/9265026/diff/10001/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/9265026/diff/10001/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_factory.cc:62: #include "chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h" On 2012/01/26 19:34:18, xiyuan wrote: > nit: sort Done. http://codereview.chromium.org/9265026/diff/10001/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_factory.cc:254: if (url.host() == chrome::kChromeUIRetailModeLogoutDialogHost) On 2012/01/26 19:34:18, xiyuan wrote: > nit: keep the "if"s sorted on host constant name Done. http://codereview.chromium.org/9265026/diff/10001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/9265026/diff/10001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:532: 'browser/chromeos/retail_mode/retail_mode_screen_locker.h', On 2012/01/26 19:34:18, xiyuan wrote: > nit: sort Done. http://codereview.chromium.org/9265026/diff/10001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:3758: 'browser/ui/webui/chromeos/retail_mode_logout_dialog.h', On 2012/01/26 19:34:18, xiyuan wrote: > nit: sort Done. http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:1200: const char kEnableRetailMode[] = "enable-retail-mode"; On 2012/01/26 19:34:18, xiyuan wrote: > nit: sort Done. http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switch... File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/9265026/diff/10001/chrome/common/chrome_switch... chrome/common/chrome_switches.h:330: extern const char kEnableRetailMode[]; On 2012/01/26 19:34:18, xiyuan wrote: > nit: sort Done. http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants... chrome/common/url_constants.cc:59: const char kChromeUIRetailModeLogoutDialogURL[] = On 2012/01/26 19:34:18, xiyuan wrote: > nit: sort and should probably put in #if defined(OS_CHROMEOS) section. Done. http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants... chrome/common/url_constants.cc:163: const char kChromeUIRetailModeLogoutDialogHost[] = "retail-mode-logout"; On 2012/01/26 19:34:18, xiyuan wrote: > nit: sort and put in #if defined(OS_CHROMEOS) section. Done. http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants.h File chrome/common/url_constants.h (right): http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants... chrome/common/url_constants.h:52: extern const char kChromeUIRetailModeLogoutDialogURL[]; On 2012/01/26 19:34:18, xiyuan wrote: > nit: sort and put in #if defined(OS_CHROMEOS) section Done. http://codereview.chromium.org/9265026/diff/10001/chrome/common/url_constants... chrome/common/url_constants.h:149: extern const char kChromeUIRetailModeLogoutDialogHost[]; On 2012/01/26 19:34:18, xiyuan wrote: > nit: sort and put in #if defined(OS_CHROMEOS) section Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/9265026/12008
Can't apply patch for file chrome/browser/ui/browser_dialogs.h. While running patch -p1 --forward --force; patching file chrome/browser/ui/browser_dialogs.h Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file chrome/browser/ui/browser_dialogs.h.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/9265026/12008
Can't apply patch for file chrome/browser/ui/browser_dialogs.h. While running patch -p1 --forward --force; patching file chrome/browser/ui/browser_dialogs.h Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file chrome/browser/ui/browser_dialogs.h.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/9265026/12010
Presubmit check for 9265026-12010 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/chromeos/dbus/power_manager_client.cc,chrome/browser/chromeos/dbus/power_manager_client.h,chrome/browser/chromeos/dbus/mock_power_manager_client.h,chrome/browser/ui/webui/chrome_web_ui_factory.cc Presubmit checks took 3.6s to calculate.
Added reviewers for OWNER's reviews. satorux for chrome/browser/chromeos/dbus estade,arv,jhawkins for chrome/browser/ui/webui
tl;dr I'd prefer this waited until the design work was done. Your current implementation requires that chrome be relaunched to get the auto-logout behavior. This works quite well for the demo account case, but it's not clear that we want that for the ephemeral, signed-in user case. If it goes that path, then it means less code can be shared across the different ephemeral account implementations. Additionally, it's not clear that we want the same dialog in both scenarios. On the demo account, the dialog makes some sense, but for a signed-in user, it makes a lot less sense. Especially if the point is to make sure someone doesn't wander off while signed in. It'd be nice to get this stuff sorted before commits start landing. thanks, will
dbus LGTM if few nits fixed: http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... File chrome/browser/chromeos/dbus/power_manager_client.h (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... chrome/browser/chromeos/dbus/power_manager_client.h:81: // Called when we go idle for threshold time a period is missing. please add it for consistency with others. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... chrome/browser/chromeos/dbus/power_manager_client.h:82: virtual void IdleNotify(int64 threshold) {} what's the unit of |threshold|? if it's seconds, threshould_secs would make it clear. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... chrome/browser/chromeos/dbus/power_manager_client.h:84: // Called when we go from idle to active ditto. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... chrome/browser/chromeos/dbus/power_manager_client.h:133: virtual void RequestIdleNotification(int64 threshold) = 0; ditto. please clarify the unit of |threshold| and add some suffix accordingly.
Instead of submiting this at once, you might want to create a smaller patch for 'dbus' changes. dbus part can be done separately. On 2012/01/26 23:55:44, satorux1 wrote: > dbus LGTM if few nits fixed: > > http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... > File chrome/browser/chromeos/dbus/power_manager_client.h (right): > > http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... > chrome/browser/chromeos/dbus/power_manager_client.h:81: // Called when we go > idle for threshold time > a period is missing. please add it for consistency with others. > > http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... > chrome/browser/chromeos/dbus/power_manager_client.h:82: virtual void > IdleNotify(int64 threshold) {} > what's the unit of |threshold|? if it's seconds, threshould_secs would make it > clear. > > http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... > chrome/browser/chromeos/dbus/power_manager_client.h:84: // Called when we go > from idle to active > ditto. > > http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... > chrome/browser/chromeos/dbus/power_manager_client.h:133: virtual void > RequestIdleNotification(int64 threshold) = 0; > ditto. please clarify the unit of |threshold| and add some suffix accordingly.
http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:44: const int kRetailModeLogoutDialogHeight = 120; document all these http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:46: } // namespace http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:50: void ShowRetailModeLogoutDialog() { why is this here and not in retail_mode_screen_locker.cc http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:97: : contents_(NULL), handler_(NULL) { too many spaces of indent http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:16: #include "content/public/browser/web_ui_message_handler.h" you don't need to include this one, you can forward declare http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:18: remove \n
http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/retail_mode_logout_dialog.css (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.css:17: background: -webkit-linear-gradient(left, #FFFFFF, #DADADA); s/#FFFFFF/white s/#DADADA/#dadada or rgb notation is preferable according to chromium web style guide. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.css:18: border-bottom: 1px solid #bbbbbb; s/#bbbbbb/#bbb http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.css:19: display: -webkit-box; Why do you use webkit-box here and below? http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.css:30: font-size: 13px; This class seems to be applied to all text on the page, move this to body. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/retail_mode_logout_dialog.html (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.html:16: <body i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> Assuming you want to override these values in CSS you should remove them from here. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.html:19: <div id="title" class="text" i18n-content="title"></div> This inner div seems unnecessary. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.html:22: <div id="warning" class="text"></div> Here too. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/retail_mode_logout_dialog.js (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.js:10: function decrementTimer(time) { Document me, remove time argument. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.js:30: setTimeout(decrementTimer, 1000); Use setInterval and remove it when time reaches 0. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:38: using content::WebUIMessageHandler; Avoid use of using. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:22: class RetailModeLogoutDialog : private HtmlDialogUIDelegate { Class comment. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:48: RetailModeLogoutDialogHandler* handler_; Comments, including on ownership of pointer. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:53: class RetailModeLogoutDialogUI : public HtmlDialogUI { Class comment.
Review comments incorporated. Will/Chris: This is just a first iteration, all of this code is behind a flag. I'll be incorporating other aspects of the design (including tests and enterprise policy lookup) in further iterations before I remove the flag. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... File chrome/browser/chromeos/dbus/power_manager_client.h (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... chrome/browser/chromeos/dbus/power_manager_client.h:81: // Called when we go idle for threshold time On 2012/01/26 23:55:44, satorux1 wrote: > a period is missing. please add it for consistency with others. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... chrome/browser/chromeos/dbus/power_manager_client.h:82: virtual void IdleNotify(int64 threshold) {} On 2012/01/26 23:55:44, satorux1 wrote: > what's the unit of |threshold|? if it's seconds, threshould_secs would make it > clear. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... chrome/browser/chromeos/dbus/power_manager_client.h:84: // Called when we go from idle to active On 2012/01/26 23:55:44, satorux1 wrote: > ditto. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbu... chrome/browser/chromeos/dbus/power_manager_client.h:133: virtual void RequestIdleNotification(int64 threshold) = 0; On 2012/01/26 23:55:44, satorux1 wrote: > ditto. please clarify the unit of |threshold| and add some suffix accordingly. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/retail_mode_logout_dialog.css (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.css:17: background: -webkit-linear-gradient(left, #FFFFFF, #DADADA); On 2012/01/27 07:48:04, flackr wrote: > s/#FFFFFF/white > s/#DADADA/#dadada or rgb notation is preferable according to chromium web style > guide. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.css:18: border-bottom: 1px solid #bbbbbb; On 2012/01/27 07:48:04, flackr wrote: > s/#bbbbbb/#bbb Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.css:19: display: -webkit-box; On 2012/01/27 07:48:04, flackr wrote: > Why do you use webkit-box here and below? Not needed, just the regular div works fine; removed. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.css:30: font-size: 13px; On 2012/01/27 07:48:04, flackr wrote: > This class seems to be applied to all text on the page, move this to body. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/retail_mode_logout_dialog.html (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.html:16: <body i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> On 2012/01/27 07:48:04, flackr wrote: > Assuming you want to override these values in CSS you should remove them from > here. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.html:19: <div id="title" class="text" i18n-content="title"></div> On 2012/01/27 07:48:04, flackr wrote: > This inner div seems unnecessary. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.html:22: <div id="warning" class="text"></div> On 2012/01/27 07:48:04, flackr wrote: > Here too. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/retail_mode_logout_dialog.js (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.js:10: function decrementTimer(time) { On 2012/01/27 07:48:04, flackr wrote: > Document me, remove time argument. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/ch... chrome/browser/resources/chromeos/retail_mode_logout_dialog.js:30: setTimeout(decrementTimer, 1000); On 2012/01/27 07:48:04, flackr wrote: > Use setInterval and remove it when time reaches 0. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:38: using content::WebUIMessageHandler; On 2012/01/27 07:48:04, flackr wrote: > Avoid use of using. This is being used 216 times in just the chrome/browser/ui directory structure, so I assumed this was an accept pattern. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:44: const int kRetailModeLogoutDialogHeight = 120; On 2012/01/27 06:54:46, Evan Stade wrote: > document all these Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:46: } On 2012/01/27 06:54:46, Evan Stade wrote: > // namespace Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:50: void ShowRetailModeLogoutDialog() { On 2012/01/27 06:54:46, Evan Stade wrote: > why is this here and not in retail_mode_screen_locker.cc Seemed like a better fit since all the UI display code is here. Moved anyhow, it fits with that code too since the declarations are there. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc:97: : contents_(NULL), handler_(NULL) { On 2012/01/27 06:54:46, Evan Stade wrote: > too many spaces of indent Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h (right): http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:16: #include "content/public/browser/web_ui_message_handler.h" On 2012/01/27 06:54:46, Evan Stade wrote: > you don't need to include this one, you can forward declare Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:18: On 2012/01/27 06:54:46, Evan Stade wrote: > remove \n Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:22: class RetailModeLogoutDialog : private HtmlDialogUIDelegate { On 2012/01/27 07:48:04, flackr wrote: > Class comment. Done. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:48: RetailModeLogoutDialogHandler* handler_; On 2012/01/27 07:48:04, flackr wrote: > Comments, including on ownership of pointer. This is a bit self evident. I haven't seen people documenting at this level in almost any of the current code for similar dialogs. http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:53: class RetailModeLogoutDialogUI : public HtmlDialogUI { On 2012/01/27 07:48:04, flackr wrote: > Class comment. Ditto.
On Tue, Feb 7, 2012 at 6:52 PM, <rkc@chromium.org> wrote: > Review comments incorporated. > > Will/Chris: This is just a first iteration, all of this code is behind a > flag. > I'll be incorporating other aspects of the design (including tests and > enterprise policy lookup) in further iterations before I remove the flag. > > Tests aren't something to be added later :-/ Mal, Linus, etc have said repeatedly that it's up to code reviewers to ensure that our code has test coverage, and not let changes go in without attendant tests. > > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/chromeos/dbus/power_**manager_client.h<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbus/power_manager_client.h> > File chrome/browser/chromeos/dbus/**power_manager_client.h (right): > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/chromeos/dbus/power_**manager_client.h#newcode81<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbus/power_manager_client.h#newcode81> > chrome/browser/chromeos/dbus/**power_manager_client.h:81: // Called when > we go idle for threshold time > On 2012/01/26 23:55:44, satorux1 wrote: > >> a period is missing. please add it for consistency with others. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/chromeos/dbus/power_**manager_client.h#newcode82<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbus/power_manager_client.h#newcode82> > chrome/browser/chromeos/dbus/**power_manager_client.h:82: virtual void > IdleNotify(int64 threshold) {} > On 2012/01/26 23:55:44, satorux1 wrote: > >> what's the unit of |threshold|? if it's seconds, threshould_secs would >> > make it > >> clear. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/chromeos/dbus/power_**manager_client.h#newcode84<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbus/power_manager_client.h#newcode84> > chrome/browser/chromeos/dbus/**power_manager_client.h:84: // Called when > we go from idle to active > On 2012/01/26 23:55:44, satorux1 wrote: > >> ditto. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/chromeos/dbus/power_**manager_client.h#newcode133<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/chromeos/dbus/power_manager_client.h#newcode133> > chrome/browser/chromeos/dbus/**power_manager_client.h:133: virtual void > RequestIdleNotification(int64 threshold) = 0; > On 2012/01/26 23:55:44, satorux1 wrote: > >> ditto. please clarify the unit of |threshold| and add some suffix >> > accordingly. > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.css<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.css> > File chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.css > (right): > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.css#**newcode17<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.css#newcode17> > chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.css:17: > background: -webkit-linear-gradient(left, #FFFFFF, #DADADA); > On 2012/01/27 07:48:04, flackr wrote: > >> s/#FFFFFF/white >> s/#DADADA/#dadada or rgb notation is preferable according to chromium >> > web style > >> guide. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.css#**newcode18<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.css#newcode18> > chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.css:18: > border-bottom: 1px solid #bbbbbb; > On 2012/01/27 07:48:04, flackr wrote: > >> s/#bbbbbb/#bbb >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.css#**newcode19<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.css#newcode19> > chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.css:19: > display: -webkit-box; > On 2012/01/27 07:48:04, flackr wrote: > >> Why do you use webkit-box here and below? >> > Not needed, just the regular div works fine; removed. > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.css#**newcode30<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.css#newcode30> > chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.css:30: > font-size: 13px; > On 2012/01/27 07:48:04, flackr wrote: > >> This class seems to be applied to all text on the page, move this to >> > body. > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.html<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.html> > File chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.html > (right): > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.**html#newcode16<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.html#newcode16> > chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.html:16: > <body > i18n-values=".style.**fontFamily:fontfamily;.style.**fontSize:fontsize"> > On 2012/01/27 07:48:04, flackr wrote: > >> Assuming you want to override these values in CSS you should remove >> > them from > >> here. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.**html#newcode19<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.html#newcode19> > chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.html:19: > <div id="title" class="text" i18n-content="title"></div> > On 2012/01/27 07:48:04, flackr wrote: > >> This inner div seems unnecessary. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.**html#newcode22<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.html#newcode22> > chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.html:22: > <div id="warning" class="text"></div> > On 2012/01/27 07:48:04, flackr wrote: > >> Here too. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.js<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.js> > File chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.js > (right): > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.js#**newcode10<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.js#newcode10> > chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.js:10: > function decrementTimer(time) { > On 2012/01/27 07:48:04, flackr wrote: > >> Document me, remove time argument. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/resources/chromeos/**retail_mode_logout_dialog.js#**newcode30<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/resources/chromeos/retail_mode_logout_dialog.js#newcode30> > chrome/browser/resources/**chromeos/retail_mode_logout_**dialog.js:30: > setTimeout(decrementTimer, 1000); > On 2012/01/27 07:48:04, flackr wrote: > >> Use setInterval and remove it when time reaches 0. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.cc<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc> > File chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.cc > (right): > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.cc#**newcode38<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc#newcode38> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.cc:38: > using > content::WebUIMessageHandler; > On 2012/01/27 07:48:04, flackr wrote: > >> Avoid use of using. >> > This is being used 216 times in just the chrome/browser/ui directory > structure, so I assumed this was an accept pattern. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.cc#**newcode44<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc#newcode44> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.cc:44: > const > int kRetailModeLogoutDialogHeight = 120; > On 2012/01/27 06:54:46, Evan Stade wrote: > >> document all these >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.cc#**newcode46<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc#newcode46> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.cc:46: } > On 2012/01/27 06:54:46, Evan Stade wrote: > >> // namespace >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.cc#**newcode50<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc#newcode50> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.cc:50: void > ShowRetailModeLogoutDialog() { > On 2012/01/27 06:54:46, Evan Stade wrote: > >> why is this here and not in retail_mode_screen_locker.cc >> > > Seemed like a better fit since all the UI display code is here. Moved > anyhow, it fits with that code too since the declarations are there. > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.cc#**newcode97<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.cc#newcode97> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.cc:97: : > contents_(NULL), handler_(NULL) { > On 2012/01/27 06:54:46, Evan Stade wrote: > >> too many spaces of indent >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.h<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h> > File chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.h > (right): > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.h#**newcode16<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h#newcode16> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.h:16: > #include "content/public/browser/web_**ui_message_handler.h" > On 2012/01/27 06:54:46, Evan Stade wrote: > >> you don't need to include this one, you can forward declare >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.h#**newcode18<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h#newcode18> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.h:18: > On 2012/01/27 06:54:46, Evan Stade wrote: > >> remove \n >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.h#**newcode22<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h#newcode22> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.h:22: class > RetailModeLogoutDialog : private HtmlDialogUIDelegate { > On 2012/01/27 07:48:04, flackr wrote: > >> Class comment. >> > > Done. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.h#**newcode48<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h#newcode48> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.h:48: > RetailModeLogoutDialogHandler* handler_; > On 2012/01/27 07:48:04, flackr wrote: > >> Comments, including on ownership of pointer. >> > > This is a bit self evident. I haven't seen people documenting at this > level in almost any of the current code for similar dialogs. > > http://codereview.chromium.**org/9265026/diff/12010/chrome/** > browser/ui/webui/chromeos/**retail_mode_logout_dialog.h#**newcode53<http://codereview.chromium.org/9265026/diff/12010/chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h#newcode53> > chrome/browser/ui/webui/**chromeos/retail_mode_logout_**dialog.h:53: class > RetailModeLogoutDialogUI : public HtmlDialogUI { > On 2012/01/27 07:48:04, flackr wrote: > >> Class comment. >> > > Ditto. > > http://codereview.chromium.**org/9265026/<http://codereview.chromium.org/9265... >
On 2012/02/08 03:45:13, Chris Masone wrote: > On Tue, Feb 7, 2012 at 6:52 PM, <mailto:rkc@chromium.org> wrote: > > > Review comments incorporated. > > > > Will/Chris: This is just a first iteration, all of this code is behind a > > flag. > > I'll be incorporating other aspects of the design (including tests and > > enterprise policy lookup) in further iterations before I remove the flag. > > > > > Tests aren't something to be added later :-/ Mal, Linus, etc have said > repeatedly that it's up to code reviewers to ensure that our code has test > coverage, and not let changes go in without attendant tests. > > It's experimental code behind a flag; already said I will add tests. I won't be adding more code to this CL, it's big enough already. I am pretty certain mal/linus/et al didn't mean for every feature to be checked in complete with tests in one CL. I am not sure why you're insisting that I add tests to "this" CL and not a follow-up CL?
rkc@, please create a separate patch for chrome/browser/chromeos/dbus. then, this patch will be a bit smaller. I guess other parts can also be split but I only looked at dbus stuff.
On Tue, Feb 7, 2012 at 8:07 PM, <rkc@chromium.org> wrote: > On 2012/02/08 03:45:13, Chris Masone wrote: > > On Tue, Feb 7, 2012 at 6:52 PM, <mailto:rkc@chromium.org> wrote: >> > > > Review comments incorporated. >> > >> > Will/Chris: This is just a first iteration, all of this code is behind a >> > flag. >> > I'll be incorporating other aspects of the design (including tests and >> > enterprise policy lookup) in further iterations before I remove the >> flag. >> > >> > >> Tests aren't something to be added later :-/ Mal, Linus, etc have said >> repeatedly that it's up to code reviewers to ensure that our code has test >> coverage, and not let changes go in without attendant tests. >> > > > > It's experimental code behind a flag; already said I will add tests. > I won't be adding more code to this CL, it's big enough already. > > I am pretty certain mal/linus/et al didn't mean for every feature to be > checked > in complete with tests in one CL. > > No, I'm pretty sure the intent was that features be broken up into CLs small enough that you can land each piece along with its own unit tests/browser tests. Perhaps you can break this CL up? Satorux already suggested one piece you could break off. > I am not sure why you're insisting that I add tests to "this" CL and not a > follow-up CL? > > > http://codereview.chromium.**org/9265026/<http://codereview.chromium.org/9265... >
https://chromiumcodereview.appspot.com/9265026/diff/12010/chrome/browser/ui/w... File chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h (right): https://chromiumcodereview.appspot.com/9265026/diff/12010/chrome/browser/ui/w... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:48: RetailModeLogoutDialogHandler* handler_; On 2012/02/08 02:52:19, Rahul Chaturvedi wrote: > On 2012/01/27 07:48:04, flackr wrote: > > Comments, including on ownership of pointer. > > This is a bit self evident. I haven't seen people documenting at this level in > almost any of the current code for similar dialogs. It is self evident what the data member is, it may not be self evident if the pointer is owned by this class. If it is, you should use scoped_ptr which would make that self evident, or document that it is a weak pointer if the pointer is owned by another class. https://chromiumcodereview.appspot.com/9265026/diff/12010/chrome/browser/ui/w... chrome/browser/ui/webui/chromeos/retail_mode_logout_dialog.h:53: class RetailModeLogoutDialogUI : public HtmlDialogUI { On 2012/02/08 02:52:19, Rahul Chaturvedi wrote: > On 2012/01/27 07:48:04, flackr wrote: > > Class comment. > > Ditto. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments https://chromiumcodereview.appspot.com/9265026/diff/25001/chrome/browser/reso... File chrome/browser/resources/chromeos/retail_mode_logout_dialog.js (right): https://chromiumcodereview.appspot.com/9265026/diff/25001/chrome/browser/reso... chrome/browser/resources/chromeos/retail_mode_logout_dialog.js:9: var countdownTimer = -1; nit: leave value undefined. |
