[Extensions Views] Update the extension installed bubble's sync promo

[Extensions Views] Update the extension installed bubble's sync promo

Update the extension installed bubble's sync promo to match that of the
bookmark added bubble (and rename BookmarkSyncPromoView to
BubbleSyncPromoView).

Also, restructure the layout of the extension installed bubble to use a grid
layout to chop out a couple hundred LOC.

BUG=551695
Committed: https://crrev.com/305da4552528094c32c9bd76867a3c3e6aec8797
Cr-Commit-Position: refs/heads/master@{#363892}

[Devlin, 2015-12-04 20:55:31]

rdevlin.cronin@chromium.org changed reviewers:
+ sky@chromium.org

[Devlin, 2015-12-04 20:55:31]

Another one for you, Scott.  Sorry for the churn in here - I figured it would be
easier to implement and review in the long-term to simplify the layout, but it
makes for a big diff. :/

Screenshot at https://drive.google.com/open?id=0BxJroP76GCDpRHpHZE96OVI3X1U
(chromium.org)

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:349: //
Views specific implementation.
None of this changed - Rietveld just doesn't know it.

[sky, 2015-12-04 23:41:06]

LGTM

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/extension_installed_bubble.cc (right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/extension_installed_bubble.cc:176: auto x = new
ExtensionInstalledBubbleObserver(
x, huh? bubble_observer?

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/extension_installed_bubble.cc:180: if
(reg->enabled_extensions().GetByID(extension->id())) {
nit: no {}

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:56:
views::Label* GetLabel(const base::string16& text, const gfx::FontList& font) {
nit: Create is better than Get here as this creates a new instance.

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:311: auto
add_content_view = [&layout](views::View* view) {
tricky

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.h
(right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:54: enum
Flavors {
nit: your description says options, which IMO is a better name rather than
Flavors.

[Devlin, 2015-12-05 01:10:14]

rdevlin.cronin@chromium.org changed reviewers:
+ finnur@chromium.org

[Devlin, 2015-12-05 01:10:15]

+Finnur for an extra pair of eyes on the layout, since (I think?) he did the
original bubble and layout is tricky. :)

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/extension_installed_bubble.cc (right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/extension_installed_bubble.cc:176: auto x = new
ExtensionInstalledBubbleObserver(
On 2015/12/04 23:41:06, sky wrote:
> x, huh? bubble_observer?

D'oh!  This was all for testing.  I remembered to revert the other files, but
missed this one.

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/extension_installed_bubble.cc:180: if
(reg->enabled_extensions().GetByID(extension->id())) {
On 2015/12/04 23:41:06, sky wrote:
> nit: no {}

Ditto.

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:56:
views::Label* GetLabel(const base::string16& text, const gfx::FontList& font) {
On 2015/12/04 23:41:06, sky wrote:
> nit: Create is better than Get here as this creates a new instance.

Done.

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:311: auto
add_content_view = [&layout](views::View* view) {
On 2015/12/04 23:41:06, sky wrote:
> tricky

Good tricky or bad tricky? ;)

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.h
(right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:54: enum
Flavors {
On 2015/12/04 23:41:06, sky wrote:
> nit: your description says options, which IMO is a better name rather than
> Flavors.

Carried over flavors from the old code.  Replaced with Options.

[sky, 2015-12-07 16:07:45]

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:311: auto
add_content_view = [&layout](views::View* view) {
On 2015/12/05 01:10:15, Devlin wrote:
> On 2015/12/04 23:41:06, sky wrote:
> > tricky
> 
> Good tricky or bad tricky? ;)

I say tricky as a normal function would work just fine here and not be as
subtle.

[Finnur, 2015-12-07 18:10:02]

I don't see anything obviously wrong with this, and it is hard to review layout
code for correctness. Can you provide screenshots on the bug (different flavors,
or options as you call them now)...

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/bubble_sync_promo_delegate.h (right):

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/bubble_sync_promo_delegate.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:253:
MAIN_COLUMN_SET,
nit: s/SET/SET = 0/

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.h
(right):

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:83: int
options_;
Nit: This one probably should be initialized to NONE in the ctor, no?

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:90:
views::Link* manage_shortcut_;
nit: While these are easy to understand on their own I'd still document them
because people have a tendency to add new variables at the bottom and if they
see undocumented ones they are more likely to skip documenting theirs.

[Devlin, 2015-12-07 20:57:51]

Finnur, I've added a couple of screenshots to the bug.  Lemme know if you want
anything else I missed. :)

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:311: auto
add_content_view = [&layout](views::View* view) {
On 2015/12/07 16:07:45, sky wrote:
> On 2015/12/05 01:10:15, Devlin wrote:
> > On 2015/12/04 23:41:06, sky wrote:
> > > tricky
> > 
> > Good tricky or bad tricky? ;)
> 
> I say tricky as a normal function would work just fine here and not be as
> subtle.

I'm not sure I agree.  If we have it as a normal function, we have to pass in
the extra parameters of the column set id and the layout.  More importantly,
this is really only a viable "add view to layout" function for this specific
part - i.e., trying to use this to other views, where we might not want to
auto-skip columns, wouldn't be good.

I can pull it out into a separate function if you like, but I'd have a slight
preference to keep it as-is.

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/bubble_sync_promo_delegate.h (right):

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/bubble_sync_promo_delegate.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2015/12/07 18:10:02, Finnur wrote:
> nit: 2015

I never know what the right thing to do for moved, renamed files is.  But I
guess this one's probably small/different enough to warrant the update.  Done.

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:253:
MAIN_COLUMN_SET,
On 2015/12/07 18:10:02, Finnur wrote:
> nit: s/SET/SET = 0/

Done.

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.h
(right):

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:83: int
options_;
On 2015/12/07 18:10:02, Finnur wrote:
> Nit: This one probably should be initialized to NONE in the ctor, no?

Yep.  Done.

https://codereview.chromium.org/1503583002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:90:
views::Link* manage_shortcut_;
On 2015/12/07 18:10:02, Finnur wrote:
> nit: While these are easy to understand on their own I'd still document them
> because people have a tendency to add new variables at the bottom and if they
> see undocumented ones they are more likely to skip documenting theirs.

Fair enough. :) Done.

[sky, 2015-12-07 21:46:41]

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/1503583002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:311: auto
add_content_view = [&layout](views::View* view) {
On 2015/12/07 20:57:50, Devlin wrote:
> On 2015/12/07 16:07:45, sky wrote:
> > On 2015/12/05 01:10:15, Devlin wrote:
> > > On 2015/12/04 23:41:06, sky wrote:
> > > > tricky
> > > 
> > > Good tricky or bad tricky? ;)
> > 
> > I say tricky as a normal function would work just fine here and not be as
> > subtle.
> 
> I'm not sure I agree.  If we have it as a normal function, we have to pass in
> the extra parameters of the column set id and the layout.

I would argue you are passing in the layout too. You specify layout as part of
the capture set, which IMO is roughly the same as passing layout directly.

> More importantly,
> this is really only a viable "add view to layout" function for this specific
> part - i.e., trying to use this to other views, where we might not want to
> auto-skip columns, wouldn't be good.

I agree with you here.

> I can pull it out into a separate function if you like, but I'd have a slight
> preference to keep it as-is.

This is why I LGTMd your patch. It's mostly a matter of personal style.

[Finnur, 2015-12-07 23:39:56]

Last couple of questions (I think):

Are the UX designers ok with having two types of links in the middle dialog (one
with underline and the other without)?

It is also curious how it wraps before the word 'sign' in the 'sign in to
Chrome' sentence (as opposed to after it). No biggie though.

[Devlin, 2015-12-07 23:46:04]

On 2015/12/07 23:39:56, Finnur wrote:
> Last couple of questions (I think):
> 
> Are the UX designers ok with having two types of links in the middle dialog
(one
> with underline and the other without)?

Good eye.  I've asked on the bug.

> It is also curious how it wraps before the word 'sign' in the 'sign in to
> Chrome' sentence (as opposed to after it). No biggie though.

I'm mostly trusting the layout manager to do the right thing, there. :)

[Finnur, 2015-12-07 23:51:45]

Anyway, see what they say about the links. Probably an easy fix if they think
you need to change it. Otherwise LGTM.

[Devlin, 2015-12-09 00:42:23]

Underline removed; submitting.

[Devlin, 2015-12-09 00:42:29]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-12-09 00:42:29]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/1503583002/#ps80001
(title: "Latest master")

[commit-bot: I haz the power, 2015-12-09 00:46:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1503583002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1503583002/80001

[commit-bot: I haz the power, 2015-12-09 01:52:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-09 01:53:02]

Description was changed from

==========
[Extensions Views] Update the extension installed bubble's sync promo

Update the extension installed bubble's sync promo to match that of the
bookmark added bubble (and rename BookmarkSyncPromoView to
BubbleSyncPromoView).

Also, restructure the layout of the extension installed bubble to use a grid
layout to chop out a couple hundred LOC.

BUG=551695
==========

to

==========
[Extensions Views] Update the extension installed bubble's sync promo

Update the extension installed bubble's sync promo to match that of the
bookmark added bubble (and rename BookmarkSyncPromoView to
BubbleSyncPromoView).

Also, restructure the layout of the extension installed bubble to use a grid
layout to chop out a couple hundred LOC.

BUG=551695

Committed: https://crrev.com/305da4552528094c32c9bd76867a3c3e6aec8797
Cr-Commit-Position: refs/heads/master@{#363892}
==========

[commit-bot: I haz the power, 2015-12-09 01:53:03]

Patchset 5 (id:??) landed as
https://crrev.com/305da4552528094c32c9bd76867a3c3e6aec8797
Cr-Commit-Position: refs/heads/master@{#363892}