Move Infobars core files to the Infobars component

Move Infobars core files to the Infobars component

This CL sets up the directory structure for the Infobars component and the
infobars_core target. It also does a simple code move to the compoonent.

This CL does not move ConfirmInfoBarDelegate because it still has a
dependency on chrome/ (for strings).

BUG=354379
TBR=sky, reed

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

[droger, 2014-04-16 15:12:39]

pkasting: please do an overall review
jochen: please review the creation of the Infobar component in particular

[Peter Kasting, 2014-04-16 20:18:00]

https://codereview.chromium.org/240193003/diff/20001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (left):

https://codereview.chromium.org/240193003/diff/20001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:306: const char kDisableInfoBars[]             
 = "disable-infobars";
How do you know removing this is safe?

https://codereview.chromium.org/240193003/diff/20001/components/infobars/DEPS
File components/infobars/DEPS (right):

https://codereview.chromium.org/240193003/diff/20001/components/infobars/DEPS...
components/infobars/DEPS:3: # can be shared on iOS.
Nit: How about:

The infobars component may not use content/.  This makes it possible to use on
iOS.

[droger, 2014-04-17 08:09:51]

https://codereview.chromium.org/240193003/diff/20001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (left):

https://codereview.chromium.org/240193003/diff/20001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:306: const char kDisableInfoBars[]             
 = "disable-infobars";
On 2014/04/16 20:18:01, Peter Kasting wrote:
> How do you know removing this is safe?

Why wouldn't it be safe?
Note that this was not completely removed, but moved to
components/infobars/core/infobars_switches.cc.

https://codereview.chromium.org/240193003/diff/20001/components/infobars/DEPS
File components/infobars/DEPS (right):

https://codereview.chromium.org/240193003/diff/20001/components/infobars/DEPS...
components/infobars/DEPS:3: # can be shared on iOS.
On 2014/04/16 20:18:01, Peter Kasting wrote:
> Nit: How about:
> 
> The infobars component may not use content/.  This makes it possible to use on
> iOS.

Actually my comment was wrong, as infobars will use content, but just not in
infobars/core/:
InfoBarService will go to components/infobars/content/ and will use content.

I updated the text to use the same language as components/translate/DEPS and
components/autofill/DEPS.

[jochen (gone - plz use gerrit), 2014-04-17 08:22:27]

https://codereview.chromium.org/240193003/diff/30001/components/infobars/OWNERS
File components/infobars/OWNERS (left):

https://codereview.chromium.org/240193003/diff/30001/components/infobars/OWNE...
components/infobars/OWNERS:1: pkasting@chromium.org
is there nobody else that could do reviews?

https://codereview.chromium.org/240193003/diff/30001/components/infobars/core...
File components/infobars/core/infobar.h (right):

https://codereview.chromium.org/240193003/diff/30001/components/infobars/core...
components/infobars/core/infobar.h:17: class InfoBarContainer;
can you move the symbols into a namespace?

[droger, 2014-04-17 12:30:07]

https://codereview.chromium.org/240193003/diff/30001/components/infobars/OWNERS
File components/infobars/OWNERS (left):

https://codereview.chromium.org/240193003/diff/30001/components/infobars/OWNE...
components/infobars/OWNERS:1: pkasting@chromium.org
On 2014/04/17 08:22:28, jochen wrote:
> is there nobody else that could do reviews?

I kept the same OWNERS file as in chrome/browser/infobar.

https://codereview.chromium.org/240193003/diff/30001/components/infobars/core...
File components/infobars/core/infobar.h (right):

https://codereview.chromium.org/240193003/diff/30001/components/infobars/core...
components/infobars/core/infobar.h:17: class InfoBarContainer;
On 2014/04/17 08:22:28, jochen wrote:
> can you move the symbols into a namespace?

I was originally planning to do this in a follow up, but done.
Note that I put a couple "using" directives in infobar_service.cc and
confirm_infobar_delegate.cc because I plan to move them to the component in the
short term.

[jochen (gone - plz use gerrit), 2014-04-17 12:33:15]

components/ lgtm

[Peter Kasting, 2014-04-17 20:20:51]

I wish you had not made the namespace change in this CL, at least without
discussing it some first.  It may not be wrong, but I wanted to have some
clarity on the following questions before proceeding:

* I'm not convinced of the desirability of a namespace.  Chrome code largely
avoids these.  Why do we need one here?
* If we are going to use a namespace, it seems like we should try and remove
redundancy in the typenames, e.g. infobars::InfoBarXXX -> infobars::XXX.  Unless
there's a reason not to do this?

The rest of the change LGTM.

[Peter Kasting, 2014-04-17 20:23:59]

(Forgot to send this)

https://codereview.chromium.org/240193003/diff/20001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (left):

https://codereview.chromium.org/240193003/diff/20001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:306: const char kDisableInfoBars[]             
 = "disable-infobars";
On 2014/04/17 08:09:53, droger wrote:
> On 2014/04/16 20:18:01, Peter Kasting wrote:
> > How do you know removing this is safe?
> 
> Note that this was not completely removed, but moved to
> components/infobars/core/infobars_switches.cc.

Ah.  That's fine then.  I thought you were removing support for the flag
entirely and I didn't know if it was still in use by someone.

[jochen (gone - plz use gerrit), 2014-04-17 20:24:36]

On 2014/04/17 20:20:51, Peter Kasting wrote:
> I wish you had not made the namespace change in this CL, at least without
> discussing it some first.  It may not be wrong, but I wanted to have some
> clarity on the following questions before proceeding:
> 
> * I'm not convinced of the desirability of a namespace.  Chrome code largely
> avoids these.  Why do we need one here?
> * If we are going to use a namespace, it seems like we should try and remove
> redundancy in the typenames, e.g. infobars::InfoBarXXX -> infobars::XXX. 
Unless
> there's a reason not to do this?
> 
> The rest of the change LGTM.

We usually put all components into their own namespace.

Making the names less redundant sounds like a good thing, but that's up to you
as owner of the code

[Ilya Sherman, 2014-04-17 20:26:02]

On 2014/04/17 20:20:51, Peter Kasting wrote:
> * I'm not convinced of the desirability of a namespace.  Chrome code largely
> avoids these.  Why do we need one here?

Components use namespaces, same as //content does.

> * If we are going to use a namespace, it seems like we should try and remove
> redundancy in the typenames, e.g. infobars::InfoBarXXX -> infobars::XXX. 
Unless
> there's a reason not to do this?

+1 to removing redundancy, as long as "XXX" is clear on its own.  I'm not sure
that "infobars::Delegate" is clear enough on its own, though, as an example.

[droger, 2014-04-17 20:43:59]

pasting: I'm ok to do a renaming if needed.
Can I commit this CL as is and consider the renaming later or do you prefer the
renaming to be part of this CL?

[Peter Kasting, 2014-04-17 20:45:56]

On 2014/04/17 20:43:59, droger wrote:
> pasting: I'm ok to do a renaming if needed.
> Can I commit this CL as is and consider the renaming later or do you prefer
the
> renaming to be part of this CL?

You can go ahead.  Let's discuss what the renaming should actually be before you
go to all the effort.

[droger, 2014-04-17 20:46:15]

On 2014/04/17 20:43:59, droger wrote:
> pasting: [...]

Oops I meant 'pkasting', but it was wrongly "auto-corrected".

[droger, 2014-04-17 20:48:37]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-04-17 20:49:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/240193003/70001

[commit-bot: I haz the power, 2014-04-17 20:49:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-17 20:49:53]

Failed to apply patch for chrome/browser/DEPS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/DEPS
  Hunk #1 FAILED at 18.
  1 out of 1 hunk FAILED -- saving rejects to file chrome/browser/DEPS.rej

Patch:       chrome/browser/DEPS
Index: chrome/browser/DEPS
diff --git a/chrome/browser/DEPS b/chrome/browser/DEPS
index
f2bc86d917aed7f8be4743a3cc4e26af945a2f80..4834b08f16acbe6894287678e1c67d859f4aec1d
100644
--- a/chrome/browser/DEPS
+++ b/chrome/browser/DEPS
@@ -18,6 +18,7 @@ include_rules = [
   "+components/data_reduction_proxy",
   "+components/dom_distiller",
   "+components/domain_reliability",
+  "+components/infobars",
   "+components/keyed_service",
   "+components/language_usage_metrics",
   "+components/nacl/browser",

[droger, 2014-04-18 09:08:36]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-04-18 09:08:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/240193003/90001

[commit-bot: I haz the power, 2014-04-18 09:34:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 09:34:47]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[droger, 2014-04-18 09:47:44]

TBR reed for dependency of compontents/infobars on third_party/skia/
TBR sky for dependency of compontents/infobars on ui/

[droger, 2014-04-18 09:47:58]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-04-18 09:48:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/240193003/90001

[commit-bot: I haz the power, 2014-04-18 10:23:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 10:23:29]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[droger, 2014-04-18 11:12:49]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-04-18 11:12:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/240193003/90001

[commit-bot: I haz the power, 2014-04-18 11:46:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 11:46:13]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[droger, 2014-04-18 12:02:20]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-04-18 12:02:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/240193003/110001

[commit-bot: I haz the power, 2014-04-18 15:09:50]

Change committed as 264792

[blundell, 2014-04-28 09:05:20]

On 2014/04/17 20:24:36, jochen wrote:
> On 2014/04/17 20:20:51, Peter Kasting wrote:
> > I wish you had not made the namespace change in this CL, at least without
> > discussing it some first.  It may not be wrong, but I wanted to have some
> > clarity on the following questions before proceeding:
> > 
> > * I'm not convinced of the desirability of a namespace.  Chrome code largely
> > avoids these.  Why do we need one here?
> > * If we are going to use a namespace, it seems like we should try and remove
> > redundancy in the typenames, e.g. infobars::InfoBarXXX -> infobars::XXX. 
> Unless
> > there's a reason not to do this?
> > 
> > The rest of the change LGTM.
> 
> We usually put all components into their own namespace.
> 
> Making the names less redundant sounds like a good thing, but that's up to you
> as owner of the code

Yes, all components get their own namespace, in accordance with the Chromium
convention that modules that are intended to be consumed by embedders get a
namespace corresponding to the top-level module name (e.g., ::base). This is
specified in //components/README.

Wrt the redundancy in names, I see tradeoffs each way. Most components have e.g.
"*Manager", "*Driver", "*Client". These can either be signin::SigninManager,
autofill::AutofillManager, infobars::InfoBarManager, or signin::Manager,
autofill::Manager, infobars::Manager. I don't feel strongly about which way is
preferable; to date, we've done the former, and if we were to do a change, I
would like it to be a consistent decision across components (i.e., I *don't*
think the decision should be made in different ways for different components).