Make ShortcutsBackend a ProfileKeyedService.

Make ShortcutsBackend a ProfileKeyedService.

Removed all GetShortcutsBackend. Also minor enhancement of ShortcutsBackend unit tests to test shortcut update notifications.

BUG=112558
TEST=All tests pass. Enhanced one test.
TBR=sky
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=145378

[mrossetti, 2012-06-29 19:06:13]

Rachel, if you wouldn't mind taking a quick look at this to see if I've missed
anything serious I'd be grateful. Then after I clear up any issues you raise
I'll toss it over to Miranda for owner approval.

[rpetterson, 2012-06-29 20:57:06]

Will you be removing the instances of GetShortcutsBackend() in a later CL?

Also, I have owners approval for profiles if that's all you were gonna bug
Miranda about.

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete/shortcuts_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/shortcuts_provider_unittest.cc:180: backend_ =
ShortcutsBackendFactory::GetForProfile(&profile_);
Since you're using SetTestingFactoryAndUse, you can do these two lines as one
line because SetTestingFactoryAndUse will return the service it just created.

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/history...
File chrome/browser/history/shortcuts_backend_factory.h (right):

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/history...
chrome/browser/history/shortcuts_backend_factory.h:33:
GetForProfileForTesting(Profile* profile);
I think a better name would be BuildProfileForTesting since it mimics the
BuildServiceInstanceFor function rather than the GetForProfile function.

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/history...
chrome/browser/history/shortcuts_backend_factory.h:38:
GetForProfileNoDatabaseForTesting(Profile* profile);
Same as above, BuildProfileNoDatabaseForTesting.

[mrossetti, 2012-07-02 18:31:39]

Addressed all issues. Removed all GetShortcutsBackend's.

Tossing it back to you rpetterson for final review.

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete/shortcuts_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/shortcuts_provider_unittest.cc:180: backend_ =
ShortcutsBackendFactory::GetForProfile(&profile_);
On 2012/06/29 20:57:06, rpetterson wrote:
> Since you're using SetTestingFactoryAndUse, you can do these two lines as one
> line because SetTestingFactoryAndUse will return the service it just created.

ShortcutsBackend being a RefcountedProfileKeyedService gets in the way of doing
a decent conversion so I was just following the pattern used elsewhere.

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/history...
File chrome/browser/history/shortcuts_backend_factory.h (right):

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/history...
chrome/browser/history/shortcuts_backend_factory.h:33:
GetForProfileForTesting(Profile* profile);
On 2012/06/29 20:57:06, rpetterson wrote:
> I think a better name would be BuildProfileForTesting since it mimics the
> BuildServiceInstanceFor function rather than the GetForProfile function.

Done.

https://chromiumcodereview.appspot.com/10701043/diff/1/chrome/browser/history...
chrome/browser/history/shortcuts_backend_factory.h:38:
GetForProfileNoDatabaseForTesting(Profile* profile);
On 2012/06/29 20:57:06, rpetterson wrote:
> Same as above, BuildProfileNoDatabaseForTesting.

Done.

[mrossetti, 2012-07-03 14:53:58]

Thanks for the assist!

[Miranda Callahan, 2012-07-03 15:23:41]

A few questions -- here you go:

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/shortcuts_provider.cc:273:
backend->shortcuts_map().end();
I am confused -- you check to see if the backend doesn't exist, and then call
something from if if it doesn't? Should this just be a return?

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
File chrome/browser/history/shortcuts_backend.cc (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
chrome/browser/history/shortcuts_backend.cc:269:
BrowserThread::CurrentlyOn(BrowserThread::UI));
hmm, I am again confused -- is this really DCHECKing that the thread is either
the UI thread or not the UI thread?

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
File chrome/browser/history/shortcuts_backend.h (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
chrome/browser/history/shortcuts_backend.h:75: // of the database, in which case
all operations are performed in memory only.
Nice improvement of this comment. :-)

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
File chrome/browser/history/shortcuts_backend_factory.h (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
chrome/browser/history/shortcuts_backend_factory.h:35: // Creates and returns a
backend but without creating its persistant database
nit: s/persistant/persistent

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:45: #include
"chrome/browser/history/shortcuts_backend_factory.h"
does this need to be added?

[mrossetti, 2012-07-03 16:42:29]

Thanks for your eagle eye!

Issues addressed.

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/shortcuts_provider.cc:273:
backend->shortcuts_map().end();
On 2012/07/03 15:23:41, Miranda Callahan wrote:
> I am confused -- you check to see if the backend doesn't exist, and then call
> something from if if it doesn't? Should this just be a return?

Whoa! I don't know how this got overlooked. Fixed.

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
File chrome/browser/history/shortcuts_backend.cc (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
chrome/browser/history/shortcuts_backend.cc:269:
BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/07/03 15:23:41, Miranda Callahan wrote:
> hmm, I am again confused -- is this really DCHECKing that the thread is either
> the UI thread or not the UI thread?

That is correct! ;^)

The IsWellKnownThread test takes care of the potentially strange threading
environment while running a unit test when we don't really care if the thread is
actually the UI thread or not. When being run during normal browser operations
this will insure that the function is only run on the proper thread.

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
File chrome/browser/history/shortcuts_backend.h (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
chrome/browser/history/shortcuts_backend.h:75: // of the database, in which case
all operations are performed in memory only.
On 2012/07/03 15:23:41, Miranda Callahan wrote:
> Nice improvement of this comment. :-)

Thanks.

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
File chrome/browser/history/shortcuts_backend_factory.h (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
chrome/browser/history/shortcuts_backend_factory.h:35: // Creates and returns a
backend but without creating its persistant database
On 2012/07/03 15:23:41, Miranda Callahan wrote:
> nit: s/persistant/persistent

Done.

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:45: #include
"chrome/browser/history/shortcuts_backend_factory.h"
On 2012/07/03 15:23:41, Miranda Callahan wrote:
> does this need to be added?

No, removed.

[Miranda Callahan, 2012-07-03 17:41:26]

LGTM -- thanks!

On 2012/07/03 16:42:29, mzrossetti wrote:
> Thanks for your eagle eye!
> 
> Issues addressed.
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/aut...
> File chrome/browser/autocomplete/shortcuts_provider.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/aut...
> chrome/browser/autocomplete/shortcuts_provider.cc:273:
> backend->shortcuts_map().end();
> On 2012/07/03 15:23:41, Miranda Callahan wrote:
> > I am confused -- you check to see if the backend doesn't exist, and then
call
> > something from if if it doesn't? Should this just be a return?
> 
> Whoa! I don't know how this got overlooked. Fixed.
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
> File chrome/browser/history/shortcuts_backend.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
> chrome/browser/history/shortcuts_backend.cc:269:
> BrowserThread::CurrentlyOn(BrowserThread::UI));
> On 2012/07/03 15:23:41, Miranda Callahan wrote:
> > hmm, I am again confused -- is this really DCHECKing that the thread is
either
> > the UI thread or not the UI thread?
> 
> That is correct! ;^)
> 
> The IsWellKnownThread test takes care of the potentially strange threading
> environment while running a unit test when we don't really care if the thread
is
> actually the UI thread or not. When being run during normal browser operations
> this will insure that the function is only run on the proper thread.
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
> File chrome/browser/history/shortcuts_backend.h (right):
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
> chrome/browser/history/shortcuts_backend.h:75: // of the database, in which
case
> all operations are performed in memory only.
> On 2012/07/03 15:23:41, Miranda Callahan wrote:
> > Nice improvement of this comment. :-)
> 
> Thanks.
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
> File chrome/browser/history/shortcuts_backend_factory.h (right):
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/his...
> chrome/browser/history/shortcuts_backend_factory.h:35: // Creates and returns
a
> backend but without creating its persistant database
> On 2012/07/03 15:23:41, Miranda Callahan wrote:
> > nit: s/persistant/persistent
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/pro...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10701043/diff/12001/chrome/browser/pro...
> chrome/browser/profiles/profile_impl.cc:45: #include
> "chrome/browser/history/shortcuts_backend_factory.h"
> On 2012/07/03 15:23:41, Miranda Callahan wrote:
> > does this need to be added?
> 
> No, removed.