Add JavaScript dialogs functionality to content_shell on Windows

Add JavaScript dialogs functionality to content_shell on Windows

BUG=120155
TEST=JavaScript dialogs should work in content_shell on Windows


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129904

[Avi (use Gerrit), 2012-03-29 15:24:52]

Wow, that's fast. I'm completely unqualified to review this; passing to John
because he's a Windows type. The only question that I have is the .rc file in
the target; it looks like we should already be using it, but right now it's only
in content_browsertests?

[Cem Kocagil, 2012-03-29 15:44:42]

Yes, that shell.rc bugged me as well.

It seems like someone coded a menu bar, but it didn't appear until I added
shell.rc. This menu bar has an item that creates new windows, which also
fixes/helps with bug 120153 on Windows.

[jam, 2012-03-29 15:50:30]

thanks for helping :) some nits

http://codereview.chromium.org/9918013/diff/4002/content/shell/shell_javascri...
File content/shell/shell_javascript_dialog_win.cc (right):

http://codereview.chromium.org/9918013/diff/4002/content/shell/shell_javascri...
content/shell/shell_javascript_dialog_win.cc:15: INT_PTR CALLBACK
ShellJavaScriptDialog::DialogProc(HWND hDlg, UINT msg,
nit: per style guide, this should e HWND dialog, UINT message, WPARAM wparam,
LPARAM lparam

http://codereview.chromium.org/9918013/diff/4002/content/shell/shell_javascri...
content/shell/shell_javascript_dialog_win.cc:20: SetWindowLongPtr(hDlg,
DWL_USER, (LONG_PTR)lParam);
nit: here and below, we don't use c style casts per style guide. change to
static_cast

http://codereview.chromium.org/9918013/diff/4002/content/shell/shell_javascri...
content/shell/shell_javascript_dialog_win.cc:25: SetDlgItemText(hDlg,
IDC_PROMPTEDIT,
nit: this tabbing isn't according to style guide, move the second line to line
up with the first line's parameters

http://codereview.chromium.org/9918013/diff/4002/content/shell/shell_javascri...
content/shell/shell_javascript_dialog_win.cc:46: case IDOK:
nit: need indentation for each case per style guide

http://codereview.chromium.org/9918013/diff/4002/content/shell/shell_javascri...
content/shell/shell_javascript_dialog_win.cc:52: TCHAR* buf = new TCHAR[length];
nit: we don't use TCHAR in code. just use string_util.h's WriteInto with a
wstring

http://codereview.chromium.org/9918013/diff/4002/content/shell/shell_javascri...
content/shell/shell_javascript_dialog_win.cc:89: int dlgType;
nit: ditto for variable name

[Cem Kocagil, 2012-03-29 16:35:20]

Thanks for the nice review! I think I fixed the style issues now.

[jam, 2012-03-29 17:16:12]

lgtm

http://codereview.chromium.org/9918013/diff/4011/content/shell/shell_javascri...
File content/shell/shell_javascript_dialog.h (right):

http://codereview.chromium.org/9918013/diff/4011/content/shell/shell_javascri...
content/shell/shell_javascript_dialog.h:50: static INT_PTR CALLBACK
DialogProc(HWND hDlg, UINT msg, WPARAM wParam,
nit: here too

[commit-bot: I haz the power, 2012-03-29 17:43:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/9918013/3005

[commit-bot: I haz the power, 2012-03-29 17:43:43]

Can't apply patch for file content/shell/shell.rc.
While running patch -p1 --forward --force;
patching file content/shell/shell.rc
Hunk #1 FAILED at 59.
1 out of 1 hunk FAILED -- saving rejects to file content/shell/shell.rc.rej

[commit-bot: I haz the power, 2012-03-29 17:53:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/9918013/3005

[commit-bot: I haz the power, 2012-03-29 17:53:55]

Can't apply patch for file content/shell/shell.rc.
While running patch -p1 --forward --force;
patching file content/shell/shell.rc
Hunk #1 FAILED at 59.
1 out of 1 hunk FAILED -- saving rejects to file content/shell/shell.rc.rej

[commit-bot: I haz the power, 2012-03-29 23:19:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/9918013/9010

[commit-bot: I haz the power, 2012-03-29 23:19:38]

Can't apply patch for file content/shell/shell.rc.
While running patch -p1 --forward --force;
patching file content/shell/shell.rc
Hunk #1 FAILED at 59.
1 out of 1 hunk FAILED -- saving rejects to file content/shell/shell.rc.rej

[Cem Kocagil, 2012-03-30 01:35:57]

For some reason the patch can't be applied, but I can apply it locally. Any
ideas?

[jam, 2012-03-30 15:49:56]

looks like this was because it didn't have the eol-style svn property. I just
set it in 129862, so if you sync past that revision and reupload, it should
patch.

[commit-bot: I haz the power, 2012-03-30 16:28:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/9918013/22001

[commit-bot: I haz the power, 2012-03-30 19:53:31]

Change committed as 129904

[Mihai Parparita -not on Chrome, 2012-03-30 20:40:03]

On 2012/03/30 19:53:31, I haz the power (commit-bot) wrote:
> Change committed as 129904

It looks like shell_javascript_dialog_win.cc has the executable bit set, which
makes bots complain:

http://build.chromium.org/p/chromium/builders/Linux/builds/21864/steps/check_...

The patch also says "new file mode 100755"

[Mihai Parparita -not on Chrome, 2012-03-30 20:48:32]

On 2012/03/30 20:40:03, Mihai Parparita wrote:
> On 2012/03/30 19:53:31, I haz the power (commit-bot) wrote:
> > Change committed as 129904
> 
> It looks like shell_javascript_dialog_win.cc has the executable bit set, which
> makes bots complain:
> 
>
http://build.chromium.org/p/chromium/builders/Linux/builds/21864/steps/check_...
> 
> The patch also says "new file mode 100755"

Fixed with http://crrev.com/129926