Bookmark sync promo for Views.

Bookmark sync promo for Views.

Adds a bookmark sync promo at the bottom of the bookmark bubble. The promo is only shown to users that are not signed in. Clicking the link in the sync promo opens the sign in page.

Screenshot with layout_constants: http://i.imgur.com/aYG2y2n.png
(old screenshot: http://i.imgur.com/IqBNGXK.png)

Design document: https://docs.google.com/a/google.com/document/d/1Jduor831szvkdq3cwlWrFw91a8Pzu1ynAs5hlGc7MvM/edit?usp=sharing

TEST=
1. Open Chromium with the --enable-bookmark-sync-promo flag. 
2. Make sure you are not signed in. 
3. Open the bookmark bubble (click on the bookmark star or Ctrl+D).
Expected result: a sync promo is displayed at the bottom of the bookmark bubble.
4. Click the "Sign in" link in the sync promo.
Expected result: the sign in page is loaded.

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

[fdoray, 2013-07-04 18:15:58]

Could you review this CL?

** Changes to ui/views/controls/* are already being reviewed in this CL:
https://codereview.chromium.org/17756003/ . 17756003 will need to be committed
before 18603006 **

[Roger Tawa OOO till Jul 10th, 2013-07-11 15:28:30]

You can remove the changes for cl/17756003 from this one by making the former's
git branch upstream from the latter's git branch.  Come see me if you need more
details.

Some questions below.

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/sync/syn...
File chrome/browser/ui/sync/sync_promo_ui.cc (left):

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/sync/syn...
chrome/browser/ui/sync/sync_promo_ui.cc:103: 
Why do we need this check?  We don't suppress other sign in access points in
incognito.  Also, it seems from section "Behavior of the “Sign in” link", 2a, in
the design doc that incognito would be supported.

If we do want to suppress in incognito, maybe add a test for this case?

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:247: const int
kSyncPromoColumnSetID = 2;
Would it be better to put the three constants in an enum at the top of the
function?

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc (right):

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc:69:
promo_label->AddStyleRange(after_link_range, promo_style);
Are the range not allowed to overlap?

[fdoray, 2013-07-11 19:00:39]

I made the changes requested by Roger.

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/sync/syn...
File chrome/browser/ui/sync/sync_promo_ui.cc (left):

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/sync/syn...
chrome/browser/ui/sync/sync_promo_ui.cc:103: 
SyncPromoUI::ShouldShowSyncPromo is used to determine if the bookmark sync promo
should be displayed. Removing this makes the sync promo visible in incognito, as
we want.

To my knowledge, SyncPromoUI::ShouldShowSyncPromo is never used in incognito
currently, so this change has no impact on existing code.
- If you install an extension from incognito, ExtensionInstalledBubble is
displayed in a normal browser.
- The incognito NTP doesn't even call SyncPromoUI::ShouldShowSyncPromo.

On 2013/07/11 15:28:30, Roger Tawa wrote:
> Why do we need this check?  We don't suppress other sign in access points in
> incognito.  Also, it seems from section "Behavior of the “Sign in” link", 2a,
in
> the design doc that incognito would be supported.
> 
> If we do want to suppress in incognito, maybe add a test for this case?

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:247: const int
kSyncPromoColumnSetID = 2;
On 2013/07/11 15:28:30, Roger Tawa wrote:
> Would it be better to put the three constants in an enum at the top of the
> function?

Done.

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc (right):

https://codereview.chromium.org/18603006/diff/6001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc:69:
promo_label->AddStyleRange(after_link_range, promo_style);
On 2013/07/11 15:28:30, Roger Tawa wrote:
> Are the range not allowed to overlap?

Overlap doesn't work in all cases.

It works if the start index of the link is 0, like in English. link_style is
used for the link and then StyledLabel tries to find another style for the
remaining text.

However, if in some language the link is in the middle of the sentence,
promo_style is picked at index 0 and StyledLabel use it for the whole label. It
doesn't look at the other styles and no link is rendered.

[fdoray, 2013-07-11 20:00:02]

+noms@
+bcwhite@
Could you review this CL?

[Roger Tawa OOO till Jul 10th, 2013-07-11 20:42:09]

lgtm

[noms, 2013-07-11 20:47:17]

lgtm

[fdoray, 2013-07-12 14:10:33]

+sky@
Owner for chrome/browser/ui/views/bookmarks/*

Could you review this CL?

[sky, 2013-07-15 18:14:10]

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:49: // Top padding of
the bubble.
Can we use the standard constants we have instead of one offs? I'm thinking of
the constants in layout_constants.

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:190: enum ColumnSetID
{
Chrome style is ALL_CAPS for enums.

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:250:
SyncPromoUI::ShouldShowSyncPromo(profile_)) {
I don't like changing the meaning of ShouldShowSyncPromo to mean should show
sync promo or bookmark bubble promo. There should be a separate function for the
bookmarks and it shouldn't be on SyncPromoUI.

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:260: Browser* browser
= chrome::FindBrowserWithProfile(
There is nothing that forces BookmarkBubbleView to be used by a browser. I think
it would be better to put this logic in a delegate so that BookmarkBubbleView
isn't dependent on a browser. This would likely simplify your test code too.

[fdoray, 2013-07-16 17:49:58]

Roger, could you review my comments?

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:49: // Top padding of
the bubble.
On 2013/07/15 18:14:10, sky wrote:
> Can we use the standard constants we have instead of one offs? I'm thinking of
> the constants in layout_constants.

Done for kBubblePadding. But there is no relevant constant to replace
kBubbleTopPadding, unless I don't respect the mock crbug.com/231694.

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:190: enum ColumnSetID
{
On 2013/07/15 18:14:10, sky wrote:
> Chrome style is ALL_CAPS for enums.

Done.

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:250:
SyncPromoUI::ShouldShowSyncPromo(profile_)) {
ShouldShowSyncPromo is already used to determine if a sync promo should be
displayed at startup, on the NTP or in a bubble after an extension is installed.
Using it to determine if a sync promo should be displayed in the bookmark bubble
seems logical.

Is the problem that I removed "if (profile->IsOffTheRecord())" from the
function? This has no impact on existing code:

sync_promo_ui.cc(46): I copied the "if (profile->IsOffTheRecord())" check at the
beginning of the ShouldShowSyncPromoAtStartup function.

ntp_login_handler.cc(143): NTPLoginHandler is never instantiated for an
incognito NTP.

extension_installed_bubble.cc(137): The bubble is always shown in a normal
browser, even when the extension is installed from an incognito browser.


On 2013/07/15 18:14:10, sky wrote:
> I don't like changing the meaning of ShouldShowSyncPromo to mean should show
> sync promo or bookmark bubble promo. There should be a separate function for
the
> bookmarks and it shouldn't be on SyncPromoUI.

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:260: Browser* browser
= chrome::FindBrowserWithProfile(
I couldn't figure out how a delegate would be useful here. I tried something
else to make BookmarkBubbleView independent from BookmarkSyncPromoView and from
any browser. I you prefer a delegate, what would its interface look like?

On 2013/07/15 18:14:10, sky wrote:
> There is nothing that forces BookmarkBubbleView to be used by a browser. I
think
> it would be better to put this logic in a delegate so that BookmarkBubbleView
> isn't dependent on a browser. This would likely simplify your test code too.

[fdoray, 2013-07-16 17:58:56]

Scott, could you review this CL?

I addressed all your comments.

https://codereview.chromium.org/18603006/diff/45001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc (right):

https://codereview.chromium.org/18603006/diff/45001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc:33: const int
kVerticalPadding = 15;
This comes from the designer. Not defined in layout_constants.

[Roger Tawa OOO till Jul 10th, 2013-07-16 19:39:30]

On 2013/07/16 17:49:58, fdoray wrote:
> Roger, could you review my comments?
> 
>
https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
> File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):
> 
>
https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
> chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:49: // Top padding
of
> the bubble.
> On 2013/07/15 18:14:10, sky wrote:
> > Can we use the standard constants we have instead of one offs? I'm thinking
of
> > the constants in layout_constants.
> 
> Done for kBubblePadding. But there is no relevant constant to replace
> kBubbleTopPadding, unless I don't respect the mock crbug.com/231694.
> 
>
https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
> chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:190: enum
ColumnSetID
> {
> On 2013/07/15 18:14:10, sky wrote:
> > Chrome style is ALL_CAPS for enums.
> 
> Done.
> 
>
https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
> chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:250:
> SyncPromoUI::ShouldShowSyncPromo(profile_)) {
> ShouldShowSyncPromo is already used to determine if a sync promo should be
> displayed at startup, on the NTP or in a bubble after an extension is
installed.
> Using it to determine if a sync promo should be displayed in the bookmark
bubble
> seems logical.
> 
> Is the problem that I removed "if (profile->IsOffTheRecord())" from the
> function? This has no impact on existing code:
> 
> sync_promo_ui.cc(46): I copied the "if (profile->IsOffTheRecord())" check at
the
> beginning of the ShouldShowSyncPromoAtStartup function.
> 
> ntp_login_handler.cc(143): NTPLoginHandler is never instantiated for an
> incognito NTP.
> 
> extension_installed_bubble.cc(137): The bubble is always shown in a normal
> browser, even when the extension is installed from an incognito browser.
> 
> 
> On 2013/07/15 18:14:10, sky wrote:
> > I don't like changing the meaning of ShouldShowSyncPromo to mean should show
> > sync promo or bookmark bubble promo. There should be a separate function for
> the
> > bookmarks and it shouldn't be on SyncPromoUI.
> 
>
https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
> chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:260: Browser*
browser
> = chrome::FindBrowserWithProfile(
> I couldn't figure out how a delegate would be useful here. I tried something
> else to make BookmarkBubbleView independent from BookmarkSyncPromoView and
from
> any browser. I you prefer a delegate, what would its interface look like?
> 
> On 2013/07/15 18:14:10, sky wrote:
> > There is nothing that forces BookmarkBubbleView to be used by a browser. I
> think
> > it would be better to put this logic in a delegate so that
BookmarkBubbleView
> > isn't dependent on a browser. This would likely simplify your test code too.

Looks for good for changes to ShouldShowSyncPromo().  Thanks for checking
Francois.

[sky, 2013-07-16 21:23:00]

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:250:
SyncPromoUI::ShouldShowSyncPromo(profile_)) {
On 2013/07/16 17:49:58, fdoray wrote:
> ShouldShowSyncPromo is already used to determine if a sync promo should be
> displayed at startup, on the NTP or in a bubble after an extension is
installed.
> Using it to determine if a sync promo should be displayed in the bookmark
bubble
> seems logical.
> 
> Is the problem that I removed "if (profile->IsOffTheRecord())" from the
> function? This has no impact on existing code:
> 
> sync_promo_ui.cc(46): I copied the "if (profile->IsOffTheRecord())" check at
the
> beginning of the ShouldShowSyncPromoAtStartup function.
> 
> ntp_login_handler.cc(143): NTPLoginHandler is never instantiated for an
> incognito NTP.
> 
> extension_installed_bubble.cc(137): The bubble is always shown in a normal
> browser, even when the extension is installed from an incognito browser.
> 
> 
> On 2013/07/15 18:14:10, sky wrote:
> > I don't like changing the meaning of ShouldShowSyncPromo to mean should show
> > sync promo or bookmark bubble promo. There should be a separate function for
> the
> > bookmarks and it shouldn't be on SyncPromoUI.
> 

My concern still stands. Isn't SyncPromoUI a specific thing and not a handful of
different UI bits?

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:260: Browser* browser
= chrome::FindBrowserWithProfile(
On 2013/07/16 17:49:58, fdoray wrote:
> I couldn't figure out how a delegate would be useful here. I tried something
> else to make BookmarkBubbleView independent from BookmarkSyncPromoView and
from
> any browser. I you prefer a delegate, what would its interface look like?
> 
> On 2013/07/15 18:14:10, sky wrote:
> > There is nothing that forces BookmarkBubbleView to be used by a browser. I
> think
> > it would be better to put this logic in a delegate so that
BookmarkBubbleView
> > isn't dependent on a browser. This would likely simplify your test code too.
> 

class BookmarkBubbleViewDelegate {
 public:
  virtual void OnXXXClicked() = 0;
};

https://codereview.chromium.org/18603006/diff/45001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc (right):

https://codereview.chromium.org/18603006/diff/45001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc:33: const int
kVerticalPadding = 15;
On 2013/07/16 17:58:56, fdoray wrote:
> This comes from the designer. Not defined in layout_constants.

There is a reason why we try to use the same constants every where. Can you
please talk with the designer to make sure he really wants to deviate from
common constants?

[Roger Tawa OOO till Jul 10th, 2013-07-17 13:26:31]

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:250:
SyncPromoUI::ShouldShowSyncPromo(profile_)) {
On 2013/07/16 21:23:00, sky wrote:
> On 2013/07/16 17:49:58, fdoray wrote:
> > ShouldShowSyncPromo is already used to determine if a sync promo should be
> > displayed at startup, on the NTP or in a bubble after an extension is
> installed.
> > Using it to determine if a sync promo should be displayed in the bookmark
> bubble
> > seems logical.
> > 
> > Is the problem that I removed "if (profile->IsOffTheRecord())" from the
> > function? This has no impact on existing code:
> > 
> > sync_promo_ui.cc(46): I copied the "if (profile->IsOffTheRecord())" check at
> the
> > beginning of the ShouldShowSyncPromoAtStartup function.
> > 
> > ntp_login_handler.cc(143): NTPLoginHandler is never instantiated for an
> > incognito NTP.
> > 
> > extension_installed_bubble.cc(137): The bubble is always shown in a normal
> > browser, even when the extension is installed from an incognito browser.
> > 
> > 
> > On 2013/07/15 18:14:10, sky wrote:
> > > I don't like changing the meaning of ShouldShowSyncPromo to mean should
show
> > > sync promo or bookmark bubble promo. There should be a separate function
for
> > the
> > > bookmarks and it shouldn't be on SyncPromoUI.
> > 
> 
> My concern still stands. Isn't SyncPromoUI a specific thing and not a handful
of
> different UI bits?

Initially there was only one sync promo, but over time other sync promos have
been added.  They all use this static function to determine if they should show
themselves or not.  So its OK to reuse this function in different UI bits.

[fdoray, 2013-07-17 15:01:59]

Scott, I addressed all your comments. Could you review the CL?

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:49: // Top padding of
the bubble.
On 2013/07/16 17:49:58, fdoray wrote:
> On 2013/07/15 18:14:10, sky wrote:
> > Can we use the standard constants we have instead of one offs? I'm thinking
of
> > the constants in layout_constants.
> 
> Done for kBubblePadding. But there is no relevant constant to replace
> kBubbleTopPadding, unless I don't respect the mock crbug.com/231694.

Done.

https://codereview.chromium.org/18603006/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:260: Browser* browser
= chrome::FindBrowserWithProfile(
On 2013/07/16 21:23:00, sky wrote:
> On 2013/07/16 17:49:58, fdoray wrote:
> > I couldn't figure out how a delegate would be useful here. I tried something
> > else to make BookmarkBubbleView independent from BookmarkSyncPromoView and
> from
> > any browser. I you prefer a delegate, what would its interface look like?
> > 
> > On 2013/07/15 18:14:10, sky wrote:
> > > There is nothing that forces BookmarkBubbleView to be used by a browser. I
> > think
> > > it would be better to put this logic in a delegate so that
> BookmarkBubbleView
> > > isn't dependent on a browser. This would likely simplify your test code
too.
> > 
> 
> class BookmarkBubbleViewDelegate {
>  public:
>   virtual void OnXXXClicked() = 0;
> };

Done. This is true on all platforms, so I wrote a reusable delegate.

https://codereview.chromium.org/18603006/diff/45001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc (right):

https://codereview.chromium.org/18603006/diff/45001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc:33: const int
kVerticalPadding = 15;
I sent a screenshot of the result with a common constant to the designer. It's
not too different from his mock.

On 2013/07/16 21:23:00, sky wrote:
> On 2013/07/16 17:58:56, fdoray wrote:
> > This comes from the designer. Not defined in layout_constants.
> 
> There is a reason why we try to use the same constants every where. Can you
> please talk with the designer to make sure he really wants to deviate from
> common constants?

[sky, 2013-07-17 16:08:02]

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
File chrome/browser/ui/bookmarks/bookmark_bubble_delegate.h (right):

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_bubble_delegate.h:11: virtual
~BookmarkBubbleDelegate() {}
destructor before other methods.

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc (right):

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc:6: #include
"chrome/browser/ui/chrome_pages.h"
nit: newline between 5/6.

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc:13:
chrome::ShowBrowserSignin(browser_, SyncPromoUI::SOURCE_BOOKMARK_BUBBLE);
How do you know browser_ is valid at the time the callback is invoked?

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right):

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:38:
BookmarkBubbleDelegate* delegate,
Make this take a scoped_ptr so that ownership is clear.

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.h (right):

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.h:19: explicit
BookmarkSyncPromoView(BookmarkBubbleDelegate* delegate);
Document ownership.

[fdoray, 2013-07-17 19:39:56]

Scott, I addressed all your comments. Could you review the CL?

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
File chrome/browser/ui/bookmarks/bookmark_bubble_delegate.h (right):

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_bubble_delegate.h:11: virtual
~BookmarkBubbleDelegate() {}
On 2013/07/17 16:08:02, sky wrote:
> destructor before other methods.

Done.

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc (right):

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc:6: #include
"chrome/browser/ui/chrome_pages.h"
On 2013/07/17 16:08:02, sky wrote:
> nit: newline between 5/6.

Done.

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc:13:
chrome::ShowBrowserSignin(browser_, SyncPromoUI::SOURCE_BOOKMARK_BUBBLE);
Done. If browser_ is no longer valid, I pick either the most recently used
browser for |profile_| or I create a new one. 

On 2013/07/17 16:08:02, sky wrote:
> How do you know browser_ is valid at the time the callback is invoked?

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right):

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:38:
BookmarkBubbleDelegate* delegate,
On 2013/07/17 16:08:02, sky wrote:
> Make this take a scoped_ptr so that ownership is clear.

Done.

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.h (right):

https://codereview.chromium.org/18603006/diff/82001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.h:19: explicit
BookmarkSyncPromoView(BookmarkBubbleDelegate* delegate);
On 2013/07/17 16:08:02, sky wrote:
> Document ownership.

Done.

[sky, 2013-07-17 21:42:53]

https://codereview.chromium.org/18603006/diff/109001/chrome/browser/ui/bookma...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc (right):

https://codereview.chromium.org/18603006/diff/109001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc:39: browser_ =
new Browser(Browser::CreateParams(profile_,
Does this do the right thing if profile_ is OTR?

[fdoray, 2013-07-17 23:22:53]

Scott, I addressed your comment. You can review the CL again.
Thank you!

https://codereview.chromium.org/18603006/diff/109001/chrome/browser/ui/bookma...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc (right):

https://codereview.chromium.org/18603006/diff/109001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc:39: browser_ =
new Browser(Browser::CreateParams(profile_,
On 2013/07/17 21:42:54, sky wrote:
> Does this do the right thing if profile_ is OTR?

Done. The new code prevents the creation of 1 incognito browser + 1 normal
browser. If no browser exist, only one normal browser will be created.

[sky, 2013-07-18 16:08:42]

LGTM

https://codereview.chromium.org/18603006/diff/93002/chrome/browser/ui/bookmar...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc (right):

https://codereview.chromium.org/18603006/diff/93002/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc:44:
browser_->window()->Show();
DCHECK(tabstrip not empty?)

[fdoray, 2013-07-18 18:49:12]

Scott: I added a question to help me understand your last comment correctly.

https://codereview.chromium.org/18603006/diff/93002/chrome/browser/ui/bookmar...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc (right):

https://codereview.chromium.org/18603006/diff/93002/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc:44:
browser_->window()->Show();
On 2013/07/18 16:08:42, sky wrote:
> DCHECK(tabstrip not empty?)

The tab strip of a newly created browser IS empty. A new tab is opened by
chrome::ShowBrowserSignin. Do you want this check after line 27? Is the problem
that I show the browser before creating the first tab?

Let me know if I made the right change.

[sky, 2013-07-19 00:02:31]

https://codereview.chromium.org/18603006/diff/93002/chrome/browser/ui/bookmar...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc (right):

https://codereview.chromium.org/18603006/diff/93002/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.cc:44:
browser_->window()->Show();
On 2013/07/18 18:49:12, fdoray wrote:
> On 2013/07/18 16:08:42, sky wrote:
> > DCHECK(tabstrip not empty?)
> 
> The tab strip of a newly created browser IS empty. A new tab is opened by
> chrome::ShowBrowserSignin. Do you want this check after line 27? Is the
problem
> that I show the browser before creating the first tab?
> 
> Let me know if I made the right change.

Yes, sorry, DCHECK after ShowBrowserSignin.

[commit-bot: I haz the power, 2013-07-19 00:28:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18603006/118001

[commit-bot: I haz the power, 2013-07-19 01:01:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-19 12:53:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18603006/66002

[commit-bot: I haz the power, 2013-07-19 13:29:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18603006/161001

[commit-bot: I haz the power, 2013-07-19 14:07:13]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-19 14:33:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18603006/178001

[commit-bot: I haz the power, 2013-07-19 16:04:39]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-07-19 17:28:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18603006/148004

[commit-bot: I haz the power, 2013-07-19 18:27:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-19 23:24:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18603006/148004

[commit-bot: I haz the power, 2013-07-20 02:43:49]

Change committed as 212724