Abstract //content-level dependencies out of InMemoryURLIndex

Abstract //content-level dependencies out of InMemoryURLIndex

It now takes a base::SequencedWorkerPool and a set of client schemes to
whitelist in its constructor.

BUG=371538
TBR=phajdan.jr

Committed: https://crrev.com/a6da40f8abbf44f8dab8db59a425b85c1db44376
Cr-Commit-Position: refs/heads/master@{#334799}

[blundell, 2015-06-15 13:51:56]

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

[Peter Kasting, 2015-06-15 21:28:51]

I feel like this change sort of papers over the issue, and instead of a hard
dependency, we now have a "conceptual dependency", which is really just as much
of a layering violation, except our tools won't catch it.

It feels like maybe instead of this sort of thing, if we can't solve the problem
by implementing the embedder-specific logic in embedder-side functions, maybe we
should have oracle-type functions where the embedder gives the content layer the
set of additional schemes to consider and the content layer can basically treat
that list as a black box, or something.

This is a bit hand-wavey, and maybe it's not possible in general, so LGTM if
there's no better way, but both for this code and the url_fixer.cc example you
pointed out, try and come up with something more conceptually solid if you can.

[blundell, 2015-06-16 11:38:03]

Patchset #2 (id:20001) has been deleted

[blundell, 2015-06-16 12:14:29]

Thanks for the pushback. In this case, it turns out on inspection to actually be
pretty easy to build the layer cake the right way (in the url fixer case it's
unfortunately not).

[blundell, 2015-06-16 12:14:54]

On 2015/06/16 12:14:29, blundell wrote:
> Thanks for the pushback. In this case, it turns out on inspection to actually
be
> pretty easy to build the layer cake the right way (in the url fixer case it's
> unfortunately not).

(To be clear, that was a "PTAL" :).

[Peter Kasting, 2015-06-16 22:18:07]

LGTM

https://codereview.chromium.org/1186103002/diff/60001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index.h (right):

https://codereview.chromium.org/1186103002/diff/60001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index.h:109: const
std::set<std::string>& client_schemes_to_whitelist);
I wonder if it makes sense to add a typedef for this set of strings.

https://codereview.chromium.org/1186103002/diff/60001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1186103002/diff/60001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:53: const char
kClientWhitelistedScheme[] = "chrome";
Nit: Let's use "xyz" here or something to make it obvious there aren't
dependencies on the actual chrome directories or concepts.

https://codereview.chromium.org/1186103002/diff/60001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/1186103002/diff/60001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:203:
chrome_schemes_to_whitelist.insert(content::kChromeUIScheme);
Is this actually important for some tests?

[blundell, 2015-06-17 07:55:44]

Thanks for all the prompt reviews!

https://codereview.chromium.org/1186103002/diff/60001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index.h (right):

https://codereview.chromium.org/1186103002/diff/60001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index.h:109: const
std::set<std::string>& client_schemes_to_whitelist);
On 2015/06/16 22:18:07, Peter Kasting wrote:
> I wonder if it makes sense to add a typedef for this set of strings.

Done.

https://codereview.chromium.org/1186103002/diff/60001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1186103002/diff/60001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:53: const char
kClientWhitelistedScheme[] = "chrome";
On 2015/06/16 22:18:07, Peter Kasting wrote:
> Nit: Let's use "xyz" here or something to make it obvious there aren't
> dependencies on the actual chrome directories or concepts.

Done.

https://codereview.chromium.org/1186103002/diff/60001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/1186103002/diff/60001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:203:
chrome_schemes_to_whitelist.insert(content::kChromeUIScheme);
On 2015/06/16 22:18:07, Peter Kasting wrote:
> Is this actually important for some tests?

Nope. Removed.

[blundell, 2015-06-17 08:05:54]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-06-17 08:05:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1186103002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-17 08:06:57]

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

[commit-bot: I haz the power, 2015-06-17 08:14:36]

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

[commit-bot: I haz the power, 2015-06-17 08:14:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[blundell, 2015-06-17 08:28:45]

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

[blundell, 2015-06-17 08:29:14]

TBR=phajdan.jr for //chrome/test/base/testing_profile.cc

[blundell, 2015-06-17 08:29:20]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2015-06-17 08:30:17]

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

[commit-bot: I haz the power, 2015-06-17 09:42:23]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-06-17 09:43:30]

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

[paulmeyer, 2015-06-17 17:23:18]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/1184923006/ by paulmeyer@chromium.org.

The reason for reverting is: Speculative revert to fix the Windows unit_test
failures..