[Extensions] Make extension message bubble factory platform-abstract

[Extensions] Make extension message bubble factory platform-abstract

We want to show extension message bubbles on multiple platforms;
the first step towards this is to make the bubble factory
platform-abstract. Do so, and as an added bonus, shave off
200 lines of code.

BUG=474092
Committed: https://crrev.com/a9a17da0de2260f6cb1a10b542a6d3a2b5fbf504
Cr-Commit-Position: refs/heads/master@{#324715}

[Devlin, 2015-04-09 19:53:51]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-04-09 19:53:57]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-04-10 00:21:27]

Patchset #1 (id:40001) has been deleted

[Devlin, 2015-04-10 00:28:59]

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

[Devlin, 2015-04-10 00:29:00]

Hey Finnur - I think this one's ready.  My plan for this (the bug) is pretty
much two stages; first abstract out the show mechanism, then implement it on mac
(the latter is done, but obviously doing this one first).

Take a look when you can. :)

[Devlin, 2015-04-10 00:30:13]

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/OWNERS (right):

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/OWNERS:6: per-file
extension*=rdevlin.cronin@chromium.org
Also lemme know if you're okay with this - after a bit of thought, it seems more
appropriate (to grasp this, the install ui, etc).  I'll run it by Scott if you
think it's okay.

[Finnur, 2015-04-10 11:24:21]

LGTM

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.cc (right):

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:166: return
true;
The NTP override bubble delegate returns |false| from this function, but this
one |true|. Why the difference?

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/OWNERS (right):

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/OWNERS:6: per-file
extension*=rdevlin.cronin@chromium.org
The name for this directory is so general that I worry all kinds of crap is
going to be added to it that we probably shouldn't be owners of (even with the
limitation of the extension_ prefix, which is pretty general). Is there a
pattern to this that make sense to exploit (e.g. the ones related to install,
put them in a subdirectory 'install' so we don't have to take ownership of
future random stuff)? :)

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:254: // and done so if one
exists.
nit: This comment is very confusing. Especially the "and done so" part. :)
Can you reword it?

[Devlin, 2015-04-10 15:55:20]

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.cc (right):

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:166: return
true;
On 2015/04/10 11:24:21, Finnur wrote:
> The NTP override bubble delegate returns |false| from this function, but this
> one |true|. Why the difference?

Just to keep with current behavior.  Right now, we don't highlight the
extensions for the NTP override bubble, but do for the proxy override behavior. 
I figured it best to try and have no behavior changes as a result of this patch,
just so it's more obvious if it breaks stuff. :)

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/OWNERS (right):

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/OWNERS:6: per-file
extension*=rdevlin.cronin@chromium.org
On 2015/04/10 11:24:21, Finnur wrote:
> The name for this directory is so general that I worry all kinds of crap is
> going to be added to it that we probably shouldn't be owners of (even with the
> limitation of the extension_ prefix, which is pretty general). Is there a
> pattern to this that make sense to exploit (e.g. the ones related to install,
> put them in a subdirectory 'install' so we don't have to take ownership of
> future random stuff)? :)

I think we try to avoid having really fine-grained directories (for better or
worse - I'd probably prefer it, but I see the advantage both ways, and best to
stick to the norm), so I think having a directory for "install" and a directory
for "bubble" and a directory for "extension_action" is probably wrong.  But
you're also definitely right that we don't want to accidentally end up owning
"random" stuff. :)

So, in lieu of that, I'd be inclined to say that a few extra lines in OWNERS
files is okay, and just add in explicit rules - it's still readable, and more
accurate overall. :)

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1078543002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:254: // and done so if one
exists.
On 2015/04/10 11:24:21, Finnur wrote:
> nit: This comment is very confusing. Especially the "and done so" part. :)
> Can you reword it?

Done.

[Devlin, 2015-04-10 16:25:35]

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

[Devlin, 2015-04-10 16:25:36]

Scott, mind taking a quick look at toolbar_view.*, and confirming the OWNERS
change is okay?

[sky, 2015-04-10 19:24:39]

I can't seem to view side-by-side diffs. Could you try reuploading?

[Devlin, 2015-04-10 19:31:16]

On 2015/04/10 19:24:39, sky wrote:
> I can't seem to view side-by-side diffs. Could you try reuploading?

Re-uploaded, but it looks like it has the same error (and can confirm it's also
not just this issue; looks like Rietveld hit a bug :()

It looks like the side-by-side diffs from patch set 2 still work - since the
only diff between 3 and 2 is a ChromeOS fix in
extension_message_bubble_controller_unittest.cc, would you mind reviewing the
files in PS 2 in the meantime?

[sky, 2015-04-10 20:01:18]

LGTM

[Devlin, 2015-04-10 21:17:09]

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

[Devlin, 2015-04-10 21:17:09]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/1078543002/#ps120001
(title: "Re-Upload")

[commit-bot: I haz the power, 2015-04-10 21:19:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1078543002/120001

[commit-bot: I haz the power, 2015-04-10 23:58:00]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-04-10 23:59:12]

Patchset 4 (id:??) landed as
https://crrev.com/a9a17da0de2260f6cb1a10b542a6d3a2b5fbf504
Cr-Commit-Position: refs/heads/master@{#324715}

[Devlin, 2015-04-13 17:40:44]

A revert of this CL (patchset #4 id:120001) has been created in
https://codereview.chromium.org/1081113002/ by rdevlin.cronin@chromium.org.

The reason for reverting is: Looks like this caused crbug.com/476426. Reverting
to investigate..