Add a blocking version of the sync promo dialog

Add a blocking version of the sync promo dialog

This is a new version of the sync promo that is displayed inside a blocking dialog window.

If a new browser window is spawned from inside the sync promo then we close the dialog box and foucs on the new window. Any remaining first run tabs are added as inactive tabs to the new browser window.

Screenshots: http://imgur.com/iBspQ

BUG=107219
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119827

[sail, 2012-01-30 08:04:14]

dbeam: promo stuff
jhawkins: webui/* OWNERS
erg: gtk/* OWNERS
rsesek: cocoa/* OWNERS
sky@chromium.org: browser_init and views/* OWNERS

[sky, 2012-01-30 15:27:47]

http://codereview.chromium.org/9225053/diff/4001/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9225053/diff/4001/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:1172: if (profile_ && !browser &&
process_startup &&
Move this into its own function.

http://codereview.chromium.org/9225053/diff/4001/chrome/browser/ui/views/html...
File chrome/browser/ui/views/html_dialog_view.cc (right):

http://codereview.chromium.org/9225053/diff/4001/chrome/browser/ui/views/html...
chrome/browser/ui/views/html_dialog_view.cc:275: if (delegate_ &&
delegate_->HandlerAddNewContents(
HandlerAddNewContents->HandleAddNewContents.

http://codereview.chromium.org/9225053/diff/4001/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h (right):

http://codereview.chromium.org/9225053/diff/4001/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:58: };
DISALLOW_COPY_AND_ASSIGN

[sail, 2012-01-30 17:13:03]

Address review comments and fixed mac and GTK builds. Please take another look.

https://chromiumcodereview.appspot.com/9225053/diff/4001/chrome/browser/ui/br...
File chrome/browser/ui/browser_init.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/4001/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1172: if (profile_ && !browser &&
process_startup &&
On 2012/01/30 15:27:47, sky wrote:
> Move this into its own function.

Done.

https://chromiumcodereview.appspot.com/9225053/diff/4001/chrome/browser/ui/vi...
File chrome/browser/ui/views/html_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/4001/chrome/browser/ui/vi...
chrome/browser/ui/views/html_dialog_view.cc:275: if (delegate_ &&
delegate_->HandlerAddNewContents(
On 2012/01/30 15:27:47, sky wrote:
> HandlerAddNewContents->HandleAddNewContents.

Done.

https://chromiumcodereview.appspot.com/9225053/diff/4001/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h (right):

https://chromiumcodereview.appspot.com/9225053/diff/4001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:58: };
On 2012/01/30 15:27:47, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[sail, 2012-01-30 17:38:55]

ben: FYI

[Dan Beam, 2012-01-30 18:10:05]

Perhaps this isn't the way we do things, but it seems like there's a lot of
duplication of code between GTK/Cocoa/Views implementations. Could we just #if
defined() or make this templated if there's enough re-use?  I just hate to see
the same methods repeated 3 or 4 times (seemingly) in every file just doing the
same thing, but perhaps that's the nature of having to do native work ...
(slight differentiation on every platform).  I've just done very little native
work so far.

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/br...
File chrome/browser/ui/browser_init.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1161: if (!profile_ || *browser ||
!process_startup ||
You may want to leave comment clarifying what you're doing here.  I'm assuming
you don't have a ! in front of *browser as you're overwriting this later on?

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/co...
File chrome/browser/ui/cocoa/html_dialog_window_controller.mm (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/co...
chrome/browser/ui/cocoa/html_dialog_window_controller.mm:233: source,
new_contents, disposition, initial_pos, user_gesture)) {
probably need to indent this + ^\s{4}

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/gt...
File chrome/browser/ui/gtk/html_dialog_gtk.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/gt...
chrome/browser/ui/gtk/html_dialog_gtk.cc:175: source, new_contents, disposition,
initial_pos, user_gesture)) {
same here

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
File chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:56: if (source &&
source->IsCrashed() &&
why are you asking if the source is crashed?

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:58:
params.transition == content::PAGE_TRANSITION_LINK)
curlies

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:63:
nav_params.user_gesture = true;
what is this for?

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:93:
TabContentsWrapper* wrapper = new TabContentsWrapper(new_contents);
There's no chance of this leaking here, as ownership will be transferred,
correct? (and deleted via ~ScopedTargetContentsOwner, right?)

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
File chrome/browser/ui/webui/html_dialog_ui.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/html_dialog_ui.cc:114: return false;
Why do we do this?  To implement an interface and allow other delegates that
actually know how to do this to have precedence?

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc:53: void
SyncPromoDialog::GetDialogSize(gfx::Size* size) const {
I'm assuming this is handled somewhere, but what happens to the old *size?  can
you DCHECK() it's NULL or delete it if not or something reasonable?

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc:83:
sync_promo_was_closed_ = params.disposition == CURRENT_TAB;
what is this doing?

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:24: Browser*
spawned_browser() { return spawned_browser_; }
() const {, possibly ^const or ^const Browser&? (I assume you probably modify
spawned_browser_ after returning it though?)

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:27: bool
sync_promo_was_closed() { return sync_promo_was_closed_; }
 const {, ^const?

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:30: //
HtmlDialogUIDelegate:
are these all just implementing HtmlDialogUIDelegate?  if not, more comments...

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:53: Profile* profile_;
if you feel it's useful, add more comments to internal private members here as
well

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_promo/sync_promo_handler2.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_handler2.cc:208: if (|browser ||
!browser->IsAttemptingToCloseBrowser()) {
s/|/!/

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_promo/sync_promo_trial.h (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_trial.h:9: #include
"chrome/browser/ui/webui/sync_promo/sync_promo_ui.h"
convert to forward, possibly?

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:302: if (value >=
VERSION_DEFAULT && value < VERSION_COUNT)
convert to 1 if?

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:225: int title_id = is_launch_page
? IDS_SYNC_PROMO_TITLE_SHORT :
where did this change? did this value already exist?

[sky, 2012-01-30 18:13:41]

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:1154: size_t
BrowserInit::LaunchWithProfile::ShowSyncPromoDialog(
Make position match that of header.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:1175: return 0;
I don't understand the return value here. CAn you add comments to clarify why
it's 0 and 1 and doesn't relate to i in anyway.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:1233: index = 0;
Add a comment as to why you're forcing these to appear first.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.h (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.h:251: bool process_startup,
out params should be last, so make process_startup the first param

[sail, 2012-01-30 21:00:38]

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/br...
File chrome/browser/ui/browser_init.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1154: size_t
BrowserInit::LaunchWithProfile::ShowSyncPromoDialog(
On 2012/01/30 18:13:41, sky wrote:
> Make position match that of header.

Done.

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1161: if (!profile_ || *browser ||
!process_startup ||
On 2012/01/30 18:10:05, Dan Beam wrote:
> You may want to leave comment clarifying what you're doing here.  I'm assuming
> you don't have a ! in front of *browser as you're overwriting this later on?

Done.

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1175: return 0;
On 2012/01/30 18:13:41, sky wrote:
> I don't understand the return value here. CAn you add comments to clarify why
> it's 0 and 1 and doesn't relate to i in anyway.

Done.

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1233: index = 0;
On 2012/01/30 18:13:41, sky wrote:
> Add a comment as to why you're forcing these to appear first.

Done.

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/co...
File chrome/browser/ui/cocoa/html_dialog_window_controller.mm (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/co...
chrome/browser/ui/cocoa/html_dialog_window_controller.mm:233: source,
new_contents, disposition, initial_pos, user_gesture)) {
On 2012/01/30 18:10:05, Dan Beam wrote:
> probably need to indent this + ^\s{4}

Done.

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/gt...
File chrome/browser/ui/gtk/html_dialog_gtk.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/gt...
chrome/browser/ui/gtk/html_dialog_gtk.cc:175: source, new_contents, disposition,
initial_pos, user_gesture)) {
On 2012/01/30 18:10:05, Dan Beam wrote:
> same here

Done.

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
File chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:56: if (source &&
source->IsCrashed() &&
On 2012/01/30 18:10:05, Dan Beam wrote:
> why are you asking if the source is crashed?

not sure, this isn't my code, this is existing code moved to a new place

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:58:
params.transition == content::PAGE_TRANSITION_LINK)
On 2012/01/30 18:10:05, Dan Beam wrote:
> curlies

Done.

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:63:
nav_params.user_gesture = true;
On 2012/01/30 18:10:05, Dan Beam wrote:
> what is this for?

not sure

https://chromiumcodereview.appspot.com/9225053/diff/8002/chrome/browser/ui/we...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:93:
TabContentsWrapper* wrapper = new TabContentsWrapper(new_contents);
On 2012/01/30 18:10:05, Dan Beam wrote:
> There's no chance of this leaking here, as ownership will be transferred,
> correct? (and deleted via ~ScopedTargetContentsOwner, right?)

not sure (existing code)

[Elliot Glaysher, 2012-01-30 21:01:39]

I am basically fine with the gtk changes modulo Dan as long as something like
http://code.google.com/p/chromium/issues/detail?id=111985 is fixed before branch
point and makes it into M18.

lgtm

[James Hawkins, 2012-01-30 21:02:14]

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
File chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h:23: // URL is opened
in. Returns the browser spawned by the operatoin.
operation

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h:24: static Browser*
StaticOpenURLFromTab(Profile* profile,
Document |profile|, |source|, |params|.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h:31: static Browser*
StaticAddNewContents(Profile* profile,
Document remaining params.

[sail, 2012-01-30 21:52:36]

addressed review comments, please take another look

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
File chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h:23: // URL is opened
in. Returns the browser spawned by the operatoin.
On 2012/01/30 21:02:14, James Hawkins wrote:
> operation

Done.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h:24: static Browser*
StaticOpenURLFromTab(Profile* profile,
On 2012/01/30 21:02:14, James Hawkins wrote:
> Document |profile|, |source|, |params|.

Done.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h:31: static Browser*
StaticAddNewContents(Profile* profile,
On 2012/01/30 21:02:14, James Hawkins wrote:
> Document remaining params.

Done.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
File chrome/browser/ui/webui/html_dialog_ui.cc (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/html...
chrome/browser/ui/webui/html_dialog_ui.cc:114: return false;
On 2012/01/30 18:10:05, Dan Beam wrote:
> Why do we do this?  To implement an interface and allow other delegates that
> actually know how to do this to have precedence?

Yea, this is the same as the HandleContextMenu case. Most delegates want the
default behavior. Those delegates that want to have custom behavior will return
true.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc:53: void
SyncPromoDialog::GetDialogSize(gfx::Size* size) const {
On 2012/01/30 18:10:05, Dan Beam wrote:
> I'm assuming this is handled somewhere, but what happens to the old *size? 
can
> you DCHECK() it's NULL or delete it if not or something reasonable?

there's only one size variable, there's nothing to delete here. The user passes
in their gfx::Size variable and fill it in.
added a dcheck for extra measure

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc:83:
sync_promo_was_closed_ = params.disposition == CURRENT_TAB;
On 2012/01/30 18:10:05, Dan Beam wrote:
> what is this doing?

if open URL request is for the current tab then that means the sync promo will
be closed, added comment

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:24: Browser*
spawned_browser() { return spawned_browser_; }
On 2012/01/30 18:10:05, Dan Beam wrote:
> () const {, possibly ^const or ^const Browser&? (I assume you probably modify
> spawned_browser_ after returning it though?)

Done.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:27: bool
sync_promo_was_closed() { return sync_promo_was_closed_; }
On 2012/01/30 18:10:05, Dan Beam wrote:
>  const {, ^const?

Done.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:30: //
HtmlDialogUIDelegate:
On 2012/01/30 18:10:05, Dan Beam wrote:
> are these all just implementing HtmlDialogUIDelegate?  if not, more
comments...

they're just implementing

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:53: Profile* profile_;
On 2012/01/30 18:10:05, Dan Beam wrote:
> if you feel it's useful, add more comments to internal private members here as
> well

i think they're pretty straight forward, the more interesting ones have comments
in the getter methods above

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo/sync_promo_handler2.cc (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_handler2.cc:208: if (|browser ||
!browser->IsAttemptingToCloseBrowser()) {
On 2012/01/30 18:10:05, Dan Beam wrote:
> s/|/!/

Done. Ouch, good catch.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo/sync_promo_trial.h (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_trial.h:9: #include
"chrome/browser/ui/webui/sync_promo/sync_promo_ui.h"
On 2012/01/30 18:10:05, Dan Beam wrote:
> convert to forward, possibly?

there's no easy way to forward declare enums

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:302: if (value >=
VERSION_DEFAULT && value < VERSION_COUNT)
On 2012/01/30 18:10:05, Dan Beam wrote:
> convert to 1 if?

At 3 lines it's getting hard for me to follow

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_handler.cc:225: int title_id = is_launch_page
? IDS_SYNC_PROMO_TITLE_SHORT :
On 2012/01/30 18:10:05, Dan Beam wrote:
> where did this change? did this value already exist?

yea, this value already existed, I was using the wrong one here

[sky, 2012-01-30 23:08:14]

LGTM

http://codereview.chromium.org/9225053/diff/8006/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.h (right):

http://codereview.chromium.org/9225053/diff/8006/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.h:251: bool process_startup,
out params should be last, so make process_startup the first param

[sail, 2012-01-30 23:45:53]

http://codereview.chromium.org/9225053/diff/8006/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.h (right):

http://codereview.chromium.org/9225053/diff/8006/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.h:251: bool process_startup,
On 2012/01/30 23:08:14, sky wrote:
> out params should be last, so make process_startup the first param

Done.

[Robert Sesek, 2012-01-31 00:07:57]

cocoa/ lgtm

[James Hawkins, 2012-01-31 00:23:22]

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/htm...
File chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc (right):

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/htm...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:43: Profile*
profile,
Optional: Save space and put more than one parameter on a line.  The
one-per-line requirement is only for the declaration.

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/htm...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:47: if (!profile)
Under what circumstances is profile NULL?

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/htm...
File chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h (right):

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/htm...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h:22: // Opens a new
URL inside |source| (if source is 0 open in the current
s/0/NULL/

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/htm...
File chrome/browser/ui/webui/html_dialog_ui.h (right):

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/htm...
chrome/browser/ui/webui/html_dialog_ui.h:92: virtual bool
HandleAddNewContents(content::WebContents* source,
Document params.

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc (right):

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc:55: *size =
gfx::Size(500, 540);
Use static const vars.

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc:66: void
SyncPromoDialog::OnCloseContents(content::WebContents* source,
Must we override all of these methods we're not specializing?

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h (right):

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:14: // Shows the sync
promo in a HTML dialog.
Optional nit: s/a/an/

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:17:
SyncPromoDialog(Profile* profile, GURL url);
Document params.

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:53: Profile* profile_;
Document member variables.

[Dan Beam, 2012-01-31 00:31:39]

lgtm with nits & jhawkins' feedback fulfilled

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo/sync_promo_trial.h (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_trial.h:9: #include
"chrome/browser/ui/webui/sync_promo/sync_promo_ui.h"
On 2012/01/30 21:52:36, sail wrote:
> On 2012/01/30 18:10:05, Dan Beam wrote:
> > convert to forward, possibly?
> 
> there's no easy way to forward declare enums

Ah, OK.

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

http://codereview.chromium.org/9225053/diff/8002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:302: if (value >=
VERSION_DEFAULT && value < VERSION_COUNT)
On 2012/01/30 21:52:36, sail wrote:
> On 2012/01/30 18:10:05, Dan Beam wrote:
> > convert to 1 if?
> 
> At 3 lines it's getting hard for me to follow

Fine.

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/views/htm...
File chrome/browser/ui/views/html_dialog_view.cc (right):

http://codereview.chromium.org/9225053/diff/12033/chrome/browser/ui/views/htm...
chrome/browser/ui/views/html_dialog_view.cc:276: source, new_contents,
disposition, initial_pos, user_gesture)) {
missed this one with indent

[sail, 2012-01-31 01:03:46]

This CL also contains a 0.2 fade in animation on load. This was suggested by
Glen to offset the the white flash that users see when the page loads.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/v...
File chrome/browser/ui/views/html_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/v...
chrome/browser/ui/views/html_dialog_view.cc:276: source, new_contents,
disposition, initial_pos, user_gesture)) {
On 2012/01/31 00:31:40, Dan Beam wrote:
> missed this one with indent

Done.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
File chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:43: Profile*
profile,
On 2012/01/31 00:23:22, James Hawkins wrote:
> Optional: Save space and put more than one parameter on a line.  The
> one-per-line requirement is only for the declaration.

Done.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.cc:47: if (!profile)
On 2012/01/31 00:23:22, James Hawkins wrote:
> Under what circumstances is profile NULL?

I'm not sure. This is just existing code that I moved into a shared static
function.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
File chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
chrome/browser/ui/webui/html_dialog_tab_contents_delegate.h:22: // Opens a new
URL inside |source| (if source is 0 open in the current
On 2012/01/31 00:23:22, James Hawkins wrote:
> s/0/NULL/

Done.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
File chrome/browser/ui/webui/html_dialog_ui.h (right):

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
chrome/browser/ui/webui/html_dialog_ui.h:92: virtual bool
HandleAddNewContents(content::WebContents* source,
On 2012/01/31 00:23:22, James Hawkins wrote:
> Document params.

Done.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc (right):

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc:55: *size =
gfx::Size(500, 540);
On 2012/01/31 00:23:22, James Hawkins wrote:
> Use static const vars.

Done.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.cc:66: void
SyncPromoDialog::OnCloseContents(content::WebContents* source,
On 2012/01/31 00:23:22, James Hawkins wrote:
> Must we override all of these methods we're not specializing?

They're all abstract functions so we have to override them.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
File chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h (right):

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:14: // Shows the sync
promo in a HTML dialog.
On 2012/01/31 00:23:22, James Hawkins wrote:
> Optional nit: s/a/an/

Done.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:17:
SyncPromoDialog(Profile* profile, GURL url);
On 2012/01/31 00:23:22, James Hawkins wrote:
> Document params.

Done.

https://chromiumcodereview.appspot.com/9225053/diff/12033/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_promo/sync_promo_dialog.h:53: Profile* profile_;
On 2012/01/31 00:23:22, James Hawkins wrote:
> Document member variables.

Done.

[James Hawkins, 2012-01-31 01:04:48]

lgtm