Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(300)

Issue 10824077: Content shell/GTK: Add JavaScript dialog support. (Closed)

Created:
8 years, 4 months ago by Shouqun Liu
Modified:
8 years, 4 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 17

Patch Set 2 : Fix the style #

Total comments: 4

Patch Set 3 : Fix the style nits #

Patch Set 4 : Supress the uninitailized buttons #

Unified diffs Side-by-side diffs Delta from patch set Stats (+152 lines, -5 lines) Patch
M content/content_shell.gypi View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/shell/shell_javascript_dialog.h View 1 3 chunks +9 lines, -0 lines 0 comments Download
M content/shell/shell_javascript_dialog_creator.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M content/shell/shell_javascript_dialog_creator.cc View 1 7 chunks +12 lines, -4 lines 0 comments Download
A content/shell/shell_javascript_dialog_gtk.cc View 1 2 3 1 chunk +127 lines, -0 lines 0 comments Download
M content/shell/shell_javascript_dialog_mac.mm View 1 chunk +1 line, -0 lines 0 comments Download
M content/shell/shell_javascript_dialog_win.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
Shouqun Liu
Hi, this patch adds the JavaScript dialog functionality to content shell on GTK, thanks for ...
8 years, 4 months ago (2012-07-30 06:51:10 UTC) #1
jochen (gone - plz use gerrit)
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#newcode84 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_dialog.h File content/shell/shell_javascript_dialog.h (right): http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript_dialog.h#newcode9 ...
8 years, 4 months ago (2012-07-30 07:32:23 UTC) #2
Shouqun Liu
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#newcode84 ...
8 years, 4 months ago (2012-07-30 08:08:43 UTC) #3
jochen (gone - plz use gerrit)
lgtm with nits http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript_dialog_gtk.cc File content/shell/shell_javascript_dialog_gtk.cc (right): http://codereview.chromium.org/10824077/diff/1/content/shell/shell_javascript_dialog_gtk.cc#newcode18 content/shell/shell_javascript_dialog_gtk.cc:18: const char kPromptTextId[] = "content_shell_prompt_text"; On ...
8 years, 4 months ago (2012-07-30 08:15:49 UTC) #4
Shouqun Liu
Patch updated with nits fixed, thanks! http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascript_dialog_creator.h File content/shell/shell_javascript_dialog_creator.h (right): http://codereview.chromium.org/10824077/diff/6001/content/shell/shell_javascript_dialog_creator.h#newcode51 content/shell/shell_javascript_dialog_creator.h:51: #if defined(OS_MACOSX) || ...
8 years, 4 months ago (2012-07-30 08:26:20 UTC) #5
jochen (gone - plz use gerrit)
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 ...
8 years, 4 months ago (2012-07-30 11:40:07 UTC) #6
jochen (gone - plz use gerrit)
seems I've figured out how to make the tryjob results appear on this review :)
8 years, 4 months ago (2012-07-30 11:44:26 UTC) #7
Shouqun Liu
Many thanks! Seems the tryjobs are failed at patching, I'll try to rebase and re-try ...
8 years, 4 months ago (2012-07-30 12:47:23 UTC) #8
jam
gyp changes lgtm
8 years, 4 months ago (2012-07-30 15:23:50 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10824077/4003
8 years, 4 months ago (2012-07-30 15:34:19 UTC) #10
commit-bot: I haz the power
8 years, 4 months ago (2012-07-30 20:52:49 UTC) #11
Change committed as 149019

Powered by Google App Engine
This is Rietveld 408576698