Make GetDismissedSuggestionsForDebugging asynchronous

Make GetDismissedSuggestionsForDebugging asynchronous

Use callback instead of return value for GetDismissedSuggestionsForDebugging
in ContentSuggestionsProvider and ContentSuggestionsService. Change the
implementations of all providers accordingly. Change the SnippetsInternals
to fetch the dismissed suggestions asynchronously only if the corresponding
UI is visible. Adjust unit tests.

Change the implementation in OfflinePageSuggestionsProvider to return the
actual suggestions instead of dummies.

BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[Philipp Keck, 2016-08-19 09:15:50]

Description was changed from

==========
Make GetDismissedSuggestionsForDebugging asynchronous

Use callback instead of return value for GetDismissedSuggestionsForDebugging
in ContentSuggestionsProvider and ContentSuggestionsService. Change the
implementations of all providers accordingly. Change the SnippetsInternals
to fetch the dismissed suggestions asynchronously only if the corresponding
UI is visible. Adjust unit tests.

Change the implementation in OfflinePageSuggestionsProvider to return the
actual suggestions instead of dummies.

BUG=628198
==========

to

==========
Make GetDismissedSuggestionsForDebugging asynchronous

Use callback instead of return value for GetDismissedSuggestionsForDebugging
in ContentSuggestionsProvider and ContentSuggestionsService. Change the
implementations of all providers accordingly. Change the SnippetsInternals
to fetch the dismissed suggestions asynchronously only if the corresponding
UI is visible. Adjust unit tests.

Change the implementation in OfflinePageSuggestionsProvider to return the
actual suggestions instead of dummies.

BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Philipp Keck, 2016-08-19 09:20:19]

pke@google.com changed reviewers:
+ treib@chromium.org

[Philipp Keck, 2016-08-19 09:20:20]

PTAL

[Marc Treib, 2016-08-19 10:07:51]

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:153: for (auto& it
: dismissed_state_) {
Please use the actual type, and a proper name - this isn't an iterator.

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:160:
base::Unretained(this)));
I don't think Unretained is safe here - you'll probably have to add a
WeakPtrFactory and use a WeakPtr here.

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:216:
base::Unretained(this)));
Here too.

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:239:
base::Unretained(this)));
And here.

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.h (right):

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.h:80:
dismissed_suggestions_;
Maybe make a struct that contains DismissedState and the vector of suggestions,
then you'd need only one map?

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:140: return
callback.Run(category, std::move(suggestions));
No "return"

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:34: Category category,
The category param isn't really required in the callback - the caller has it
anyway. I.e. in the internals, you could do:
base::Bind(&SnippetsInternalsMessageHandler::OnDismissedSuggestionsLoaded,
base::Unretained(this), category));
(note how you bind the category param here, so the callback type won't have it
anymore).

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:112:
iterator->second->GetDismissedSuggestionsForDebugging(category, callback);
else return empty vector?

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:360: return
callback.Run(category, std::move(result));
No "return" - does this compile?!

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:226:
// |OnOfflinePagesLoadedForDismissedDebugging| below.
Maybe time to extract it into a helper function?

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:276:
return callback.Run(category, std::move(suggestions));
No "return"

[Philipp Keck, 2016-08-19 12:11:54]

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:153: for (auto& it
: dismissed_state_) {
On 2016/08/19 10:07:51, Marc Treib wrote:
> Please use the actual type, and a proper name - this isn't an iterator.

Done.

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:160:
base::Unretained(this)));
On 2016/08/19 10:07:51, Marc Treib wrote:
> I don't think Unretained is safe here - you'll probably have to add a
> WeakPtrFactory and use a WeakPtr here.

Done.

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:216:
base::Unretained(this)));
On 2016/08/19 10:07:51, Marc Treib wrote:
> Here too.

Done.

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:239:
base::Unretained(this)));
On 2016/08/19 10:07:51, Marc Treib wrote:
> And here.

Done.

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.h (right):

https://codereview.chromium.org/2260783002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.h:80:
dismissed_suggestions_;
On 2016/08/19 10:07:51, Marc Treib wrote:
> Maybe make a struct that contains DismissedState and the vector of
suggestions,
> then you'd need only one map?

Not sure that would help readability (except if it has a great name, but I
couldn't come up with one) or lead to less code.

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:140: return
callback.Run(category, std::move(suggestions));
On 2016/08/19 10:07:51, Marc Treib wrote:
> No "return"

Done. Why does that even compile ...

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:34: Category category,
On 2016/08/19 10:07:51, Marc Treib wrote:
> The category param isn't really required in the callback - the caller has it
> anyway. I.e. in the internals, you could do:
> base::Bind(&SnippetsInternalsMessageHandler::OnDismissedSuggestionsLoaded,
> base::Unretained(this), category));
> (note how you bind the category param here, so the callback type won't have it
> anymore).

That's right. Why does ImageFetchedCallback have the |suggestion_id| parameter?
Maybe they should both be removed.

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:112:
iterator->second->GetDismissedSuggestionsForDebugging(category, callback);
On 2016/08/19 10:07:51, Marc Treib wrote:
> else return empty vector?

Done.

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:360: return
callback.Run(category, std::move(result));
On 2016/08/19 10:07:51, Marc Treib wrote:
> No "return" - does this compile?!

Done. No idea why the compiler accepts this.

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:226:
// |OnOfflinePagesLoadedForDismissedDebugging| below.
On 2016/08/19 10:07:51, Marc Treib wrote:
> Maybe time to extract it into a helper function?

I thought about that, also in order to simplify the huge if-condition at line
266.
To make that simpler, I had a solution with a function pointer (not sure if
that's an anti-pattern?), which I'm putting back in now. Let me know what you
think.

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:276:
return callback.Run(category, std::move(suggestions));
On 2016/08/19 10:07:51, Marc Treib wrote:
> No "return"

Done.

[Marc Treib, 2016-08-19 12:36:15]

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:34: Category category,
On 2016/08/19 12:11:53, Philipp Keck wrote:
> On 2016/08/19 10:07:51, Marc Treib wrote:
> > The category param isn't really required in the callback - the caller has it
> > anyway. I.e. in the internals, you could do:
> > base::Bind(&SnippetsInternalsMessageHandler::OnDismissedSuggestionsLoaded,
> > base::Unretained(this), category));
> > (note how you bind the category param here, so the callback type won't have
it
> > anymore).
> 
> That's right. Why does ImageFetchedCallback have the |suggestion_id|
parameter?
> Maybe they should both be removed.

I don't know if there's a good reason for it in the ImageFetchedCallback. If it
can be removed, that fine with me; different CL though.

https://codereview.chromium.org/2260783002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2260783002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:18: #include
"base/memory/weak_ptr.h"
-> header

https://codereview.chromium.org/2260783002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.h (right):

https://codereview.chromium.org/2260783002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.h:82:
base::WeakPtrFactory<SnippetsInternalsMessageHandler> weak_ptr_factory_;
#include "base/memory/weak_ptr.h"

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:267:
(category == recent_tabs_category_) ? &IsRecentTab : &IsDownload;
This is quite hard to read. How about something like
if ((category == recent_tabs && IsRecentTab(..)) || (same for downloads))
? Much easier to understand, and not much longer overall.

[Philipp Keck, 2016-08-19 14:20:40]

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2260783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:34: Category category,
On 2016/08/19 12:36:14, Marc Treib wrote:
> On 2016/08/19 12:11:53, Philipp Keck wrote:
> > On 2016/08/19 10:07:51, Marc Treib wrote:
> > > The category param isn't really required in the callback - the caller has
it
> > > anyway. I.e. in the internals, you could do:
> > > base::Bind(&SnippetsInternalsMessageHandler::OnDismissedSuggestionsLoaded,
> > > base::Unretained(this), category));
> > > (note how you bind the category param here, so the callback type won't
have
> it
> > > anymore).
> > 
> > That's right. Why does ImageFetchedCallback have the |suggestion_id|
> parameter?
> > Maybe they should both be removed.
> 
> I don't know if there's a good reason for it in the ImageFetchedCallback. If
it
> can be removed, that fine with me; different CL though.

Done.

https://codereview.chromium.org/2260783002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2260783002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:18: #include
"base/memory/weak_ptr.h"
On 2016/08/19 12:36:15, Marc Treib wrote:
> -> header

Done.

https://codereview.chromium.org/2260783002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.h (right):

https://codereview.chromium.org/2260783002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.h:82:
base::WeakPtrFactory<SnippetsInternalsMessageHandler> weak_ptr_factory_;
On 2016/08/19 12:36:15, Marc Treib wrote:
> #include "base/memory/weak_ptr.h"

Done.

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:267:
(category == recent_tabs_category_) ? &IsRecentTab : &IsDownload;
On 2016/08/19 12:36:15, Marc Treib wrote:
> This is quite hard to read. How about something like
> if ((category == recent_tabs && IsRecentTab(..)) || (same for downloads))
> ? Much easier to understand, and not much longer overall.

The reason I introduced it is because the structure of the if would be:
if (((A&B) | (C&D)) & E)
where E is the dismissed-check.

If the | is done here, the if becomes if (X & E), which is much nicer.

Maybe I'm missing something?, but I wouldn't find the other solution easier to
read. This solution needs you to read some stars and a function pointer, but I
think the logical flow is more obvious here.

That being said, I'm fine with either solution.

[Philipp Keck, 2016-08-19 14:28:36]

pke@google.com changed reviewers:
+ bauerb@chromium.org

[Philipp Keck, 2016-08-19 14:28:36]

bauerb@chromium.org: Please review changes in 
chrome/browser/*

[Marc Treib, 2016-08-19 15:18:31]

LGTM with the one remaining comment

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:267:
(category == recent_tabs_category_) ? &IsRecentTab : &IsDownload;
On 2016/08/19 14:20:40, Philipp Keck wrote:
> On 2016/08/19 12:36:15, Marc Treib wrote:
> > This is quite hard to read. How about something like
> > if ((category == recent_tabs && IsRecentTab(..)) || (same for downloads))
> > ? Much easier to understand, and not much longer overall.
> 
> The reason I introduced it is because the structure of the if would be:
> if (((A&B) | (C&D)) & E)
> where E is the dismissed-check.
> 
> If the | is done here, the if becomes if (X & E), which is much nicer.
> 
> Maybe I'm missing something?, but I wouldn't find the other solution easier to
> read. This solution needs you to read some stars and a function pointer, but I
> think the logical flow is more obvious here.
> 
> That being said, I'm fine with either solution.

If you're concerned that the condition in the if becomes to long, then pull part
of it out into a separate variable. Raw function pointers are IMO almost never a
good idea in C++.

[Bernhard Bauer, 2016-08-19 16:22:38]

chrome/browser/LGTM, but I agree with Marc's comment about the function pointer.

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:267:
(category == recent_tabs_category_) ? &IsRecentTab : &IsDownload;
On 2016/08/19 15:18:30, Marc Treib wrote:
> On 2016/08/19 14:20:40, Philipp Keck wrote:
> > On 2016/08/19 12:36:15, Marc Treib wrote:
> > > This is quite hard to read. How about something like
> > > if ((category == recent_tabs && IsRecentTab(..)) || (same for downloads))
> > > ? Much easier to understand, and not much longer overall.
> > 
> > The reason I introduced it is because the structure of the if would be:
> > if (((A&B) | (C&D)) & E)
> > where E is the dismissed-check.
> > 
> > If the | is done here, the if becomes if (X & E), which is much nicer.
> > 
> > Maybe I'm missing something?, but I wouldn't find the other solution easier
to
> > read. This solution needs you to read some stars and a function pointer, but
I
> > think the logical flow is more obvious here.
> > 
> > That being said, I'm fine with either solution.
> 
> If you're concerned that the condition in the if becomes to long, then pull
part
> of it out into a separate variable. Raw function pointers are IMO almost never
a
> good idea in C++.

Alternatively / in addition, you could split the check up by `continue`-ing if
one of the conditions fails. And if you want to keep the category match check in
a single expression, you could try using a lambda instead of a function pointer
(which at least looks nicer) or using a helper function that calls out to
IsRecentTab() or IsDownload() based on the category.

[Philipp Keck, 2016-08-19 16:52:36]

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2260783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:267:
(category == recent_tabs_category_) ? &IsRecentTab : &IsDownload;
On 2016/08/19 16:22:38, Bernhard Bauer wrote:
> On 2016/08/19 15:18:30, Marc Treib wrote:
> > On 2016/08/19 14:20:40, Philipp Keck wrote:
> > > On 2016/08/19 12:36:15, Marc Treib wrote:
> > > > This is quite hard to read. How about something like
> > > > if ((category == recent_tabs && IsRecentTab(..)) || (same for
downloads))
> > > > ? Much easier to understand, and not much longer overall.
> > > 
> > > The reason I introduced it is because the structure of the if would be:
> > > if (((A&B) | (C&D)) & E)
> > > where E is the dismissed-check.
> > > 
> > > If the | is done here, the if becomes if (X & E), which is much nicer.
> > > 
> > > Maybe I'm missing something?, but I wouldn't find the other solution
easier
> to
> > > read. This solution needs you to read some stars and a function pointer,
but
> I
> > > think the logical flow is more obvious here.
> > > 
> > > That being said, I'm fine with either solution.
> > 
> > If you're concerned that the condition in the if becomes to long, then pull
> part
> > of it out into a separate variable. Raw function pointers are IMO almost
never
> a
> > good idea in C++.
> 
> Alternatively / in addition, you could split the check up by `continue`-ing if
> one of the conditions fails. And if you want to keep the category match check
in
> a single expression, you could try using a lambda instead of a function
pointer
> (which at least looks nicer) or using a helper function that calls out to
> IsRecentTab() or IsDownload() based on the category.

The "continue"-solution is a good idea, I implemented that. No need for a single
condition, that was just to save indentation. But with "continue", that problem
is gone.

Would a lambda get rid of the function pointer?

[Philipp Keck, 2016-08-19 16:53:40]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-19 16:53:41]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2260783002/#ps60001
(title: "Replace function pointer, use multiple if-continue")

[commit-bot: I haz the power, 2016-08-19 16:54:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 18:30:28]

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

[commit-bot: I haz the power, 2016-08-19 18:30:30]

Exceeded global retry quota

[Philipp Keck, 2016-08-22 06:27:56]

The CQ bit was checked by pke@google.com

[commit-bot: I haz the power, 2016-08-22 06:28:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 08:54:43]

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

[commit-bot: I haz the power, 2016-08-22 08:54:44]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Marc Treib, 2016-08-22 08:57:43]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-08-22 08:58:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 11:25:07]

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

[commit-bot: I haz the power, 2016-08-22 11:25:09]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Philipp Keck, 2016-08-23 07:31:54]

The CQ bit was checked by pke@google.com

[commit-bot: I haz the power, 2016-08-23 07:32:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 09:18:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-23 09:20:37]

Description was changed from

==========
Make GetDismissedSuggestionsForDebugging asynchronous

Use callback instead of return value for GetDismissedSuggestionsForDebugging
in ContentSuggestionsProvider and ContentSuggestionsService. Change the
implementations of all providers accordingly. Change the SnippetsInternals
to fetch the dismissed suggestions asynchronously only if the corresponding
UI is visible. Adjust unit tests.

Change the implementation in OfflinePageSuggestionsProvider to return the
actual suggestions instead of dummies.

BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make GetDismissedSuggestionsForDebugging asynchronous

Use callback instead of return value for GetDismissedSuggestionsForDebugging
in ContentSuggestionsProvider and ContentSuggestionsService. Change the
implementations of all providers accordingly. Change the SnippetsInternals
to fetch the dismissed suggestions asynchronously only if the corresponding
UI is visible. Adjust unit tests.

Change the implementation in OfflinePageSuggestionsProvider to return the
actual suggestions instead of dummies.

BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/de0dd9f4b9a81eb0f168fbf1311a6b315f092b68
Cr-Commit-Position: refs/heads/master@{#413699}
==========

[commit-bot: I haz the power, 2016-08-23 09:20:38]

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