Changed ChromeURLDataManager to a ProfileKeyedService and made a Factory for it.

Changed ChromeURLDataManager to a ProfileKeyedService and made a Factory for it.

BUG=121009
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133958

[Kyle Horimoto, 2012-04-23 22:33:12]

+yoz for a chrome/browser/extensions/ OWNER

[eroman, 2012-04-23 22:38:40]

lgtm for browser/net_internals/*

On Mon, Apr 23, 2012 at 3:33 PM, <khorimoto@chromium.org> wrote:

> +yoz for a chrome/browser/extensions/ OWNER
>
>
https://chromiumcodereview.**appspot.com/10196004/<https://chromiumcodereview...
>

[Kyle Horimoto, 2012-04-23 22:41:13]

+scottbyer

sky: Need review for chrome/, chrome/test, and ChromeURLDataManager
estade: Need review for chrome/browser/ui/webui
erg: Need review for chrome/browser/profiles
willchan: Need review for ProfileIOData use in Profile and its subclasses
scottbyer: Need review for chrome/browser/printing/cloud_print
yoz: Need review for chrome/browser/extensions

[Scott Byer, 2012-04-23 22:43:13]

cloud_print LGTM

[Yoyo Zhou, 2012-04-23 22:47:10]

extensions LGTM

[Elliot Glaysher, 2012-04-23 22:59:05]

lgtm/nits.

https://chromiumcodereview.appspot.com/10196004/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/about_ui.cc (right):

https://chromiumcodereview.appspot.com/10196004/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/about_ui.cc:1514: if (source)
{ } around multiline bodies.

[sky, 2012-04-23 23:40:57]

LGTM

[Kyle Horimoto, 2012-04-24 00:12:36]

https://chromiumcodereview.appspot.com/10196004/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/about_ui.cc (right):

https://chromiumcodereview.appspot.com/10196004/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/about_ui.cc:1514: if (source)
On 2012/04/23 22:59:05, Elliot Glaysher wrote:
> { } around multiline bodies.

Done.

[Evan Stade, 2012-04-24 19:02:43]

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_url_data_manager_factory.cc (right):

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_factory.cc:2: // Author:
khorimoto@google.com (Kyle Horimoto)
ditto

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_factory.cc:14:
GetInstance()->GetServiceForProfile(profile, true));
wrong indent (should be just 4 spaces)

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_url_data_manager_factory.h (right):

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_factory.h:1: // Copyright 2012
Google Inc. All Rights Reserved.
is there a new format for license headers I'm not aware of?

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_factory.h:33: virtual bool
ServiceHasOwnInstanceInIncognito() OVERRIDE;
DISALLOW_COPY_AND_ASSIGN(ChromeURLDataManagerFactory);

(I think...)

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media/media_internals_ui.cc (right):

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media/media_internals_ui.cc:48:
AddDataSource(CreateMediaInternalsHTMLSource());
having just read through about 50 files where this exact pattern is used, I
would suggest creating a utility function in ChromeURLDataManager so you can
just do:

  ChromeURLDataManager::AddDataSource(
      profile, CreateMediaInternalsHTMLSource());

or in many cases, like this one,

  ChromeURLDataManager::AddDataSource(
      Profile::FromWebUI(web_ui), CreateMediaInternalsHTMLSource());

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/pro...
File chrome/browser/ui/webui/profiler_ui.cc (right):

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/pro...
chrome/browser/ui/webui/profiler_ui.cc:158:
ChromeURLDataManagerFactory::GetForProfile(profile)
-> goes at end of line

[Kyle Horimoto, 2012-04-24 21:09:59]

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_url_data_manager_factory.cc (right):

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_factory.cc:2: // Author:
khorimoto@google.com (Kyle Horimoto)
On 2012/04/24 19:02:43, Evan Stade wrote:
> ditto

Done.

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_factory.cc:14:
GetInstance()->GetServiceForProfile(profile, true));
On 2012/04/24 19:02:43, Evan Stade wrote:
> wrong indent (should be just 4 spaces)

Done.

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_url_data_manager_factory.h (right):

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_factory.h:1: // Copyright 2012
Google Inc. All Rights Reserved.
On 2012/04/24 19:02:43, Evan Stade wrote:
> is there a new format for license headers I'm not aware of?

Oops, this was just the default license header that Eclipse gave me, so I
assumed it was the correct one. Fixed now.

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_factory.h:33: virtual bool
ServiceHasOwnInstanceInIncognito() OVERRIDE;
On 2012/04/24 19:02:43, Evan Stade wrote:
> DISALLOW_COPY_AND_ASSIGN(ChromeURLDataManagerFactory);
> 
> (I think...)

Done. (And you're right!)

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media/media_internals_ui.cc (right):

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media/media_internals_ui.cc:48:
AddDataSource(CreateMediaInternalsHTMLSource());
On 2012/04/24 19:02:43, Evan Stade wrote:
> having just read through about 50 files where this exact pattern is used, I
> would suggest creating a utility function in ChromeURLDataManager so you can
> just do:
> 
>   ChromeURLDataManager::AddDataSource(
>       profile, CreateMediaInternalsHTMLSource());
> 
> or in many cases, like this one,
> 
>   ChromeURLDataManager::AddDataSource(
>       Profile::FromWebUI(web_ui), CreateMediaInternalsHTMLSource());

Done on this one instance. Waiting on positive review before applying it to
everything else.

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/pro...
File chrome/browser/ui/webui/profiler_ui.cc (right):

http://codereview.chromium.org/10196004/diff/8001/chrome/browser/ui/webui/pro...
chrome/browser/ui/webui/profiler_ui.cc:158:
ChromeURLDataManagerFactory::GetForProfile(profile)
On 2012/04/24 19:02:43, Evan Stade wrote:
> -> goes at end of line

Done.

[Evan Stade, 2012-04-24 21:22:13]

thumbs up

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager.cc:109: void
ChromeURLDataManager::AddDataSource(Profile *profile, DataSource* source) {
star on the left

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager.h (right):

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager.h:167: static void
AddDataSource(Profile *profile, DataSource* source);
star on the left

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/me...
File chrome/browser/ui/webui/media/media_internals_ui.cc (right):

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/me...
chrome/browser/ui/webui/media/media_internals_ui.cc:10: #include
"chrome/browser/ui/webui/chrome_url_data_manager_factory.h"
this change should now be reverted

[willchan no longer on Chromium, 2012-04-24 21:35:46]

lgtm

[Kyle Horimoto, 2012-04-24 22:17:49]

estade: Does this look okay now?

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager.cc:109: void
ChromeURLDataManager::AddDataSource(Profile *profile, DataSource* source) {
On 2012/04/24 21:22:14, Evan Stade wrote:
> star on the left

Done.

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager.h (right):

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager.h:167: static void
AddDataSource(Profile *profile, DataSource* source);
On 2012/04/24 21:22:14, Evan Stade wrote:
> star on the left

Done.

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/me...
File chrome/browser/ui/webui/media/media_internals_ui.cc (right):

http://codereview.chromium.org/10196004/diff/14001/chrome/browser/ui/webui/me...
chrome/browser/ui/webui/media/media_internals_ui.cc:10: #include
"chrome/browser/ui/webui/chrome_url_data_manager_factory.h"
On 2012/04/24 21:22:14, Evan Stade wrote:
> this change should now be reverted

Done.

[Evan Stade, 2012-04-24 23:31:14]

lgtm

[Evan Stade, 2012-04-24 23:32:01]

http://codereview.chromium.org/10196004/diff/17001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/10196004/diff/17001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:105:
ChromeURLDataManagerFactory::GetForProfile(this)->AddDataSource(icon_source);
(not sure why the ones outside webui weren't also changed)

[Kyle Horimoto, 2012-04-25 00:30:39]

http://codereview.chromium.org/10196004/diff/17001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/10196004/diff/17001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:105:
ChromeURLDataManagerFactory::GetForProfile(this)->AddDataSource(icon_source);
On 2012/04/24 23:32:01, Evan Stade wrote:
> (not sure why the ones outside webui weren't also changed)

Done. Oops, did it in every file but this one.

[commit-bot: I haz the power, 2012-04-25 00:31:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10196004/21001

[commit-bot: I haz the power, 2012-04-25 00:31:25]

Can't apply patch for file chrome/browser/ui/webui/options2/options_ui2.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/options2/options_ui2.cc
Hunk #1 succeeded at 22 (offset 1 line).
Hunk #2 FAILED at 299.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/options2/options_ui2.cc.rej

[commit-bot: I haz the power, 2012-04-25 00:35:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10196004/21001

[commit-bot: I haz the power, 2012-04-25 00:36:15]

Can't apply patch for file chrome/browser/ui/webui/options2/options_ui2.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/options2/options_ui2.cc
Hunk #1 succeeded at 22 (offset 1 line).
Hunk #2 FAILED at 299.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/options2/options_ui2.cc.rej

[commit-bot: I haz the power, 2012-04-25 17:37:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10196004/21001

[commit-bot: I haz the power, 2012-04-25 17:38:55]

Can't apply patch for file chrome/browser/ui/webui/options2/options_ui2.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/options2/options_ui2.cc
Hunk #1 succeeded at 22 (offset 1 line).
Hunk #2 FAILED at 299.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/options2/options_ui2.cc.rej

[commit-bot: I haz the power, 2012-04-25 17:42:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10196004/21001

[commit-bot: I haz the power, 2012-04-25 17:43:00]

Can't apply patch for file chrome/browser/ui/webui/options2/options_ui2.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/options2/options_ui2.cc
Hunk #1 succeeded at 22 (offset 1 line).
Hunk #2 FAILED at 299.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/options2/options_ui2.cc.rej

[commit-bot: I haz the power, 2012-04-25 17:46:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10196004/25001

[commit-bot: I haz the power, 2012-04-25 18:02:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10196004/11004

[commit-bot: I haz the power, 2012-04-25 19:40:48]

Change committed as 133958