Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
297233008:
[NTP] Use server thumbnails from SuggestionsService on Android NTP. (Closed)
|
Created:
6 years, 7 months ago by Mathieu Modified:
6 years, 6 months ago Reviewers:
Ted C CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[NTP] Use server thumbnails from SuggestionsService on Android NTP.
No-op for non-server recommendations. Will prioritize local thumbnails.
BUG=None
TEST=Manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274637
Patch Set 1 #
Total comments: 5
Patch Set 2 : profile instead of service #Patch Set 3 : UI thread concerns #
Total comments: 4
Patch Set 4 : DCHECK #Messages
Total messages: 15 (0 generated)
Hi Ted, Can you have a look?
https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:121: const GURL& gurl, +2 for the param lines https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:157: SuggestionsServiceFactory::GetForProfile(profile); BrowserContextKeyedBaseFactory is NonThreadSafe, so I don't think you can do this here. Maybe this is always being called on the UI thread, so this doesn't matter, but then we are doing a bunch of UI thread work below unnecessarily and we should assert we are on the UI thread to prevent this breaking in the future.
Thanks Ted! https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:121: const GURL& gurl, On 2014/05/28 00:21:59, Ted C wrote: > +2 for the param lines Done. https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:157: SuggestionsServiceFactory::GetForProfile(profile); On 2014/05/28 00:21:59, Ted C wrote: > BrowserContextKeyedBaseFactory is NonThreadSafe, so I don't think you can do > this here. > > Maybe this is always being called on the UI thread, so this doesn't matter, but > then we are doing a bunch of UI thread work below unnecessarily and we should > assert we are on the UI thread to prevent this breaking in the future. Ah, thanks. Passing the profile pointer instead should be good then? Have a look.
https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:157: SuggestionsServiceFactory::GetForProfile(profile); On 2014/05/28 02:48:17, Mathieu Perreault wrote: > On 2014/05/28 00:21:59, Ted C wrote: > > BrowserContextKeyedBaseFactory is NonThreadSafe, so I don't think you can do > > this here. > > > > Maybe this is always being called on the UI thread, so this doesn't matter, > but > > then we are doing a bunch of UI thread work below unnecessarily and we should > > assert we are on the UI thread to prevent this breaking in the future. > > Ah, thanks. Passing the profile pointer instead should be good then? Have a > look. I don't think profile is thread safe either. And GetForProfile shouldn't be called on anything put the UI thread either (ironically it like SuggestionsServiceFactory might have overriden just enough methods that the base ThreadChecker calls are all being skipped an no DCHECKs are being hit). top_sites is passed a ref pointer and is thread safe, but I don't think you can make the same assumptions with profile. I think you need to get back to the MostVisitedSites object on the UI thread to do what you want instead of passing these values.
On 2014/05/28 17:45:38, Ted C wrote: > https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... > File chrome/browser/android/most_visited_sites.cc (right): > > https://codereview.chromium.org/297233008/diff/1/chrome/browser/android/most_... > chrome/browser/android/most_visited_sites.cc:157: > SuggestionsServiceFactory::GetForProfile(profile); > On 2014/05/28 02:48:17, Mathieu Perreault wrote: > > On 2014/05/28 00:21:59, Ted C wrote: > > > BrowserContextKeyedBaseFactory is NonThreadSafe, so I don't think you can do > > > this here. > > > > > > Maybe this is always being called on the UI thread, so this doesn't matter, > > but > > > then we are doing a bunch of UI thread work below unnecessarily and we > should > > > assert we are on the UI thread to prevent this breaking in the future. > > > > Ah, thanks. Passing the profile pointer instead should be good then? Have a > > look. > > I don't think profile is thread safe either. And GetForProfile shouldn't be > called on anything put the UI thread either (ironically it like > SuggestionsServiceFactory might have overriden just enough methods that the base > ThreadChecker calls are all being skipped an no DCHECKs are being hit). > > top_sites is passed a ref pointer and is thread safe, but I don't think you can > make the same assumptions with profile. > > I think you need to get back to the MostVisitedSites object on the UI thread to > do what you want instead of passing these values. Hi Ted, After trying a few things, I've come to this latest solution. The local thumbnail lookup still happens on the DB thread and is passed a "lookup_failed" callback which is going to be posted to the UI thread, and bound with the suggestions service that we previously obtained from the UI thread as well. I don't think this will be creating thread safety issues.
lgtm, a broader question but isn't in the scope of this review https://codereview.chromium.org/297233008/diff/30001/chrome/browser/android/m... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/297233008/diff/30001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:218: // Called from the UI Thread. Should we DCHECK we are on the UI thread here now? https://codereview.chromium.org/297233008/diff/30001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:235: base::Bind(&GetSuggestionsThumbnailOnUIThread, Out of curiosity, would it be better to batch all failed thumbnails into a single request? Just wondering if we should try to avoid 6 nearly concurrent requests to the same server.
Thanks, submitting. https://codereview.chromium.org/297233008/diff/30001/chrome/browser/android/m... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/297233008/diff/30001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:218: // Called from the UI Thread. On 2014/06/02 22:36:26, Ted C wrote: > Should we DCHECK we are on the UI thread here now? Done. https://codereview.chromium.org/297233008/diff/30001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:235: base::Bind(&GetSuggestionsThumbnailOnUIThread, On 2014/06/02 22:36:26, Ted C wrote: > Out of curiosity, would it be better to batch all failed thumbnails into a > single request? Just wondering if we should try to avoid 6 nearly concurrent > requests to the same server. My next CL is going to be implementing local caching of server thumbnails (hopefully transparent to this file). We will be able to choose whether to make GetPageThumbnail only return a thumbnail if it's cached locally. As well we can implement batching, but that will be in SuggestionsService. Lots of improvements to be made for sure.
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/297233008/50001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/297233008/50001
Message was sent while issue was closed.
Change committed as 274637 |
