Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
155291:
Adding a message box to confirm if the user really wants to delete all stored... (Closed)
|
Created:
11 years, 5 months ago by tfarina (gmail-do not use) Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, Ben Goodger (Google), glen Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdding a message box to confirm if the user really wants to delete all stored passwords.
BUG=11401
TEST=Click on Remove all
Patch Set 1 #Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 10
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 9
Patch Set 7 : '' #
Total comments: 3
Patch Set 8 : '' #
Total comments: 8
Patch Set 9 : '' #
Total comments: 1
Messages
Total messages: 30 (0 generated)
Tim, You could review this to me? Thanks!
The message text should be translatable, it's not good to have it hard coded.
I know. I sent a message to Tim asking what would be the best place to put these strings. Maybe in chromium_strings.grd or generated_resources.grd? On Thu, Jul 9, 2009 at 12:18 PM, <blackboxxx@gmail.com> wrote: > The message text should be translatable, it's not good to have it hard > coded. > > http://codereview.chromium.org/155291 >
On 2009/07/09 15:21:24, tfarina wrote: > I know. I sent a message to Tim asking what would be the best place to > put these strings. > Maybe in chromium_strings.grd or generated_resources.grd? > > On Thu, Jul 9, 2009 at 12:18 PM, <blackboxxx@gmail.com> wrote: > > The message text should be translatable, it's not good to have it hard > > coded. > > > > http://codereview.chromium.org/155291 > > What you think blackboxxx? Do you know where is the best to put these strings? If you know, please send me an email for I do this.
On 2009/07/09 15:45:11, tfarina wrote: > What you think blackboxxx? Do you know where is the best to put these strings? > If you know, please send me an email for I do this. I'm not very familiar with Chromium source, but generated_resources.grd seems more appropriate.
Yes, thanks blackboxxx, I had not read the comments in top of the files. So the best place is really the generated_resources.grd. On Thu, Jul 9, 2009 at 1:28 PM, <blackboxxx@gmail.com> wrote: > On 2009/07/09 15:45:11, tfarina wrote: >> >> What you think blackboxxx? Do you know where is the best to put these > > strings? >> >> If you know, please send me an email for I do this. > > I'm not very familiar with Chromium source, but generated_resources.grd > seems more appropriate. > > http://codereview.chromium.org/155291 >
On 2009/07/09 16:32:32, tfarina wrote: > Yes, thanks blackboxxx, I had not read the comments in top of the > files. So the best place is really the generated_resources.grd. > > On Thu, Jul 9, 2009 at 1:28 PM, <blackboxxx@gmail.com> wrote: > > On 2009/07/09 15:45:11, tfarina wrote: > >> > >> What you think blackboxxx? Do you know where is the best to put these > > > > strings? > >> > >> If you know, please send me an email for I do this. > > > > I'm not very familiar with Chromium source, but generated_resources.grd > > seems more appropriate. > > > > http://codereview.chromium.org/155291 > > Please disconsider the part of IDS_BOOKMARK_TABLE_DATE.
Thanks for the patch!
Thanks very much for working on this! I definitely agree some sort of "are you sure?" mechanism is needed. I've cc'ed Glen just to make sure this message box is the best way to go right now. http://codereview.chromium.org/155291/diff/6/7 File chrome/browser/views/options/passwords_page_view.cc (right): http://codereview.chromium.org/155291/diff/6/7#newcode226 Line 226: const UINT mb_flags = MB_YESNO | MB_ICONQUESTION | MB_SETFOREGROUND; This is windows specific, so it should at least be wrapped in a proper ifdef (see use of win_util::MessageBox in extension_error_reporter.cc for an example of this and a mac counterpart). http://codereview.chromium.org/155291/diff/6/7#newcode230 Line 230: IDS_PASSWORDS_PAGE_VIEW_CAPTION_DELETE_ALL_PASSWORDS), mb_flags)) { this should be indented 4 spaces.
Feature is fine to do, however: http://codereview.chromium.org/155291/diff/6/8 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/155291/diff/6/8#newcode2165 Line 2165: <message name="IDS_PASSWORDS_PAGE_VIEW_TEXT_DELETE_ALL_PASSWORDS" desc ="Passwords page view's message box text"> Please make the desc field more descriptive - the translators rely on it and imagine they often can't see the dialogs you're talking about when they're doing it. http://codereview.chromium.org/155291/diff/6/7 File chrome/browser/views/options/passwords_page_view.cc (right): http://codereview.chromium.org/155291/diff/6/7#newcode227 Line 227: if (IDYES == win_util::MessageBox(NULL, I don't think we want to use a Windows messagebox for this - I believe they're not visually styled like Chrome dialogs.
On Thu, Jul 9, 2009 at 12:30 PM, <glen@chromium.org> wrote: > http://codereview.chromium.org/155291/diff/6/7#newcode227 > Line 227: if (IDYES == win_util::MessageBox(NULL, > I don't think we want to use a Windows messagebox for this - I believe > they're not visually styled like Chrome dialogs. Plus they don't solve the problem for other OSes. Don't we have a utility for putting up a Chrome dialog, used by things like JS alert()s? PK http://codereview.chromium.org/155291 >
On 2009/07/09 19:32:47, pkasting wrote: > On Thu, Jul 9, 2009 at 12:30 PM, <glen@chromium.org> wrote: > > > http://codereview.chromium.org/155291/diff/6/7#newcode227 > > Line 227: if (IDYES == win_util::MessageBox(NULL, > > I don't think we want to use a Windows messagebox for this - I believe > > they're not visually styled like Chrome dialogs. > > > Plus they don't solve the problem for other OSes. > > Don't we have a utility for putting up a Chrome dialog, used by things like > JS alert()s? > > PK > > http://codereview.chromium.org/155291 > > > There is JavascriptMessageBoxDialog in jsmessage_box_dialog.h, which I was going to suggest but didn't because it seemed really "big" just for this. I looked around and saw some other uses of the win_util MessageBox by extensions and stuff, but I didn't realize it looks different.
On 2009/07/09 19:38:19, timsteele wrote: > On 2009/07/09 19:32:47, pkasting wrote: > > On Thu, Jul 9, 2009 at 12:30 PM, <glen@chromium.org> wrote: > > > > > http://codereview.chromium.org/155291/diff/6/7#newcode227 > > > Line 227: if (IDYES == win_util::MessageBox(NULL, > > > I don't think we want to use a Windows messagebox for this - I believe > > > they're not visually styled like Chrome dialogs. > > > > > > Plus they don't solve the problem for other OSes. > > > > Don't we have a utility for putting up a Chrome dialog, used by things like > > JS alert()s? > > > > PK > > > > http://codereview.chromium.org/155291 > > > > > > > There is JavascriptMessageBoxDialog in jsmessage_box_dialog.h, which I was going > to suggest but didn't because it seemed really "big" just for this. I looked > around and saw some other uses of the win_util MessageBox by extensions and > stuff, but I didn't realize it looks different. well, by "big" I mean using views::DialogDelegate seemed like a bit of overhead. But maybe it's the way to go.. I don't know what other options we've got lying around?
On 2009/07/09 19:39:44, timsteele wrote: > On 2009/07/09 19:38:19, timsteele wrote: > > On 2009/07/09 19:32:47, pkasting wrote: > > > On Thu, Jul 9, 2009 at 12:30 PM, <glen@chromium.org> wrote: > > > > > > > http://codereview.chromium.org/155291/diff/6/7#newcode227 > > > > Line 227: if (IDYES == win_util::MessageBox(NULL, > > > > I don't think we want to use a Windows messagebox for this - I believe > > > > they're not visually styled like Chrome dialogs. > > > > > > > > > Plus they don't solve the problem for other OSes. > > > > > > Don't we have a utility for putting up a Chrome dialog, used by things like > > > JS alert()s? > > > > > > PK > > > > > > http://codereview.chromium.org/155291 > > > > > > > > > > > There is JavascriptMessageBoxDialog in jsmessage_box_dialog.h, which I was > going > > to suggest but didn't because it seemed really "big" just for this. I looked > > around and saw some other uses of the win_util MessageBox by extensions and > > stuff, but I didn't realize it looks different. > > well, by "big" I mean using views::DialogDelegate seemed like a bit of overhead. > But maybe it's the way to go.. I don't know what other options we've got lying > around? I looked into a simple message dialog like chrome/browser/views/restart_message_box.h It is using DialogDelegate, I think you should do the same to create a yes no dialog or simply use jsmessage_box_dialog.h. The name of JSMessageBoxDialog may look a little strange for this purpose, we may want to refactor to something called ConfirmDialog.
On 2009/07/09 22:03:18, Alpha wrote: > On 2009/07/09 19:39:44, timsteele wrote: > > On 2009/07/09 19:38:19, timsteele wrote: > > > On 2009/07/09 19:32:47, pkasting wrote: > > > > On Thu, Jul 9, 2009 at 12:30 PM, <glen@chromium.org> wrote: > > > > > > > > > http://codereview.chromium.org/155291/diff/6/7#newcode227 > > > > > Line 227: if (IDYES == win_util::MessageBox(NULL, > > > > > I don't think we want to use a Windows messagebox for this - I believe > > > > > they're not visually styled like Chrome dialogs. > > > > > > > > > > > > Plus they don't solve the problem for other OSes. > > > > > > > > Don't we have a utility for putting up a Chrome dialog, used by things > like > > > > JS alert()s? > > > > > > > > PK > > > > > > > > http://codereview.chromium.org/155291 > > > > > > > > > > > > > > > There is JavascriptMessageBoxDialog in jsmessage_box_dialog.h, which I was > > going > > > to suggest but didn't because it seemed really "big" just for this. I > looked > > > around and saw some other uses of the win_util MessageBox by extensions and > > > stuff, but I didn't realize it looks different. > > > > well, by "big" I mean using views::DialogDelegate seemed like a bit of > overhead. > > But maybe it's the way to go.. I don't know what other options we've got > lying > > around? > > I looked into a simple message dialog like > chrome/browser/views/restart_message_box.h > It is using DialogDelegate, I think you should do the same to create a yes no > dialog or simply use jsmessage_box_dialog.h. The name of JSMessageBoxDialog may > look a little strange for this purpose, we may want to refactor to something > called ConfirmDialog. Sorry guys, for not having answered before. Thanks for all answers and suggestions. Alpha I looked at the code of both files you suggested. In both cases I will need to do write another class based in one of these. The JSMessageBoxDialog has more options, and is more flexible. The other is more simple. As Tim and Peter suggested to use JSMessageBoxDialog, I'm more inclide to write another class called ConfirmDialog based in JSMessageBoxDialog. Someone preferer that I use the other classe(RestartMessageBox), or it's ok if I rewrite the JSMessageBoxDialog calling the new class as ConfirmDialog?
I think what I like best would be to have a DialogDelegate based ConfirmDialog type that would have an easy way to pop open, similar to the static win_util::MessageBox call that returns the result of the user decision. I'm not sure that it should be based atop JsMessageBoxHandler, which depends on AppModalDialog and showing the "suppress checkbox", neither of which we need here. On Thu, Jul 9, 2009 at 4:16 PM, <thiago.farina@gmail.com> wrote: > On 2009/07/09 22:03:18, Alpha wrote: > >> On 2009/07/09 19:39:44, timsteele wrote: >> > On 2009/07/09 19:38:19, timsteele wrote: >> > > On 2009/07/09 19:32:47, pkasting wrote: >> > > > On Thu, Jul 9, 2009 at 12:30 PM, <glen@chromium.org> wrote: >> > > > >> > > > > http://codereview.chromium.org/155291/diff/6/7#newcode227 >> > > > > Line 227: if (IDYES == win_util::MessageBox(NULL, >> > > > > I don't think we want to use a Windows messagebox for this - I >> > believe > >> > > > > they're not visually styled like Chrome dialogs. >> > > > >> > > > >> > > > Plus they don't solve the problem for other OSes. >> > > > >> > > > Don't we have a utility for putting up a Chrome dialog, used by >> > things > >> like >> > > > JS alert()s? >> > > > >> > > > PK >> > > > >> > > > http://codereview.chromium.org/155291 >> > > > > >> > > > >> > > >> > > There is JavascriptMessageBoxDialog in jsmessage_box_dialog.h, >> > which I was > >> > going >> > > to suggest but didn't because it seemed really "big" just for >> > this. I > >> looked >> > > around and saw some other uses of the win_util MessageBox by >> > extensions and > >> > > stuff, but I didn't realize it looks different. >> > >> > well, by "big" I mean using views::DialogDelegate seemed like a bit >> > of > >> overhead. >> > But maybe it's the way to go.. I don't know what other options >> > we've got > >> lying >> > around? >> > > I looked into a simple message dialog like >> chrome/browser/views/restart_message_box.h >> It is using DialogDelegate, I think you should do the same to create a >> > yes no > >> dialog or simply use jsmessage_box_dialog.h. The name of >> > JSMessageBoxDialog may > >> look a little strange for this purpose, we may want to refactor to >> > something > >> called ConfirmDialog. >> > > Sorry guys, for not having answered before. Thanks for all answers and > suggestions. > > Alpha I looked at the code of both files you suggested. In both cases I > will need to do write another class based in one of these. The > JSMessageBoxDialog has more options, and is more flexible. The other is > more simple. > As Tim and Peter suggested to use JSMessageBoxDialog, I'm more inclide > to write another class called ConfirmDialog based in JSMessageBoxDialog. > > Someone preferer that I use the other classe(RestartMessageBox), or it's > ok if I rewrite the JSMessageBoxDialog calling the new class as > > ConfirmDialog? > > > http://codereview.chromium.org/155291 >
This early patchs are provisional, I will be busy today, I will be back later to night. Many things are only to see what I need to do. Some strings that I use, needs to be changed. Maybe I will need to use IPC::Message to get the results of the dialog like JsMessageBoxHandler. So I still have much work to do.
Some style nits and the biggest issue is that ConfirmMessageBoxDialog doesn't return a result.. It feels like it is not completed yet. http://codereview.chromium.org/155291/diff/19/22 File chrome/browser/views/confirm_message_box_dialog.cc (right): http://codereview.chromium.org/155291/diff/19/22#newcode13 Line 13: ConfirmMessageBoxDialog::ConfirmMessageBoxDialog(HWND parent_hwnd, nit: put the parameter definition to next line. http://codereview.chromium.org/155291/diff/19/22#newcode27 Line 27: MessageBoxFlags::kFlagHasMessage | MessageBoxFlags::kFlagHasOKButton, nit: indented by 4 spaces. http://codereview.chromium.org/155291/diff/19/22#newcode28 Line 28: message_text_, window_title_); nit: dito. http://codereview.chromium.org/155291/diff/19/22#newcode43 Line 43: return l10n_util::GetString(IDS_BEFOREUNLOAD_MESSAGEBOX_OK_BUTTON_LABEL); you can define IDS_CONFIRM_MESSAGEBOX_OK_BUTTON_LABEL http://codereview.chromium.org/155291/diff/19/23 File chrome/browser/views/confirm_message_box_dialog.h (right): http://codereview.chromium.org/155291/diff/19/23#newcode1 Line 1: #ifndef CHROME_BROWSER_VIEWS_CONFIRM_MESSAGE_BOX_DIALOG_H_ nit: you need to put licence header. http://codereview.chromium.org/155291/diff/19/23#newcode13 Line 13: ConfirmMessageBoxDialog(HWND parent_hwnd, nit: should be indented by 2 spaces, you need to fix indentation in this file. http://codereview.chromium.org/155291/diff/19/23#newcode14 Line 14: const std::wstring& message_text, nit: indentation is incorrect. http://codereview.chromium.org/155291/diff/19/20 File chrome/browser/views/options/passwords_page_view.cc (right): http://codereview.chromium.org/155291/diff/19/20#newcode230 Line 230: GetWindow()->GetNativeWindow(), nit: indented by 4 spaces http://codereview.chromium.org/155291/diff/19/20#newcode237 Line 237: if (IDYES == win_util::MessageBox(NULL, hum.. so you have the confirm dialog, but it doesn't give you any results? It seems strange to me you pressed ok and there's another message box that asks you if you want to proceed? May you should make confirm dialog to give you a true/false result. Again this is using the default windows dialog which doesn't have the chrome look and feel. http://codereview.chromium.org/155291/diff/19/20#newcode240 Line 240: IDS_PASSWORDS_PAGE_VIEW_CAPTION_DELETE_ALL_PASSWORDS), mb_flags)) { nit: indentation by 4 spaces.
On 2009/07/11 01:05:50, Alpha wrote: > Some style nits and the biggest issue is that ConfirmMessageBoxDialog doesn't > return a result.. It feels like it is not completed yet. > > http://codereview.chromium.org/155291/diff/19/22 > File chrome/browser/views/confirm_message_box_dialog.cc (right): > > http://codereview.chromium.org/155291/diff/19/22#newcode13 > Line 13: ConfirmMessageBoxDialog::ConfirmMessageBoxDialog(HWND parent_hwnd, > nit: put the parameter definition to next line. > > http://codereview.chromium.org/155291/diff/19/22#newcode27 > Line 27: MessageBoxFlags::kFlagHasMessage | MessageBoxFlags::kFlagHasOKButton, > nit: indented by 4 spaces. > > http://codereview.chromium.org/155291/diff/19/22#newcode28 > Line 28: message_text_, window_title_); > nit: dito. > > http://codereview.chromium.org/155291/diff/19/22#newcode43 > Line 43: return > l10n_util::GetString(IDS_BEFOREUNLOAD_MESSAGEBOX_OK_BUTTON_LABEL); > you can define IDS_CONFIRM_MESSAGEBOX_OK_BUTTON_LABEL > > http://codereview.chromium.org/155291/diff/19/23 > File chrome/browser/views/confirm_message_box_dialog.h (right): > > http://codereview.chromium.org/155291/diff/19/23#newcode1 > Line 1: #ifndef CHROME_BROWSER_VIEWS_CONFIRM_MESSAGE_BOX_DIALOG_H_ > nit: you need to put licence header. > > http://codereview.chromium.org/155291/diff/19/23#newcode13 > Line 13: ConfirmMessageBoxDialog(HWND parent_hwnd, > nit: should be indented by 2 spaces, you need to fix indentation in this file. > > http://codereview.chromium.org/155291/diff/19/23#newcode14 > Line 14: const std::wstring& message_text, > nit: indentation is incorrect. > > http://codereview.chromium.org/155291/diff/19/20 > File chrome/browser/views/options/passwords_page_view.cc (right): > > http://codereview.chromium.org/155291/diff/19/20#newcode230 > Line 230: GetWindow()->GetNativeWindow(), > nit: indented by 4 spaces > > http://codereview.chromium.org/155291/diff/19/20#newcode237 > Line 237: if (IDYES == win_util::MessageBox(NULL, > hum.. so you have the confirm dialog, but it doesn't give you any results? It > seems strange to me you pressed ok and there's another message box that asks you > if you want to proceed? > > May you should make confirm dialog to give you a true/false result. Again this > is using the default windows dialog which doesn't have the chrome look and feel. > > http://codereview.chromium.org/155291/diff/19/20#newcode240 > Line 240: IDS_PASSWORDS_PAGE_VIEW_CAPTION_DELETE_ALL_PASSWORDS), mb_flags)) { > nit: indentation by 4 spaces. This isn't intended to be the final patch. This isn't working yet, because dialog->Show() (file:passwords_page_view.cc; line:231) isn't blocking yet.
I made some changes on ConfirmMessageBoxDialog. Now you call the static method (RunConfirmMessageBoxDialog), and it returns the result of the dialog. And now this method blocks while the dialog is showing.
I changed the parameter from HWND to gfx::NativeWindow to make it compatible with all platforms.
Looking nice! http://codereview.chromium.org/155291/diff/1025/32 File chrome/browser/views/confirm_message_box_dialog.cc (right): http://codereview.chromium.org/155291/diff/1025/32#newcode20 Line 20: message_text, window_title); nit- indent 4 spaces http://codereview.chromium.org/155291/diff/1025/32#newcode34 Line 34: message_text_.c_str(), window_title_); Don't need c_str(). Also indent is off. http://codereview.chromium.org/155291/diff/1025/33 File chrome/browser/views/confirm_message_box_dialog.h (right): http://codereview.chromium.org/155291/diff/1025/33#newcode19 Line 19: // The method blocks while the dialog is showing. Add a comment about the return value. http://codereview.chromium.org/155291/diff/1025/33#newcode20 Line 20: static bool RunConfirmMessageBoxDialog(HWND parent_hwnd, use gfx::NativeWindow instead of HWND. http://codereview.chromium.org/155291/diff/1025/33#newcode21 Line 21: const std::wstring& message_text, Maybe just "Run" is sufficient, since it is addressed as ConfirmMessageBoxDialog::Run? http://codereview.chromium.org/155291/diff/1025/33#newcode47 Line 47: HWND parent_hwnd_; Use gfx::NativeWindow rather than HWND. http://codereview.chromium.org/155291/diff/1025/33#newcode48 Line 48: std::wstring message_text_; please add some comments for this and the members below. http://codereview.chromium.org/155291/diff/1025/33#newcode59 Line 59: DISALLOW_EVIL_CONSTRUCTORS(ConfirmMessageBoxDialog); nit - DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/155291/diff/1025/30 File chrome/browser/views/options/passwords_page_view.cc (right): http://codereview.chromium.org/155291/diff/1025/30#newcode218 Line 218: GetWindow()->GetNativeWindow(), nit- indent 4 spaces http://codereview.chromium.org/155291/diff/1031/1033 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/155291/diff/1031/1033#newcode2165 Line 2165: <message name="IDS_PASSWORDS_PAGE_VIEW_TEXT_DELETE_ALL_PASSWORDS" desc ="Passwords page view's confirmation message before delete all stored passwords"> "before deleting all stored passwords" http://codereview.chromium.org/155291/diff/1031/1033#newcode2168 Line 2168: <message name="IDS_PASSWORDS_PAGE_VIEW_CAPTION_DELETE_ALL_PASSWORDS" desc="Passwords page view's confirmation message box window title"> nit- say "remove all confirmation" rather than just "confirmation". http://codereview.chromium.org/155291/diff/1031/1033#newcode2169 Line 2169: Delete all passwords the button is labelled 'Remove' rather than delete, we should probably keep all three strings consistent.
LGTM with just one nit, please check with tim commit. :)
LGTM with nits http://codereview.chromium.org/155291/diff/40/43 File chrome/browser/views/confirm_message_box_dialog.cc (right): http://codereview.chromium.org/155291/diff/40/43#newcode28 Line 28: : message_text_(message_text), two less spaces indent http://codereview.chromium.org/155291/diff/40/43#newcode33 Line 33: message_text_, window_title_); one more space http://codereview.chromium.org/155291/diff/40/41 File chrome/browser/views/options/passwords_page_view.cc (right): http://codereview.chromium.org/155291/diff/40/41#newcode218 Line 218: GetWindow()->GetNativeWindow(), should be 4 space indent; fyi, you can check out http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... "Wrapped parameters have 4 space indent"
http://codereview.chromium.org/155291/diff/40/43 File chrome/browser/views/confirm_message_box_dialog.cc (right): http://codereview.chromium.org/155291/diff/40/43#newcode28 Line 28: : message_text_(message_text), On 2009/07/14 17:31:06, timsteele wrote: > two less spaces indent Done. http://codereview.chromium.org/155291/diff/40/43#newcode33 Line 33: message_text_, window_title_); On 2009/07/14 17:31:06, timsteele wrote: > one more space Done. http://codereview.chromium.org/155291/diff/40/41 File chrome/browser/views/options/passwords_page_view.cc (right): http://codereview.chromium.org/155291/diff/40/41#newcode218 Line 218: GetWindow()->GetNativeWindow(), On 2009/07/14 17:31:06, timsteele wrote: > should be 4 space indent; > fyi, you can check out > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > "Wrapped parameters have 4 space indent" bool accepted = ConfirmMessageBoxDialog::Run( GetWindow()->GetNativeWindow(), l10n_util::GetString(IDS_PASSWORDS_PAGE_VIEW_TEXT_DELETE_ALL_PASSWORDS), l10n_util::GetString( IDS_PASSWORDS_PAGE_VIEW_CAPTION_DELETE_ALL_PASSWORDS)); You want in this shape?
http://codereview.chromium.org/155291/diff/40/41 File chrome/browser/views/options/passwords_page_view.cc (right): http://codereview.chromium.org/155291/diff/40/41#newcode218 Line 218: GetWindow()->GetNativeWindow(), nit: indent by 4 spcaes http://codereview.chromium.org/155291/diff/40/41#newcode218 Line 218: GetWindow()->GetNativeWindow(), On 2009/07/14 17:47:14, tfarina wrote: > On 2009/07/14 17:31:06, timsteele wrote: > > should be 4 space indent; > > fyi, you can check out > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > > > "Wrapped parameters have 4 space indent" > > bool accepted = ConfirmMessageBoxDialog::Run( > GetWindow()->GetNativeWindow(), > l10n_util::GetString(IDS_PASSWORDS_PAGE_VIEW_TEXT_DELETE_ALL_PASSWORDS), > l10n_util::GetString( > IDS_PASSWORDS_PAGE_VIEW_CAPTION_DELETE_ALL_PASSWORDS)); > > You want in this shape? bool accepted = ConfirmMessageBoxDialog::Run( GetWindow()->GetNativeWindow(), l10n_util::GetString(IDS_PASSWORDS_PAGE_VIEW_TEXT_DELETE_ALL_PASSWORDS), 10n_util::GetString( IDS_PASSWORDS_PAGE_VIEW_CAPTION_DELETE_ALL_PASSWORDS));
http://codereview.chromium.org/155291/diff/51/1042 File chrome/browser/views/options/passwords_page_view.cc (right): http://codereview.chromium.org/155291/diff/51/1042#newcode218 Line 218: GetWindow()->GetNativeWindow(), I guess I should have pointed out that the 4 spaces are from the start of the previous line. So bool accepted = ConfirmMessageBoxDialog::Run( GetWindow()->GetNativeWindow(), |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
