Componentize chrome/common/content_settings files.

Componentize chrome/common/content_settings files.

BUG=387077, 387079
R=bauerb@chromium.org, jochen@chromium.org, markusheintz@chromium.org
TBR=abarth@chromium.org
TEST=build succeeds / components_unittests

Committed: https://crrev.com/077089f9cce13eb689acddf183b3af83a2b9ec2d
Cr-Commit-Position: refs/heads/master@{#294430}

[Jun Mukai, 2014-09-04 16:46:34]

mukai@chromium.org changed reviewers:
+ markusheintz@chromium.org

[vabr (Chromium), 2014-09-04 20:02:50]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2014-09-04 20:02:51]

Thanks, Jun!
LGTM with 2 comments.

Vaclav

https://codereview.chromium.org/541813002/diff/40001/chrome/DEPS
File chrome/DEPS (right):

https://codereview.chromium.org/541813002/diff/40001/chrome/DEPS#newcode19
chrome/DEPS:19: "+components/content_settings",
Is this just a temporary? If so, should there be a TODO stating when it's OK to
remove? If it's not temporary, could this be in subdirectories, which need this
instead?

https://codereview.chromium.org/541813002/diff/40001/components/content_setti...
File components/content_settings/core/common/DEPS (right):

https://codereview.chromium.org/541813002/diff/40001/components/content_setti...
components/content_settings/core/common/DEPS:2:
"+components/content_settings/core/common",
Is this line really needed? Looking at the DEPS in other components (e.g.,
autofill), it looks like the "current directory" is not explicitly mentioned in
DEPS.

[Jun Mukai, 2014-09-04 23:50:40]

https://codereview.chromium.org/541813002/diff/40001/chrome/DEPS
File chrome/DEPS (right):

https://codereview.chromium.org/541813002/diff/40001/chrome/DEPS#newcode19
chrome/DEPS:19: "+components/content_settings",
On 2014/09/04 20:02:51, vabr (Chromium) wrote:
> Is this just a temporary? If so, should there be a TODO stating when it's OK
to
> remove? If it's not temporary, could this be in subdirectories, which need
this
> instead?

Good catch. This is note temporary, but indeed this must be
content_settings/core/common.

https://codereview.chromium.org/541813002/diff/40001/components/content_setti...
File components/content_settings/core/common/DEPS (right):

https://codereview.chromium.org/541813002/diff/40001/components/content_setti...
components/content_settings/core/common/DEPS:2:
"+components/content_settings/core/common",
On 2014/09/04 20:02:51, vabr (Chromium) wrote:
> Is this line really needed? Looking at the DEPS in other components (e.g.,
> autofill), it looks like the "current directory" is not explicitly mentioned
in
> DEPS.

removed.

[blundell, 2014-09-05 09:20:31]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2014-09-05 09:20:32]

lgtm

https://codereview.chromium.org/541813002/diff/60001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/541813002/diff/60001/components/components_te...
components/components_tests.gyp:500: ['include', '^crash/'],
please add content_settings here.

[Jun Mukai, 2014-09-05 17:37:26]

rebased.  PTAL

https://codereview.chromium.org/541813002/diff/60001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/541813002/diff/60001/components/components_te...
components/components_tests.gyp:500: ['include', '^crash/'],
On 2014/09/05 09:20:32, blundell wrote:
> please add content_settings here.

Done.

[Bernhard Bauer, 2014-09-08 09:09:49]

LGTM (rubberstamp-ish, as I'm not an expert on componentization, but everything
in here looks reasonable to me. I trust that you know what you're doing :) ).

[jochen (gone - plz use gerrit), 2014-09-08 11:11:12]

https://codereview.chromium.org/541813002/diff/100001/components/test/run_all...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/541813002/diff/100001/components/test/run_all...
components/test/run_all_unittests.cc:91: url::AddStandardScheme("chrome");
why does the content_settings_pattern* test need these? It shouldn't really know
about any of those schemes.

I'm slightly concerned about adding them for all tests, as components really
shouldn't know any of them. If any relies on them by accident, the tests won't
catch this.

[Jun Mukai, 2014-09-08 16:56:05]

https://codereview.chromium.org/541813002/diff/100001/components/test/run_all...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/541813002/diff/100001/components/test/run_all...
components/test/run_all_unittests.cc:91: url::AddStandardScheme("chrome");
On 2014/09/08 11:11:12, jochen wrote:
> why does the content_settings_pattern* test need these? It shouldn't really
know
> about any of those schemes.
> 
> I'm slightly concerned about adding them for all tests, as components really
> shouldn't know any of them. If any relies on them by accident, the tests won't
> catch this.

They appear as test expectations.
Indeed components files shouldn't rely on these schemes, but
content_settings_pattern relies on GURL, and GURL fails to parse URL like
"chrome-extension://peoadpeiejnhkmpaakpnompolbglelel/" as a valid URL (this URL
appears in content_settings_pattern_unittest.cc), therefore the test fails.


I don't think adding these schemes would make side effects on other files. 
Actually, these schemes are silently added at the browser initialization,
meaning any of unit_tests, browser_tests, interative_ui_tests contain them.

[Jun Mukai, 2014-09-08 19:49:31]

mukai@chromium.org changed reviewers:
+ abarth@chromium.org, sky@chromium.org

[Jun Mukai, 2014-09-08 19:49:55]

mukai@chromium.org changed reviewers:
- sky@chromium.org

[jochen (gone - plz use gerrit), 2014-09-09 07:53:59]

On 2014/09/08 at 16:56:05, mukai wrote:
>
https://codereview.chromium.org/541813002/diff/100001/components/test/run_all...
> File components/test/run_all_unittests.cc (right):
> 
>
https://codereview.chromium.org/541813002/diff/100001/components/test/run_all...
> components/test/run_all_unittests.cc:91: url::AddStandardScheme("chrome");
> On 2014/09/08 11:11:12, jochen wrote:
> > why does the content_settings_pattern* test need these? It shouldn't really
know
> > about any of those schemes.
> > 
> > I'm slightly concerned about adding them for all tests, as components really
> > shouldn't know any of them. If any relies on them by accident, the tests
won't
> > catch this.
> 
> They appear as test expectations.
> Indeed components files shouldn't rely on these schemes, but
content_settings_pattern relies on GURL, and GURL fails to parse URL like
"chrome-extension://peoadpeiejnhkmpaakpnompolbglelel/" as a valid URL (this URL
appears in content_settings_pattern_unittest.cc), therefore the test fails.
> 
> 
> I don't think adding these schemes would make side effects on other files. 
Actually, these schemes are silently added at the browser initialization,
meaning any of unit_tests, browser_tests, interative_ui_tests contain them.

but those are all in chrome/ where it's perfectly fine to have those schemes.

If the content_settings_pattern stuff relies on any of those schemes, they
should be registered with a similar API as for the chrome-extensions scheme

[Jun Mukai, 2014-09-09 16:21:06]

On 2014/09/09 07:53:59, jochen wrote:
> On 2014/09/08 at 16:56:05, mukai wrote:
> >
>
https://codereview.chromium.org/541813002/diff/100001/components/test/run_all...
> > File components/test/run_all_unittests.cc (right):
> > 
> >
>
https://codereview.chromium.org/541813002/diff/100001/components/test/run_all...
> > components/test/run_all_unittests.cc:91: url::AddStandardScheme("chrome");
> > On 2014/09/08 11:11:12, jochen wrote:
> > > why does the content_settings_pattern* test need these? It shouldn't
really
> know
> > > about any of those schemes.
> > > 
> > > I'm slightly concerned about adding them for all tests, as components
really
> > > shouldn't know any of them. If any relies on them by accident, the tests
> won't
> > > catch this.
> > 
> > They appear as test expectations.
> > Indeed components files shouldn't rely on these schemes, but
> content_settings_pattern relies on GURL, and GURL fails to parse URL like
> "chrome-extension://peoadpeiejnhkmpaakpnompolbglelel/" as a valid URL (this
URL
> appears in content_settings_pattern_unittest.cc), therefore the test fails.
> > 
> > 
> > I don't think adding these schemes would make side effects on other files. 
> Actually, these schemes are silently added at the browser initialization,
> meaning any of unit_tests, browser_tests, interative_ui_tests contain them.
> 
> but those are all in chrome/ where it's perfectly fine to have those schemes.
> 
> If the content_settings_pattern stuff relies on any of those schemes, they
> should be registered with a similar API as for the chrome-extensions scheme

We can't do the same way. I believe this code is the most similar way to
do this in //components.

These schemes are added from url_schemes.cc in content/common:
https://chromium.googlesource.com/chromium/src/+/master/content/common/url_sc...
And the chrome-specific schemes are provided from chrome_content_client.
chrome/test/base/chrome_unit_test_suite.cc is responsible to initialize
this.

//components cannot use //content code directly because iOS code
cannot depend on //content, and using //content code just for
adding scheme doesn't make much sense to me.
Apart from //content code, I believe the current one is the most similar
way to add the schemes.

[jochen (gone - plz use gerrit), 2014-09-10 07:55:56]

ok, lgtm

[Jun Mukai, 2014-09-10 22:44:41]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-09-10 22:51:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541813002/100001

[commit-bot: I haz the power, 2014-09-10 23:57:00]

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

[commit-bot: I haz the power, 2014-09-10 23:57:02]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54342)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59272)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jun Mukai, 2014-09-10 23:59:56]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-09-11 00:01:20]

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

[commit-bot: I haz the power, 2014-09-11 00:17:03]

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

[commit-bot: I haz the power, 2014-09-11 00:17:05]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54376)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59320)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Jun Mukai, 2014-09-11 00:22:43]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-09-11 00:24:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541813002/140001

[commit-bot: I haz the power, 2014-09-11 01:07:03]

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

[commit-bot: I haz the power, 2014-09-11 01:07:06]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jun Mukai, 2014-09-11 17:05:01]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:13:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541813002/160001

[commit-bot: I haz the power, 2014-09-11 18:42:44]

Committed patchset #9 (id:160001) as feefac3614eddb6ca2e53a10bb970b42ddd43a8d

[commit-bot: I haz the power, 2014-09-11 19:09:13]

Patchset 9 (id:??) landed as
https://crrev.com/077089f9cce13eb689acddf183b3af83a2b9ec2d
Cr-Commit-Position: refs/heads/master@{#294430}