Add caching for apps list search results.

Add caching for apps list search results.
Add the ability to cache results across searches and sessions for any searches for either people or webstore.

R=xiyuan@chromium.org
BUG=295163
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231997

[xiyuan, 2013-10-29 02:32:23]

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/common/webservice_cache.cc (right):

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.cc:28: :
browser_context_(context),
nit: browser_context_ seems not use outside the constructor, consider remove

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.cc:41: const
CacheResult WebserviceCache::Get(QueryType type,
How does |type| work? If we have cached people result and webstore result under
the same |query|, how would |cache_| store them?

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.cc:52: return
std::make_pair(STALE, (base::DictionaryValue*)NULL);
static_cast<base::DictionaryValue*>(NULL)

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/common/webservice_cache.h (right):

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.h:28: enum QueryType {
Move into the WebserviceCache class.

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.h:38: struct Payload {
Is this used outside WebserviceCache? If not, move it into the private section
of the class.

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.h:67: const
CacheResult Get(QueryType type, const std::string& query);
It seems that we only use FRESH result. Why do we need to return CacheResult
instead of just FRESH value?

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.h:91:
base::DictionaryValue* DictFromPayload(const Payload& payload);
Document ownership of the returned object. Or make it a scoped_ptr<...>

[rkc, 2013-10-29 20:36:23]

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/common/webservice_cache.cc (right):

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.cc:28: :
browser_context_(context),
On 2013/10/29 02:32:24, xiyuan wrote:
> nit: browser_context_ seems not use outside the constructor, consider remove

Done.

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.cc:41: const
CacheResult WebserviceCache::Get(QueryType type,
On 2013/10/29 02:32:24, xiyuan wrote:
> How does |type| work? If we have cached people result and webstore result
under
> the same |query|, how would |cache_| store them?

I completely forgot to implement typed queries. Implemented in the latest patch.

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.cc:52: return
std::make_pair(STALE, (base::DictionaryValue*)NULL);
On 2013/10/29 02:32:24, xiyuan wrote:
> static_cast<base::DictionaryValue*>(NULL)

Done.

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/common/webservice_cache.h (right):

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.h:28: enum QueryType {
On 2013/10/29 02:32:24, xiyuan wrote:
> Move into the WebserviceCache class.

Done.

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.h:38: struct Payload {
On 2013/10/29 02:32:24, xiyuan wrote:
> Is this used outside WebserviceCache? If not, move it into the private section
> of the class.

Done.

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.h:67: const
CacheResult Get(QueryType type, const std::string& query);
On 2013/10/29 02:32:24, xiyuan wrote:
> It seems that we only use FRESH result. Why do we need to return CacheResult
> instead of just FRESH value?

Whether the value is stale or fresh, we still display the cached value. The
difference is that if the value is stale, we kick off a webservice query to
refresh the value in the cache and the display, hence the providers need to know
whether a value is fresh or stale, so they know whether to return right out or
to go ahead and start a web query anyway.

https://codereview.chromium.org/49353006/diff/120001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/common/webservice_cache.h:91:
base::DictionaryValue* DictFromPayload(const Payload& payload);
On 2013/10/29 02:32:24, xiyuan wrote:
> Document ownership of the returned object. Or make it a scoped_ptr<...>

Done.

[xiyuan, 2013-10-29 22:02:38]

lgtm

[commit-bot: I haz the power, 2013-10-29 22:23:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/49353006/250001

[commit-bot: I haz the power, 2013-10-30 01:20:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-30 20:37:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/49353006/470002

[commit-bot: I haz the power, 2013-10-31 04:22:53]

Change committed as 231997