Refactor out duplicate code from GTK/Views extension installed bubble.

Refactor out duplicate code from GTK/Views extension installed bubble.

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

[Yoyo Zhou, 2013-08-29 02:07:39]

Scott: as you suggested, there was a lot to be refactored out. Please review.
I'm least sure about the DEPS change; is this the right place for cross-platform
extensions browser UI code?
Mike: for the DEPS change.

[sky, 2013-08-29 15:39:51]

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.cc (right):

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.cc:62:
ExtensionInstalledBubble::~ExtensionInstalledBubble() {}
nit: make order match header.

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.h (left):

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:73: SkBitmap icon_;
const

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:75: BubbleType type_;
const

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.h (right):

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:35: enum BubbleType {
nit: enums before constructor.

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:61: virtual void
ShowInternal() = 0;
This pattern is leading to multiple inheritance of non pure virtual types,
something against the style guide. Maybe this class should be concrete but have
an ExtensionInstalledBubbleView or something that is pure.

[sky, 2013-08-29 15:40:13]

Also, I think the DEPS change is fine. There are classes in c/b/u/v already
including c/b/u/e

[miket_OOO, 2013-08-29 16:34:08]

On 2013/08/29 15:40:13, sky wrote:
> Also, I think the DEPS change is fine. There are classes in c/b/u/v already
> including c/b/u/e

LGTM on DEPS.

[Yoyo Zhou, 2013-08-29 21:44:45]

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.cc (right):

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.cc:62:
ExtensionInstalledBubble::~ExtensionInstalledBubble() {}
On 2013/08/29 15:39:51, sky wrote:
> nit: make order match header.

Done.

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.h (left):

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:73: SkBitmap icon_;
On 2013/08/29 15:39:51, sky wrote:
> const

Done.

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:75: BubbleType type_;
On 2013/08/29 15:39:51, sky wrote:
> const

I think the type determination is too complex to put in the initializer list.

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.h (right):

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:35: enum BubbleType {
On 2013/08/29 15:39:51, sky wrote:
> nit: enums before constructor.

Done.

https://codereview.chromium.org/23461013/diff/3001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:61: virtual void
ShowInternal() = 0;
On 2013/08/29 15:39:51, sky wrote:
> This pattern is leading to multiple inheritance of non pure virtual types,
> something against the style guide. Maybe this class should be concrete but
have
> an ExtensionInstalledBubbleView or something that is pure.

I added a View delegate. This added a bit of complexity.

[tfarina, 2013-08-29 22:19:54]

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.h (right):

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:41: class View {
why this is an inner class? can you move it into its own header file?

can it be Delegate instead of View?

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/views/DEPS
File chrome/browser/ui/views/DEPS (right):

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/views/DE...
chrome/browser/ui/views/DEPS:2: "+chrome/browser/ui/extensions",
why did you need this? checkdeps.py complained?

[Yoyo Zhou, 2013-09-03 19:34:36]

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.h (right):

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:41: class View {
On 2013/08/29 22:19:54, tfarina wrote:
> why this is an inner class? can you move it into its own header file?

There's no benefit, since everyone who implements the delegate needs this header
file.

> can it be Delegate instead of View?

Sure, renamed.

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/views/DEPS
File chrome/browser/ui/views/DEPS (right):

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/views/DE...
chrome/browser/ui/views/DEPS:2: "+chrome/browser/ui/extensions",
On 2013/08/29 22:19:54, tfarina wrote:
> why did you need this? checkdeps.py complained?

It did, but it no longer seems necessary.

[tfarina, 2013-09-03 19:49:13]

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.h (right):

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:41: class View {
On 2013/09/03 19:34:36, Yoyo Zhou wrote:
> On 2013/08/29 22:19:54, tfarina wrote:
> > why this is an inner class? can you move it into its own header file?
> 
> There's no benefit, since everyone who implements the delegate needs this
header
> file.
> 
they need because they are referring to BubbleType?

If it's that, ok. If it is not, then forward declare ExtensionInstalledBubble in
those headers and move the delegate into its own header file.

> > can it be Delegate instead of View?
> 
> Sure, renamed.

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:42: public:
virtual destructor?

[tfarina, 2013-09-03 19:51:27]

https://codereview.chromium.org/23461013/diff/18001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.h
(right):

https://codereview.chromium.org/23461013/diff/18001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:36: Browser
*browser,
nit: Browser*

https://codereview.chromium.org/23461013/diff/18001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:50: //
views::WidgetDelegate
views::WidgetDelegate:

https://codereview.chromium.org/23461013/diff/18001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:56:
ExtensionInstalledBubble bubble_;
Scott prefers like this, on the stack. But I'd use scoped_ptr<...> and you know
the rest... ;)

[Yoyo Zhou, 2013-09-03 22:15:54]

sky: ping

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/extension_installed_bubble.h (right):

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:41: class View {
On 2013/09/03 19:49:13, tfarina wrote:
> On 2013/09/03 19:34:36, Yoyo Zhou wrote:
> > On 2013/08/29 22:19:54, tfarina wrote:
> > > why this is an inner class? can you move it into its own header file?
> > 
> > There's no benefit, since everyone who implements the delegate needs this
> header
> > file.
> > 
> they need because they are referring to BubbleType?
> 
> If it's that, ok. If it is not, then forward declare ExtensionInstalledBubble
in
> those headers and move the delegate into its own header file.

As per the other comment, if I declare an ExtensionInstalledBubble on the stack
instead of a scoped_ptr...

https://codereview.chromium.org/23461013/diff/9001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/extension_installed_bubble.h:42: public:
On 2013/09/03 19:49:13, tfarina wrote:
> virtual destructor?

Done.

https://codereview.chromium.org/23461013/diff/18001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.h
(right):

https://codereview.chromium.org/23461013/diff/18001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:36: Browser
*browser,
On 2013/09/03 19:51:27, tfarina wrote:
> nit: Browser*

Done.

https://codereview.chromium.org/23461013/diff/18001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:50: //
views::WidgetDelegate
On 2013/09/03 19:51:27, tfarina wrote:
> views::WidgetDelegate:

Done.

https://codereview.chromium.org/23461013/diff/18001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:56:
ExtensionInstalledBubble bubble_;
On 2013/09/03 19:51:27, tfarina wrote:
> Scott prefers like this, on the stack. But I'd use scoped_ptr<...> and you
know
> the rest... ;)

In that case I'll leave it as is.

[sky, 2013-09-04 00:04:06]

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/extension_installed_bubble.cc (right):

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.cc:89: if (type ==
chrome::NOTIFICATION_EXTENSION_LOADED) {
nit: IMO you've got enough ifs to warrant a switch here.

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/extension_installed_bubble.h (right):

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:50: virtual void
OnBrowserClosing() = 0;
Do you even need this method? Should it instead be replaced with 'delete
delegate_'?

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:60: const
extensions::Extension* extension() { return extension_; }
const extensions::Extension* extension() const

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:62: const SkBitmap&
icon() { return icon_; }
const SkBitmap& icon() const

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:63: BubbleType type()
{ return type_; }
BubbleType type() const

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:69: void
MaybeShowLater();
AFAICT this is an implementation detail and only public so that the Delegate can
invoke it. Could you instead make Delegate::ShowInternal() return a status so
that MaybeShowLater doesn't need to be public. IMO doing this would simplify
interaction between the two classes too.

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:86: const
extensions::Extension* extension_;
nit: document this is NULL when deleted.

[Yoyo Zhou, 2013-09-05 00:12:21]

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/extension_installed_bubble.cc (right):

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.cc:89: if (type ==
chrome::NOTIFICATION_EXTENSION_LOADED) {
On 2013/09/04 00:04:06, sky wrote:
> nit: IMO you've got enough ifs to warrant a switch here.

Done.

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/extension_installed_bubble.h (right):

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:50: virtual void
OnBrowserClosing() = 0;
On 2013/09/04 00:04:06, sky wrote:
> Do you even need this method? Should it instead be replaced with 'delete
> delegate_'?

That's better.

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:60: const
extensions::Extension* extension() { return extension_; }
On 2013/09/04 00:04:06, sky wrote:
> const extensions::Extension* extension() const

Done.

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:62: const SkBitmap&
icon() { return icon_; }
On 2013/09/04 00:04:06, sky wrote:
> const SkBitmap& icon() const

Done.

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:63: BubbleType type()
{ return type_; }
On 2013/09/04 00:04:06, sky wrote:
> BubbleType type() const

Done.

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:69: void
MaybeShowLater();
On 2013/09/04 00:04:06, sky wrote:
> AFAICT this is an implementation detail and only public so that the Delegate
can
> invoke it. Could you instead make Delegate::ShowInternal() return a status so
> that MaybeShowLater doesn't need to be public. IMO doing this would simplify
> interaction between the two classes too.

Done.

https://codereview.chromium.org/23461013/diff/24001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_installed_bubble.h:86: const
extensions::Extension* extension_;
On 2013/09/04 00:04:06, sky wrote:
> nit: document this is NULL when deleted.

Done.

[sky, 2013-09-05 15:43:35]

Nice, LGTM

[commit-bot: I haz the power, 2013-09-05 15:58:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23461013/9002

[commit-bot: I haz the power, 2013-09-05 16:36:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
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-09-05 18:42:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23461013/65001

[commit-bot: I haz the power, 2013-09-05 22:17:44]

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

[commit-bot: I haz the power, 2013-09-05 23:02:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23461013/80001

[commit-bot: I haz the power, 2013-09-06 05:59:11]

Change committed as 221592