Content shell/GTK: Add JavaScript dialog support.

Content shell/GTK: Add JavaScript dialog support.

- Add popup dialog support for JavaScript alert/confirm/prompt fuctions
  on content shell GTK port.

BUG=90445, 138603
TEST=
TBR=jam@chromium.org

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

[Shouqun Liu, 2012-07-30 06:51:10]

Hi, this patch adds the JavaScript dialog functionality to content shell on GTK,
thanks for review.

[jochen (gone - plz use gerrit), 2012-07-30 07:32:23]

http://codereview.chromium.org/10824077/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10824077/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:84: 'shell/shell_javascript_dialog_gtk.cc',
nit alphabetical ordering

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
File content/shell/shell_javascript_dialog.h (right):

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog.h:9: #if defined(OS_LINUX)
should be TOOLKIT_GTK?

also, please add an empty line before this one

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
File content/shell/shell_javascript_dialog_gtk.cc (right):

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:18: const char kPromptTextId[] =
"content_shell_prompt_text";
not needed?

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:30: } // namespace
nit. two spaces before //

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:35: class ShellJavaScriptDialog;
not needed

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:70: gtk_dialog_ =
gtk_message_dialog_new(parent_window_,
if you need to wrap, either all parameters go on the next line, or each
parameter goes on its own line, but all aligned. (here and below)

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:82: // Adjust content area as
needed.  Set up the prompt text entry.
nit. comments that just repeat what the code does don't add much value IMO

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:123: } //namespace content
nit. two spaces before // and one after

[Shouqun Liu, 2012-07-30 08:08:43]

Thanks for your review, I have fixed the style issues

http://codereview.chromium.org/10824077/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10824077/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:84: 'shell/shell_javascript_dialog_gtk.cc',
On 2012/07/30 07:32:23, jochen wrote:
> nit alphabetical ordering

Done.

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
File content/shell/shell_javascript_dialog.h (right):

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog.h:9: #if defined(OS_LINUX)
On 2012/07/30 07:32:23, jochen wrote:
> should be TOOLKIT_GTK?
> 
> also, please add an empty line before this one

Done.

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
File content/shell/shell_javascript_dialog_gtk.cc (right):

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:18: const char kPromptTextId[] =
"content_shell_prompt_text";
the kPromptTextId is used to get text value from gtk_text_entry
On 2012/07/30 07:32:23, jochen wrote:
> not needed?

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:30: } // namespace
On 2012/07/30 07:32:23, jochen wrote:
> nit. two spaces before //

Done.

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:35: class ShellJavaScriptDialog;
On 2012/07/30 07:32:23, jochen wrote:
> not needed

Done.

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:70: gtk_dialog_ =
gtk_message_dialog_new(parent_window_,
Fixed the wrap style
On 2012/07/30 07:32:23, jochen wrote:
> if you need to wrap, either all parameters go on the next line, or each
> parameter goes on its own line, but all aligned. (here and below)

Done.

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:82: // Adjust content area as
needed.  Set up the prompt text entry.
On 2012/07/30 07:32:23, jochen wrote:
> nit. comments that just repeat what the code does don't add much value IMO

Done.

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:123: } //namespace content
On 2012/07/30 07:32:23, jochen wrote:
> nit. two spaces before // and one after

Done.

[jochen (gone - plz use gerrit), 2012-07-30 08:15:49]

lgtm with nits

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
File content/shell/shell_javascript_dialog_gtk.cc (right):

http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript...
content/shell/shell_javascript_dialog_gtk.cc:18: const char kPromptTextId[] =
"content_shell_prompt_text";
On 2012/07/30 08:08:44, Shouqun Liu wrote:
> the kPromptTextId is used to get text value from gtk_text_entry
ah, sorry, missed that

http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascr...
File content/shell/shell_javascript_dialog_creator.h (right):

http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascr...
content/shell/shell_javascript_dialog_creator.h:51: #if defined(OS_MACOSX) ||
defined(OS_WIN) || defined(OS_LINUX)
please change OS_LINUX to TOOLKIT_GTK everywhere

http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascr...
File content/shell/shell_javascript_dialog_gtk.cc (right):

http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascr...
content/shell/shell_javascript_dialog_gtk.cc:74: g_signal_connect(gtk_dialog_,
"delete-event",
nit. each parameter on it's own line

[Shouqun Liu, 2012-07-30 08:26:20]

Patch updated with nits fixed, thanks!

http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascr...
File content/shell/shell_javascript_dialog_creator.h (right):

http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascr...
content/shell/shell_javascript_dialog_creator.h:51: #if defined(OS_MACOSX) ||
defined(OS_WIN) || defined(OS_LINUX)
Sorry, forget this...
On 2012/07/30 08:15:49, jochen wrote:
> please change OS_LINUX to TOOLKIT_GTK everywhere

Done.

http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascr...
File content/shell/shell_javascript_dialog_gtk.cc (right):

http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascr...
content/shell/shell_javascript_dialog_gtk.cc:74: g_signal_connect(gtk_dialog_,
"delete-event",
On 2012/07/30 08:15:49, jochen wrote:
> nit. each parameter on it's own line

Done.

[jochen (gone - plz use gerrit), 2012-07-30 11:40:07]

I submitted tryjobs here:
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/43257
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/43674
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/48093
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/38096
http://build.chromium.org/p/tryserver.chromium/builders/android/builds/24930

assuming they pass, you can TBR the content_shell.gypi change to jam@ and commit

[jochen (gone - plz use gerrit), 2012-07-30 11:44:26]

seems I've figured out how to make the tryjob results appear on this review :)

[Shouqun Liu, 2012-07-30 12:47:23]

Many thanks! Seems the tryjobs are failed at patching, I'll try to rebase and
re-try them

On 2012/07/30 11:40:07, jochen wrote:
> I submitted tryjobs here:
> http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/43257
> http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/43674
> http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/48093
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/38096
> http://build.chromium.org/p/tryserver.chromium/builders/android/builds/24930
> 
> assuming they pass, you can TBR the content_shell.gypi change to jam@ and
commit

[jam, 2012-07-30 15:23:50]

gyp changes lgtm

[commit-bot: I haz the power, 2012-07-30 15:34:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10824077/4003

[commit-bot: I haz the power, 2012-07-30 20:52:49]

Change committed as 149019