Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1677073002:
Fetch snippets from ChromeReader and show them on the NTP (Closed)
|
Created:
4 years, 10 months ago by May Modified:
4 years, 10 months ago Reviewers:
Bernhard Bauer, Roger Tawa OOO till Jul 10th, Ryan Sleevi, noyau (Ping after 24h), davidben, xunjieli, Marc Treib, mmenke, blundell CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFetch snippets from ChromeReader and show them on the NTP
BUG=584620
Committed: https://crrev.com/bd68437f4f6a2641616b66713bc15b4e287876bd
Cr-Commit-Position: refs/heads/master@{#377100}
Patch Set 1 #Patch Set 2 : Cleaning up #
Total comments: 89
Patch Set 3 : Addressing comments #
Total comments: 38
Patch Set 4 : Addressing comments #
Total comments: 6
Patch Set 5 : Addressing comments #Patch Set 6 : Fixed CQ failures #
Total comments: 4
Patch Set 7 : CR comments, modified iOS NTPSnippetServiceFactory #
Total comments: 8
Patch Set 8 : Added iOS-appropriate file path #
Total comments: 7
Patch Set 9 : Updated iOS, unit tests, addressed mmenke's issues #Patch Set 10 : Fix ios build #Patch Set 11 : Rebase #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : fix asan #
Total comments: 10
Patch Set 15 : Review comments #Messages
Total messages: 79 (24 generated)
maybelle@chromium.org changed reviewers: + bauerb@chromium.org, treib@chromium.org
v1 of our ContentFetcher (though I still call it Snippets / SnippetFetcher). PTAL!
https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:686: SnippetsController.get(this); If we have an explicit call here anyway (as opposed to SnippetsController just getting created the first time someone needs it), could we go from lazy creation to an explicit create() method? Also, could you make sure we don't start fetching stuff here by default? https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NTPSnippetsBridge.java (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NTPSnippetsBridge.java:15: public class NTPSnippetsBridge { Nit: I think Java code is a bit stricter about usage of acronyms in names -- IIRC, it should be NtpSnippetsBridge. https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:107: } catch (Exception e) { Is there a more detailed set of exceptions you can catch here? Also, maybe log something in that case? https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:33: ntp_snippets_service_->AddObserver(this); When are you going to unregister yourself as an observer? (FTR, ScopedObserver can be very helpful for that.) https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:39: void NTPSnippetsBridge::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) { This method isn't called anywhere (or declared in Java, for that matter). https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:48: if (ntp_snippets_service_->is_loaded()) { Nit: no braces for single-line bodies. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:53: void NTPSnippetsBridge::NTPSnippetsServiceLoaded(NTPSnippetsService* service) { You could just directly access |ntp_snippets_service_| instead of passing it. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:61: for (auto& snippet : *service) { Can you use an actual type here instead of auto? And maybe const? https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.h (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:13: namespace ntp_snippets { Why the namespace? (For things in //chrome, you don't need a namespace, and with the class the namespace is a bit redundant.) https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:15: class NTPSnippetsService; This isn't necessary if you include the header file. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:42: bool RegisterNTPSnippetsBridge(JNIEnv* env); Make this a static class method? https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.cc (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.cc:26: if (profile) { Nit: no braces. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.cc:36: void NTPSnippetsController::FetchSnippets(JNIEnv* env, You could make this a static method that just takes the profile (or even gets the last used profile here in native code...), then you don't need to allocate anything in this class. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.h (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.h:11: #include "components/ntp_snippets/ntp_snippets_service.h" I don't think you need this include. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:52: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE), Use BrowserThread::GetBlockingPool() for this (but you should probably get a SequencedTaskRunner from it, just to make sure there are no weird concurrency issues). https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:42: CHECK(PathService::Get(base::DIR_ANDROID_APP_DATA, &dir)); DCHECK should be fine for this. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:44: NOTIMPLEMENTED(); Maybe also return an empty FilePath in this case? Also, maybe make this a NOTREACHED() as long as this is not supposed to be used anywhere but Android? https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:128: headers.SetHeader("X-GFE-SSL", "yes"); o_O Why do we need this? https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:164: if (response_code != net::HTTP_OK) { Technically, you should also handle the case where there is a race condition and the access token just expires between the time you get it back from the cache and the time it arrives at the server, in which case you have to retry the token request to get a new one (but only once, after that an error should abort). Really, we should extract all of this dance into a shared class. Maybe not for this CL, but *cough* technical debt *cough*... Marc, WDYT? Actually, why do we even use authentication here? https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:11: #include "components/signin/core/browser/signin_tracker.h" Do you need this, or SigninManager (which you could forward-declare)? https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:23: class NTPSnippetsFetcher : public KeyedService, I don't think this should be a KeyedService if it's owned by the NTPSnippetsService. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:31: virtual void OnNTPSnippetsDownloaded() = 0; Instead of a single-method interface, consider using a base::Callback and base::CallbackList instead of an ObserverList. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:86: base::WeakPtrFactory<NTPSnippetsFetcher> weak_ptr_factory_; Add DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:16: extern base::FilePath GetSnippetsSuggestionsPath(); Wait, what's this doing here? This seems like it should be declared in the header of either this class or NTPSnippetsFetcher. Probably the latter, now that I think about it. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:21: if (!success) { No braces. Also, you could use DLOG_IF(). https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:34: snippets_fetcher_.reset(snippets_fetcher); You could directly initialize this in the initializer list. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:73: if (!deserialized) { Eh, undo please 😃 https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:28: using NTPSnippetStorage = std::vector<std::unique_ptr<NTPSnippet>>; This should use scoped_ptr, not std::unique_ptr. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:36: explicit NTPSnippetsService( `explicit` is only necessary for single-argument constructors. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:84: void OnNTPSnippetsDownloaded(); If this really overrides a method, it should have the `override` modifier. Why doesn't a compiler complain about this?
https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NTPSnippetsBridge.java (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NTPSnippetsBridge.java:15: public class NTPSnippetsBridge { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Nit: I think Java code is a bit stricter about usage of acronyms in names -- > IIRC, it should be NtpSnippetsBridge. I'd just remove the "NTP". The SnippetsController doesn't have it either. https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NTPSnippetsBridge.java:26: public void onSnippetAvailable( nit: onSnippet*s*Available https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.h (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:30: void NTPSnippetsServiceShutdown(NTPSnippetsService* service) override; I think these can be private https://codereview.chromium.org/1677073002/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1880: 'android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java', Eeeh.. where does this come from? Bad merge? https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:147: token_service_->RemoveObserver(this); We should probably check if account_id matches the one we expect? https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:164: if (response_code != net::HTTP_OK) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Technically, you should also handle the case where there is a race condition and > the access token just expires between the time you get it back from the cache > and the time it arrives at the server, in which case you have to retry the token > request to get a new one (but only once, after that an error should abort). > > Really, we should extract all of this dance into a shared class. Maybe not for > this CL, but *cough* technical debt *cough*... Marc, WDYT? Yup, I was thinking the same thing while going through this code. > Actually, why do we even use authentication here? Because (at least until recently) ChromeReader required authentication. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:37: explicit NTPSnippetsFetcher( explicit is only useful for single-argument constructors https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:111: base::Bind(&NTPSnippetsService::OnFileReadDone, base::Unretained(this))); I don't think base::Unretained is safe here. You probably need to add a WeakPtrFactory and make this weak_ptr_factory_.GetWeakPtr() https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:87: void OnFileReadDone(std::string& json); const std::string&
One more comment: Right now, there are two classes that connect Java to native, the SnippetsController and the SnippetsBridge. Any reason for not merging them into one?
On 2016/02/09 10:51:53, Marc Treib wrote: > One more comment: Right now, there are two classes that connect Java to native, > the SnippetsController and the SnippetsBridge. Any reason for not merging them > into one? Yes, my intention is that SnippetsBridge is the class we use to access the list of snippets we have, and SnippetsController is what we use to go and fetch snippets (and cleanup current snippets if necessary).
https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:686: SnippetsController.get(this); On 2016/02/08 18:19:26, Bernhard Bauer wrote: > If we have an explicit call here anyway (as opposed to SnippetsController just > getting created the first time someone needs it), could we go from lazy creation > to an explicit create() method? > > Also, could you make sure we don't start fetching stuff here by default? So this was a temporary solution so we fetch new snippets on clank start while we are implementing the piece that will fetch snippets on a schedule instead. In the real implementation, we wouldn't be directly fetching snippets at this point. I could remove it for now, and leave the SnippetsController the way I was originally intending it to be. https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NTPSnippetsBridge.java (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NTPSnippetsBridge.java:15: public class NTPSnippetsBridge { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Nit: I think Java code is a bit stricter about usage of acronyms in names -- > IIRC, it should be NtpSnippetsBridge. Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NTPSnippetsBridge.java:15: public class NTPSnippetsBridge { On 2016/02/09 09:17:54, Marc Treib wrote: > On 2016/02/08 18:19:26, Bernhard Bauer wrote: > > Nit: I think Java code is a bit stricter about usage of acronyms in names -- > > IIRC, it should be NtpSnippetsBridge. > > I'd just remove the "NTP". The SnippetsController doesn't have it either. Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NTPSnippetsBridge.java:26: public void onSnippetAvailable( On 2016/02/09 09:17:54, Marc Treib wrote: > nit: onSnippet*s*Available Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:107: } catch (Exception e) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Is there a more detailed set of exceptions you can catch here? Also, maybe log > something in that case? Oh right, oversight when prototyping. :) Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:33: ntp_snippets_service_->AddObserver(this); On 2016/02/08 18:19:26, Bernhard Bauer wrote: > When are you going to unregister yourself as an observer? > > (FTR, ScopedObserver can be very helpful for that.) Thanks for the hint. Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:39: void NTPSnippetsBridge::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > This method isn't called anywhere (or declared in Java, for that matter). Fixed (by actually calling it on cleanup) https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:48: if (ntp_snippets_service_->is_loaded()) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Nit: no braces for single-line bodies. Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:53: void NTPSnippetsBridge::NTPSnippetsServiceLoaded(NTPSnippetsService* service) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > You could just directly access |ntp_snippets_service_| instead of passing it. It's an override, that's the signature of the NTPSnippetsServiceObserver. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:61: for (auto& snippet : *service) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Can you use an actual type here instead of auto? And maybe const? I used auto because it was the recommended way of accessing NTPSnippetsService (written by noyau@). I'll change it if you feel strongly. Added the const. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.h (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:13: namespace ntp_snippets { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Why the namespace? (For things in //chrome, you don't need a namespace, and with > the class the namespace is a bit redundant.) Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:15: class NTPSnippetsService; On 2016/02/08 18:19:26, Bernhard Bauer wrote: > This isn't necessary if you include the header file. Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:30: void NTPSnippetsServiceShutdown(NTPSnippetsService* service) override; On 2016/02/09 09:17:54, Marc Treib wrote: > I think these can be private Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:42: bool RegisterNTPSnippetsBridge(JNIEnv* env); On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Make this a static class method? Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.cc (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.cc:26: if (profile) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Nit: no braces. Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.cc:36: void NTPSnippetsController::FetchSnippets(JNIEnv* env, On 2016/02/08 18:19:26, Bernhard Bauer wrote: > You could make this a static method that just takes the profile (or even gets > the last used profile here in native code...), then you don't need to allocate > anything in this class. Made it static. Later (depending on how we implement the scheduled downloads) this might have to change, but for now we don't need any more than just a call to fetch snippets directly. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.h (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.h:11: #include "components/ntp_snippets/ntp_snippets_service.h" On 2016/02/08 18:19:26, Bernhard Bauer wrote: > I don't think you need this include. Removed the member NTPSnippetsService variable https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:52: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE), On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Use BrowserThread::GetBlockingPool() for this (but you should probably get a > SequencedTaskRunner from it, just to make sure there are no weird concurrency > issues). Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1880: 'android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java', On 2016/02/09 09:17:54, Marc Treib wrote: > Eeeh.. where does this come from? Bad merge? Woah, good catch. ??? Must have been a bad merge. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:42: CHECK(PathService::Get(base::DIR_ANDROID_APP_DATA, &dir)); On 2016/02/08 18:19:26, Bernhard Bauer wrote: > DCHECK should be fine for this. Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:44: NOTIMPLEMENTED(); On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Maybe also return an empty FilePath in this case? Also, maybe make this a > NOTREACHED() as long as this is not supposed to be used anywhere but Android? Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:128: headers.SetHeader("X-GFE-SSL", "yes"); On 2016/02/08 18:19:26, Bernhard Bauer wrote: > o_O Why do we need this? I'm not sure why we need it. I'd copied the request headers that apido sends (command line the tool that OnePlatform uses for testing deployed services). BUT BUT... I removed this just to see what would happen and it doesn't seem to make a difference https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:147: token_service_->RemoveObserver(this); On 2016/02/09 09:17:54, Marc Treib wrote: > We should probably check if account_id matches the one we expect? If we're going to keep using signed in accounts, yeah. But I'm going to change this whole class to not require sign in given that ChromeReader now supports it. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:164: if (response_code != net::HTTP_OK) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Technically, you should also handle the case where there is a race condition and > the access token just expires between the time you get it back from the cache > and the time it arrives at the server, in which case you have to retry the token > request to get a new one (but only once, after that an error should abort). > > Really, we should extract all of this dance into a shared class. Maybe not for > this CL, but *cough* technical debt *cough*... Marc, WDYT? > > Actually, why do we even use authentication here? Yes we should. :) However, since we're moving to a model where we won't even use the token to fetch the data (because we will instead use the user's history to filter the snippets to get relevant results) so that issue will not come up. And what Marc said -- we needed authentication to access the API previously. In theory that's not the case anymore but I haven't had a chance to test it yet. I have a bug to switch over to not requiring sign in to access ChromeReader (crbug/584346). https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:11: #include "components/signin/core/browser/signin_tracker.h" On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Do you need this, or SigninManager (which you could forward-declare)? Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:23: class NTPSnippetsFetcher : public KeyedService, On 2016/02/08 18:19:26, Bernhard Bauer wrote: > I don't think this should be a KeyedService if it's owned by the > NTPSnippetsService. You're right, leftover crud from original way I wrote it. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:31: virtual void OnNTPSnippetsDownloaded() = 0; On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Instead of a single-method interface, consider using a base::Callback and > base::CallbackList instead of an ObserverList. Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:37: explicit NTPSnippetsFetcher( On 2016/02/09 09:17:54, Marc Treib wrote: > explicit is only useful for single-argument constructors Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:86: base::WeakPtrFactory<NTPSnippetsFetcher> weak_ptr_factory_; On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Add DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:16: extern base::FilePath GetSnippetsSuggestionsPath(); On 2016/02/08 18:19:27, Bernhard Bauer wrote: > Wait, what's this doing here? This seems like it should be declared in the > header of either this class or NTPSnippetsFetcher. Probably the latter, now that > I think about it. I removed this, since I changed the callback model. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:21: if (!success) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > No braces. Also, you could use DLOG_IF(). Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:34: snippets_fetcher_.reset(snippets_fetcher); On 2016/02/08 18:19:27, Bernhard Bauer wrote: > You could directly initialize this in the initializer list. Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:73: if (!deserialized) { On 2016/02/08 18:19:26, Bernhard Bauer wrote: > Eh, undo please 😃 Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:111: base::Bind(&NTPSnippetsService::OnFileReadDone, base::Unretained(this))); On 2016/02/09 09:17:54, Marc Treib wrote: > I don't think base::Unretained is safe here. You probably need to add a > WeakPtrFactory and make this weak_ptr_factory_.GetWeakPtr() Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:28: using NTPSnippetStorage = std::vector<std::unique_ptr<NTPSnippet>>; On 2016/02/08 18:19:27, Bernhard Bauer wrote: > This should use scoped_ptr, not std::unique_ptr. Why should it be a scoped_ptr? We construct the NTPSnippetObject in NTPSnippet class and then this class claims ownership of it here. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:36: explicit NTPSnippetsService( On 2016/02/08 18:19:27, Bernhard Bauer wrote: > `explicit` is only necessary for single-argument constructors. Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:84: void OnNTPSnippetsDownloaded(); On 2016/02/08 18:19:27, Bernhard Bauer wrote: > If this really overrides a method, it should have the `override` modifier. Why > doesn't a compiler complain about this? No idea why it didn't complain. But anyway I removed it in favor a callback list https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:87: void OnFileReadDone(std::string& json); On 2016/02/09 09:17:54, Marc Treib wrote: > const std::string& Done.
A bunch more comments, all of them small :) https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:147: token_service_->RemoveObserver(this); On 2016/02/09 17:38:53, May wrote: > On 2016/02/09 09:17:54, Marc Treib wrote: > > We should probably check if account_id matches the one we expect? > > If we're going to keep using signed in accounts, yeah. But I'm going to change > this whole class to not require sign in given that ChromeReader now supports it. Acknowledged. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:164: if (response_code != net::HTTP_OK) { On 2016/02/09 17:38:53, May wrote: > On 2016/02/08 18:19:26, Bernhard Bauer wrote: > > Technically, you should also handle the case where there is a race condition > and > > the access token just expires between the time you get it back from the cache > > and the time it arrives at the server, in which case you have to retry the > token > > request to get a new one (but only once, after that an error should abort). > > > > Really, we should extract all of this dance into a shared class. Maybe not for > > this CL, but *cough* technical debt *cough*... Marc, WDYT? > > > > Actually, why do we even use authentication here? > > Yes we should. :) However, since we're moving to a model where we won't even use > the token to fetch the data (because we will instead use the user's history to > filter the snippets to get relevant results) so that issue will not come up. > > And what Marc said -- we needed authentication to access the API previously. In > theory that's not the case anymore but I haven't had a chance to test it yet. I > have a bug to switch over to not requiring sign in to access ChromeReader > (crbug/584346). So for the record, I'm fine with landing as-is for now, so that we have something that works :) https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:28: using NTPSnippetStorage = std::vector<std::unique_ptr<NTPSnippet>>; On 2016/02/09 17:38:54, May wrote: > On 2016/02/08 18:19:27, Bernhard Bauer wrote: > > This should use scoped_ptr, not std::unique_ptr. > > Why should it be a scoped_ptr? We construct the NTPSnippetObject in NTPSnippet > class and then this class claims ownership of it here. scoped_ptr and unique_ptr are essentially the same thing, but unique_ptr is a C++11 feature that's not allowed yet. https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:686: SnippetsController.get(this).fetchSnippets(false); IMO this is fine for now, but could you put in a TODO(treib) to remove it when the proper fetching logic is in place? Thanks! https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:28: * Creates an NTPSnippetsBridge for getting snippet data for the current user nit: -NTP https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:115: Log.e(TAG, "Could not get image thumbnail", e); nit: Different messages for the two exceptions? https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.h (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:14: using ntp_snippets::NTPSnippetsService; No "using" in headers https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:28: static bool RegisterNTPSnippetsBridge(JNIEnv* env); This could just be "Register", since it'll be called as NTPSnippetsBridge::Register anyway. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.cc (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.cc:17: static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) { Put the functions in an anonymous namespace instead of using "static". https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.h (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.h:12: using base::android::JavaParamRef; Here too: No "using" in headers https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.h:21: static bool RegisterNTPSnippetsController(JNIEnv* env); Here too: Simply "Register" is enough IMO https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:35: using NTPSnippetsFetcherCallback = I'd call this "SnippetsAvailableCallback" or something like that - "NTPSnippetsFetcher" doesn't really add any information, since it's already inside the class. https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:39: NTPSnippetsFetcher* snippets_fetcher); scoped_ptr<NTPSnippetsFetcher> to make clear that this will take ownership. (You'll have to use std::move in the .cc file) https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:59: bool LoadFromJSONString(const std::string& str); I think this can be private? https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:85: void OnSnippetsDownloaded(const base::FilePath download_path); &
https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:686: SnippetsController.get(this); On 2016/02/09 17:38:52, May wrote: > On 2016/02/08 18:19:26, Bernhard Bauer wrote: > > If we have an explicit call here anyway (as opposed to SnippetsController just > > getting created the first time someone needs it), could we go from lazy > creation > > to an explicit create() method? > > > > Also, could you make sure we don't start fetching stuff here by default? > > So this was a temporary solution so we fetch new snippets on clank start while > we are implementing the piece that will fetch snippets on a schedule instead. In > the real implementation, we wouldn't be directly fetching snippets at this > point. I could remove it for now, and leave the SnippetsController the way I was > originally intending it to be. OK... I just want to make sure we no-op out if the snippets experiment is not enabled. Can we do that maybe in the SnippetsFetcher? https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:53: void NTPSnippetsBridge::NTPSnippetsServiceLoaded(NTPSnippetsService* service) { On 2016/02/09 17:38:53, May wrote: > On 2016/02/08 18:19:26, Bernhard Bauer wrote: > > You could just directly access |ntp_snippets_service_| instead of passing it. > > It's an override, that's the signature of the NTPSnippetsServiceObserver. Oh, I see. Hm, could we only register as an NTPSnippetsServiceObserver in SetSnippetsObserver() instead of in the constructor? Before we have a Java observer, we're not interested in the snippets anyway, and that way this method will only be called through the interface. Or we could even merge SetSnippetsObserver() and the constructor -- the observer is set right after construction anyway, so we wouldn't have to deal with |observer_| being null. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:61: for (auto& snippet : *service) { On 2016/02/09 17:38:53, May wrote: > On 2016/02/08 18:19:26, Bernhard Bauer wrote: > > Can you use an actual type here instead of auto? And maybe const? > > I used auto because it was the recommended way of accessing NTPSnippetsService > (written by noyau@). I'll change it if you feel strongly. Added the const. Re: type, it's very complicated to find out the actual type of |snippet| here, in particular as |service| isn't directly a collection of some type. My rule is roughly, if you wouldn't use auto outside of the for-loop (and we usually don't in Chrome unless the type is very complicated), I wouldn't use it in the for loop. Re: const, it turns out that the type that is replaced with auto already is const, so the additional const isn't actually necessary. I think this really just underscores my point though that it would be preferable to use the real type, because this is extremely hard to notice unless you happen to look at the iterator method in NTPSnippetsService, and if you don't, it either looks wrong (missing const) or is wrong (double const). https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:28: using NTPSnippetStorage = std::vector<std::unique_ptr<NTPSnippet>>; On 2016/02/09 17:38:54, May wrote: > On 2016/02/08 18:19:27, Bernhard Bauer wrote: > > This should use scoped_ptr, not std::unique_ptr. > > Why should it be a scoped_ptr? We construct the NTPSnippetObject in NTPSnippet > class and then this class claims ownership of it here. Yup, but it's just that we usually use scoped_ptr in Chrome to express that notion of ownership. https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:113: Log.e(TAG, "Could not get image thumbnail", e); You could combine these these (`catch (MalformedURLException|IOException e)`). https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:77: observer_.Reset(); You might want to remove the service from the |snippets_service_observer_| here. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.h (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:28: static bool RegisterNTPSnippetsBridge(JNIEnv* env); Nit: I would even just call it Register() for brevity 😃 https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.cc (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.cc:17: static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) { You don't need Init() or Destroy() anymore either -- just make this class purely static. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:54: ->GetSequencedTaskRunnerWithShutdownBehavior( Actually, extract this SequencedTaskRunner into a local variable and then pass it to both -- that way you get the same sequence in both. https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:34: using NTPSnippetsFetcherCallbackType = void(const base::FilePath); const base::FilePath&.
(Sorry for the duplicated comments 😃) https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.cc (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.cc:17: static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) { On 2016/02/10 10:44:43, Marc Treib wrote: > Put the functions in an anonymous namespace instead of using "static". That does not work for JNI bridge methods (because they are declared in the generated header, but we still want them to get internal linkage).
https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.cc (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.cc:17: static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) { On 2016/02/10 10:51:42, Bernhard Bauer wrote: > On 2016/02/10 10:44:43, Marc Treib wrote: > > Put the functions in an anonymous namespace instead of using "static". > > That does not work for JNI bridge methods (because they are declared in the > generated header, but we still want them to get internal linkage). Today I learned! Alright, carry on then :)
https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:686: SnippetsController.get(this); On 2016/02/10 10:48:44, Bernhard Bauer wrote: > On 2016/02/09 17:38:52, May wrote: > > On 2016/02/08 18:19:26, Bernhard Bauer wrote: > > > If we have an explicit call here anyway (as opposed to SnippetsController > just > > > getting created the first time someone needs it), could we go from lazy > > creation > > > to an explicit create() method? > > > > > > Also, could you make sure we don't start fetching stuff here by default? > > > > So this was a temporary solution so we fetch new snippets on clank start while > > we are implementing the piece that will fetch snippets on a schedule instead. > In > > the real implementation, we wouldn't be directly fetching snippets at this > > point. I could remove it for now, and leave the SnippetsController the way I > was > > originally intending it to be. > > OK... I just want to make sure we no-op out if the snippets experiment is not > enabled. Can we do that maybe in the SnippetsFetcher? I wrapped this around a command line flag for now. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:53: void NTPSnippetsBridge::NTPSnippetsServiceLoaded(NTPSnippetsService* service) { On 2016/02/10 10:48:44, Bernhard Bauer wrote: > On 2016/02/09 17:38:53, May wrote: > > On 2016/02/08 18:19:26, Bernhard Bauer wrote: > > > You could just directly access |ntp_snippets_service_| instead of passing > it. > > > > It's an override, that's the signature of the NTPSnippetsServiceObserver. > > Oh, I see. > > Hm, could we only register as an NTPSnippetsServiceObserver in > SetSnippetsObserver() instead of in the constructor? Before we have a Java > observer, we're not interested in the snippets anyway, and that way this method > will only be called through the interface. > > Or we could even merge SetSnippetsObserver() and the constructor -- the observer > is set right after construction anyway, so we wouldn't have to deal with > |observer_| being null. Done. https://codereview.chromium.org/1677073002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:61: for (auto& snippet : *service) { On 2016/02/10 10:48:44, Bernhard Bauer wrote: > On 2016/02/09 17:38:53, May wrote: > > On 2016/02/08 18:19:26, Bernhard Bauer wrote: > > > Can you use an actual type here instead of auto? And maybe const? > > > > I used auto because it was the recommended way of accessing NTPSnippetsService > > (written by noyau@). I'll change it if you feel strongly. Added the const. > > Re: type, it's very complicated to find out the actual type of |snippet| here, > in particular as |service| isn't directly a collection of some type. My rule is > roughly, if you wouldn't use auto outside of the for-loop (and we usually don't > in Chrome unless the type is very complicated), I wouldn't use it in the for > loop. > > Re: const, it turns out that the type that is replaced with auto already is > const, so the additional const isn't actually necessary. I think this really > just underscores my point though that it would be preferable to use the real > type, because this is extremely hard to notice unless you happen to look at the > iterator method in NTPSnippetsService, and if you don't, it either looks wrong > (missing const) or is wrong (double const). Done. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:164: if (response_code != net::HTTP_OK) { On 2016/02/10 10:44:43, Marc Treib wrote: > On 2016/02/09 17:38:53, May wrote: > > On 2016/02/08 18:19:26, Bernhard Bauer wrote: > > > Technically, you should also handle the case where there is a race condition > > and > > > the access token just expires between the time you get it back from the > cache > > > and the time it arrives at the server, in which case you have to retry the > > token > > > request to get a new one (but only once, after that an error should abort). > > > > > > Really, we should extract all of this dance into a shared class. Maybe not > for > > > this CL, but *cough* technical debt *cough*... Marc, WDYT? > > > > > > Actually, why do we even use authentication here? > > > > Yes we should. :) However, since we're moving to a model where we won't even > use > > the token to fetch the data (because we will instead use the user's history to > > filter the snippets to get relevant results) so that issue will not come up. > > > > And what Marc said -- we needed authentication to access the API previously. > In > > theory that's not the case anymore but I haven't had a chance to test it yet. > I > > have a bug to switch over to not requiring sign in to access ChromeReader > > (crbug/584346). > > So for the record, I'm fine with landing as-is for now, so that we have > something that works :) Acknowledged. https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:28: using NTPSnippetStorage = std::vector<std::unique_ptr<NTPSnippet>>; On 2016/02/10 10:48:44, Bernhard Bauer wrote: > On 2016/02/09 17:38:54, May wrote: > > On 2016/02/08 18:19:27, Bernhard Bauer wrote: > > > This should use scoped_ptr, not std::unique_ptr. > > > > Why should it be a scoped_ptr? We construct the NTPSnippetObject in NTPSnippet > > class and then this class claims ownership of it here. > > Yup, but it's just that we usually use scoped_ptr in Chrome to express that > notion of ownership. Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:686: SnippetsController.get(this).fetchSnippets(false); On 2016/02/10 10:44:43, Marc Treib wrote: > IMO this is fine for now, but could you put in a TODO(treib) to remove it when > the proper fetching logic is in place? Thanks! Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:28: * Creates an NTPSnippetsBridge for getting snippet data for the current user On 2016/02/10 10:44:43, Marc Treib wrote: > nit: -NTP Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:113: Log.e(TAG, "Could not get image thumbnail", e); On 2016/02/10 10:48:44, Bernhard Bauer wrote: > You could combine these these (`catch (MalformedURLException|IOException e)`). I prefer to keep them separate, as the MalformedURLException might be an issue with ChromeReader, for example, or with our json parsing. But I'll change the messages to be more clear. https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:115: Log.e(TAG, "Could not get image thumbnail", e); On 2016/02/10 10:44:43, Marc Treib wrote: > nit: Different messages for the two exceptions? Acknowledged. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:77: observer_.Reset(); On 2016/02/10 10:48:44, Bernhard Bauer wrote: > You might want to remove the service from the |snippets_service_observer_| here. Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.h (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:14: using ntp_snippets::NTPSnippetsService; On 2016/02/10 10:44:43, Marc Treib wrote: > No "using" in headers Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:28: static bool RegisterNTPSnippetsBridge(JNIEnv* env); On 2016/02/10 10:44:43, Marc Treib wrote: > This could just be "Register", since it'll be called as > NTPSnippetsBridge::Register anyway. Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.h:28: static bool RegisterNTPSnippetsBridge(JNIEnv* env); On 2016/02/10 10:48:44, Bernhard Bauer wrote: > Nit: I would even just call it Register() for brevity 😃 Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.cc (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.cc:17: static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) { On 2016/02/10 10:48:44, Bernhard Bauer wrote: > You don't need Init() or Destroy() anymore either -- just make this class purely > static. Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_controller.h (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.h:12: using base::android::JavaParamRef; On 2016/02/10 10:44:43, Marc Treib wrote: > Here too: No "using" in headers Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp_snippets_controller.h:21: static bool RegisterNTPSnippetsController(JNIEnv* env); On 2016/02/10 10:44:43, Marc Treib wrote: > Here too: Simply "Register" is enough IMO Done. https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:54: ->GetSequencedTaskRunnerWithShutdownBehavior( On 2016/02/10 10:48:44, Bernhard Bauer wrote: > Actually, extract this SequencedTaskRunner into a local variable and then pass > it to both -- that way you get the same sequence in both. Whoops, right. My bad. https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:34: using NTPSnippetsFetcherCallbackType = void(const base::FilePath); On 2016/02/10 10:48:44, Bernhard Bauer wrote: > const base::FilePath&. Done. https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:35: using NTPSnippetsFetcherCallback = On 2016/02/10 10:44:43, Marc Treib wrote: > I'd call this "SnippetsAvailableCallback" or something like that - > "NTPSnippetsFetcher" doesn't really add any information, since it's already > inside the class. Done. https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:39: NTPSnippetsFetcher* snippets_fetcher); On 2016/02/10 10:44:43, Marc Treib wrote: > scoped_ptr<NTPSnippetsFetcher> > to make clear that this will take ownership. (You'll have to use std::move in > the .cc file) Done. https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:59: bool LoadFromJSONString(const std::string& str); On 2016/02/10 10:44:43, Marc Treib wrote: > I think this can be private? Done. https://codereview.chromium.org/1677073002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:85: void OnSnippetsDownloaded(const base::FilePath download_path); On 2016/02/10 10:44:43, Marc Treib wrote: > & Done.
LGTM with one last thing I've previously overlooked: https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java (right): https://codereview.chromium.org/1677073002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:113: Log.e(TAG, "Could not get image thumbnail", e); On 2016/02/10 18:16:46, May wrote: > On 2016/02/10 10:48:44, Bernhard Bauer wrote: > > You could combine these these (`catch (MalformedURLException|IOException e)`). > > > I prefer to keep them separate, as the MalformedURLException might be an issue > with ChromeReader, for example, or with our json parsing. But I'll change the > messages to be more clear. Acknowledged. https://codereview.chromium.org/1677073002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1677073002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:40: if (profile) { Actually, could we remove this? I would rather crash if this is null than silently swallowing it.
LGTM too :) https://codereview.chromium.org/1677073002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/1677073002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:24: String[] titles, String[] urls, String[] thumbnailUrls, String[] snippets); Optional nit: "snippets" is overloaded to mean both the whole "title+url+text..." package as well as only the text. Maybe call the text just "text"? https://codereview.chromium.org/1677073002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1677073002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:59: scoped_ptr<ntp_snippets::NTPSnippetsFetcher>( You could use make_scoped_ptr here, then the type doesn't need to be repeated.
https://codereview.chromium.org/1677073002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/1677073002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:24: String[] titles, String[] urls, String[] thumbnailUrls, String[] snippets); On 2016/02/11 09:23:21, Marc Treib wrote: > Optional nit: "snippets" is overloaded to mean both the whole > "title+url+text..." package as well as only the text. Maybe call the text just > "text"? Done. https://codereview.chromium.org/1677073002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1677073002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp_snippets_bridge.cc:40: if (profile) { On 2016/02/10 18:36:20, Bernhard Bauer wrote: > Actually, could we remove this? I would rather crash if this is null than > silently swallowing it. Done. https://codereview.chromium.org/1677073002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1677073002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:59: scoped_ptr<ntp_snippets::NTPSnippetsFetcher>( On 2016/02/11 09:23:21, Marc Treib wrote: > You could use make_scoped_ptr here, then the type doesn't need to be repeated. TIL! Done.
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/1677073002/#ps80001 (title: "Addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677073002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677073002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
maybelle@chromium.org changed reviewers: + rogerta@chromium.org, xunjieli@chromium.org
+rogerta for components/signin and google_apis/gaia/ LGTM. +xunjieli for net/ LGTM. PTAL!
On 2016/02/11 16:05:38, May wrote: > +rogerta for components/signin and google_apis/gaia/ LGTM. > > +xunjieli for net/ LGTM. Hmm.. I don't think there's any file from net/ though. > PTAL!
On 2016/02/11 16:09:05, xunjieli wrote: > On 2016/02/11 16:05:38, May wrote: > > +rogerta for components/signin and google_apis/gaia/ LGTM. > > > > +xunjieli for net/ LGTM. > > Hmm.. I don't think there's any file from net/ though. > > PTAL! I added a new DEPS entry for net/
Friendly ping before US folks go on holiday on Monday!
xunjieli@chromium.org changed reviewers: + rsleevi@chromium.org
On 2016/02/11 16:11:16, May wrote: > On 2016/02/11 16:09:05, xunjieli wrote: > > On 2016/02/11 16:05:38, May wrote: > > > +rogerta for components/signin and google_apis/gaia/ LGTM. > > > > > > +xunjieli for net/ LGTM. > > > > Hmm.. I don't think there's any file from net/ though. > > > PTAL! > > I added a new DEPS entry for net/ I am not sure what is the guideline on adding DEPs on net/, Sleevi, could you help with the review? Thanks.
rsleevi@chromium.org changed reviewers: + davidben@chromium.org
davidben@ should look at this :)
https://codereview.chromium.org/1677073002/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:94: token_service_->AddObserver(this); This will fail if we're already observing the token service (because of a previous StartFetch call). Could you add a bool waiting_for_refresh_token_ or something like that? https://codereview.chromium.org/1677073002/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:42: virtual ~NTPSnippetsFetcher() override; nit: Remove "virtual" - "override" already implies it. (I thought we had a presubmit warning or something for this?)
Friendly ping again for OWNERS review. @Marc: will address this tomorrow.
https://codereview.chromium.org/1677073002/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:94: token_service_->AddObserver(this); On 2016/02/16 09:06:33, Marc Treib wrote: > This will fail if we're already observing the token service (because of a > previous StartFetch call). Could you add a bool waiting_for_refresh_token_ or > something like that? Done. https://codereview.chromium.org/1677073002/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:42: virtual ~NTPSnippetsFetcher() override; On 2016/02/16 09:06:33, Marc Treib wrote: > nit: Remove "virtual" - "override" already implies it. (I thought we had a > presubmit warning or something for this?) Strange, could have sworn I'd been yelled at by the presubmit for that before. Anyway, removed.
maybelle@chromium.org changed reviewers: + noyau@chromium.org
+Eric, PTAL at ios_chrome_ntp_snippets_service_factory.cc
davidben@chromium.org changed reviewers: + mmenke@chromium.org
Because playing hot potato is such fun, Matt, mind skimming this for net DEPS? It's using URLFetcher and friends which I'm much less familiar with these days.
On 2016/02/17 18:41:06, davidben wrote: > Because playing hot potato is such fun, Matt, mind skimming this for net DEPS? > It's using URLFetcher and friends which I'm much less familiar with these days. Sure. I have a bit of a backlog, though, and a security bug on my plate. Try to get to it tomorrow, but want to get the security fix in promptly.
lgtm for deps on signin and gaia. Sorry for delay May.
On 2016/02/17 18:43:14, mmenke wrote: > On 2016/02/17 18:41:06, davidben wrote: > > Because playing hot potato is such fun, Matt, mind skimming this for net DEPS? > > It's using URLFetcher and friends which I'm much less familiar with these > days. > > Sure. I have a bit of a backlog, though, and a security bug on my plate. Try > to get to it tomorrow, but want to get the security fix in promptly. Thanks, Matt! It's been passed around to 4 net/ reviewers over 6+ days, so we'd appreciate a look whenever you can get to it. - Pam
On 2016/02/17 19:36:02, Pam (also PM for reviews) wrote: > On 2016/02/17 18:43:14, mmenke wrote: > > On 2016/02/17 18:41:06, davidben wrote: > > > Because playing hot potato is such fun, Matt, mind skimming this for net > DEPS? > > > It's using URLFetcher and friends which I'm much less familiar with these > > days. > > > > Sure. I have a bit of a backlog, though, and a security bug on my plate. Try > > to get to it tomorrow, but want to get the security fix in promptly. > > Thanks, Matt! It's been passed around to 4 net/ reviewers over 6+ days, so we'd > appreciate a look whenever you can get to it. > > - Pam Sorry about that - we're aware of the problem. A lot of reviews end up in front of David, Sleevi, and I, unfortunately. If people would just stop writing code for a while... :)
https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:45: NOTIMPLEMENTED(); Use PathService::Get(ios::FILE_LOCAL_STATE, &dir)) to put the data in some installation dependent folder. But should you use a profile dependent path instead? Is there different snippets returned to different users, or do you expect all users to get the same snippets? Anyway this would be probably better if the difference was handled in the factory and the path passed in instead of using a #if, and this would make testing simpler. https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:20: DLOG_IF(ERROR, !success) << "Error reading file " << path.LossyDisplayName(); Use DCHECK instead, as I think this will use the |success| variable even in release builds, whereas this will not happen for DLOG_IF. https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:39: scoped_ptr<NTPSnippetsFetcher> snippets_fetcher); The API changed, but the associated tests were not updated. I'm pretty sure they won't compile and run :) The simplest way is probably to create a NTPSnippetsServiceMock that you can inject. https://codereview.chromium.org/1677073002/diff/120001/ios/chrome/browser/ntp... File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1677073002/diff/120001/ios/chrome/browser/ntp... ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:48: } You should probably remove this implementation, now that is is superseded by the one below...
https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:4: #include "components/ntp_snippets/ntp_snippets_fetcher.h" nit: blank line before first include. https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:132: url_fetcher_->Start(); Note that url_fetcher_ has no timeout logic (Other than TCP/HTTP2/QUIC keepalives, if enabled on the platform), so this could just hand indefinitely, like when behind a captive portal that blackholes SSL requests. It also has no size cap, so could happily download a 15 terabyte file, though taking to a trusted server over SSL helps alleviate that potential issue. https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:75: net::URLRequestContextGetter* url_request_context_getter_; You should hold onto a reference to this, or modify your architecture so you don't need to keep a pointer to it, to make sure it always exists when you're fetching something. Note that this doesn't delay teardown of the network stack, but if the network stack is torn down, requests will just cleanly fail, as long as you're not using an invalid URLRequestContextGetter pointer. Since NTPSnippetsFetchers are owned by a service keyed on the profile, they should be deleted just before the URLRequestConext is, but I think it's best not to have that implicit dependency on shutdown ordering, since we can easily avoid it.
blundell@ - PTAL at components/BUILD.gn Thanks! https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:45: NOTIMPLEMENTED(); On 2016/02/17 20:19:47, noyau wrote: > Use PathService::Get(ios::FILE_LOCAL_STATE, &dir)) to put the data in some > installation dependent folder. But should you use a profile dependent path > instead? Is there different snippets returned to different users, or do you > expect all users to get the same snippets? > > Anyway this would be probably better if the difference was handled in the > factory and the path passed in instead of using a #if, and this would make > testing simpler. > > Done. https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:20: DLOG_IF(ERROR, !success) << "Error reading file " << path.LossyDisplayName(); On 2016/02/17 20:19:47, noyau wrote: > Use DCHECK instead, as I think this will use the |success| variable even in > release builds, whereas this will not happen for DLOG_IF. Modified. https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1677073002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:39: scoped_ptr<NTPSnippetsFetcher> snippets_fetcher); On 2016/02/17 20:19:47, noyau wrote: > The API changed, but the associated tests were not updated. I'm pretty sure they > won't compile and run :) > > The simplest way is probably to create a NTPSnippetsServiceMock that you can > inject. So turns out, the test file was not included in BUILD.gn -- which explains why I didn't see any compile errors. I've now added it and updated the tests https://codereview.chromium.org/1677073002/diff/120001/ios/chrome/browser/ntp... File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1677073002/diff/120001/ios/chrome/browser/ntp... ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:48: } On 2016/02/17 20:19:48, noyau wrote: > You should probably remove this implementation, now that is is superseded by the > one below... Saw it after I uploaded.. done. https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:4: #include "components/ntp_snippets/ntp_snippets_fetcher.h" On 2016/02/18 16:51:11, mmenke wrote: > nit: blank line before first include. Done. https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:132: url_fetcher_->Start(); On 2016/02/18 16:51:11, mmenke wrote: > Note that url_fetcher_ has no timeout logic (Other than TCP/HTTP2/QUIC > keepalives, if enabled on the platform), so this could just hand indefinitely, > like when behind a captive portal that blackholes SSL requests. > > It also has no size cap, so could happily download a 15 terabyte file, though > taking to a trusted server over SSL helps alleviate that potential issue. Acknowledged. Is there a different way to implement this, to address those? (Though like you said -- we would only ever fetch from a trusted Google server using this, so the second one is not as big of a concern). https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:75: net::URLRequestContextGetter* url_request_context_getter_; On 2016/02/18 16:51:11, mmenke wrote: > You should hold onto a reference to this, or modify your architecture so you > don't need to keep a pointer to it, to make sure it always exists when you're > fetching something. Note that this doesn't delay teardown of the network stack, > but if the network stack is torn down, requests will just cleanly fail, as long > as you're not using an invalid URLRequestContextGetter pointer. > > Since NTPSnippetsFetchers are owned by a service keyed on the profile, they > should be deleted just before the URLRequestConext is, but I think it's best not > to have that implicit dependency on shutdown ordering, since we can easily avoid > it. Thanks for the explanation, turned into a scoped_refptr
maybelle@chromium.org changed reviewers: + blundell@chromium.org
blundell@ - PTAL at components/BUILD.gn Thanks!
LGTM https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1677073002/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:132: url_fetcher_->Start(); On 2016/02/19 14:49:02, May wrote: > On 2016/02/18 16:51:11, mmenke wrote: > > Note that url_fetcher_ has no timeout logic (Other than TCP/HTTP2/QUIC > > keepalives, if enabled on the platform), so this could just hand indefinitely, > > like when behind a captive portal that blackholes SSL requests. > > > > It also has no size cap, so could happily download a 15 terabyte file, though > > taking to a trusted server over SSL helps alleviate that potential issue. > > Acknowledged. Is there a different way to implement this, to address those? > (Though like you said -- we would only ever fetch from a trusted Google server > using this, so the second one is not as big of a concern). The only way to address the size issue currently is to use the URLRequest interface (Which is a fair bit more complicated to use, and requires you use it on the IO thread). I'd like to get a hard limit in URLFetcher, but given the sheer number of consumers, that's a pretty major task. We could go with a way to set an optional limit until then, but I'd really like to switch to have a sane default limit by default (and it should not be possible to increase it, at least not by much). For the timeout issue, there's no current workaround, other than to do it yourself. The network stack doesn't have any context, so it doesn't know what is reasonable. It could be a hanging get, for instance. We do handle timeouts while establishing a connection, and use QUIC/HTTP2/TCP keepalives, if enabled (TCP keepalives are turned on for desktop, disabled for mobile. Think the others are always on), but nothing beyond that.
//components/BUILD.gn lgtm
ios/ lgtm
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org, rogerta@chromium.org, noyau@chromium.org, mmenke@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/1677073002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677073002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677073002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, rogerta@chromium.org, noyau@chromium.org, treib@chromium.org, mmenke@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/1677073002/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677073002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677073002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, rogerta@chromium.org, noyau@chromium.org, treib@chromium.org, mmenke@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/1677073002/#ps260001 (title: "fix asan")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677073002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677073002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Small nits. https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:21: DLOG(ERROR) << "Error reading file " << path.LossyDisplayName(); I'm not sure this is correct. In a release build the DLOG is completely removed, so the if without curly brackets is potentially very very dangerous. Why don't you like DCHECK? DCHECK(success) << "Error reading..."... You really want to stop in debug rather than putting out a log that everyone will ignore. https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:76: std::string language_code("fr"); Right now it's always passing language code "fr" so you could move this into the CreateSnippetservice function as well.
https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:21: DLOG(ERROR) << "Error reading file " << path.LossyDisplayName(); On 2016/02/23 14:23:45, noyau wrote: > I'm not sure this is correct. In a release build the DLOG is completely removed, > so the if without curly brackets is potentially very very dangerous. I'm fairly sure this works, but we do have DLOG_IF for this kind of thing. > Why don't you like DCHECK? According to the coding style (http://dev.chromium.org/developers/coding-style), (D)CHECKs should be treated like asserts, i.e. they should NEVER fail, even on disk corruption etc. > DCHECK(success) << "Error reading..."... > > You really want to stop in debug rather than putting out a log that everyone > will ignore.
https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:21: DLOG(ERROR) << "Error reading file " << path.LossyDisplayName(); On 2016/02/23 14:23:45, noyau wrote: > I'm not sure this is correct. In a release build the DLOG is completely removed, > so the if without curly brackets is potentially very very dangerous. (Note that the DLOG isn't completely compiled out in a Release build, but it gets turned into a dummy stream that eats everything without printing). > Why don't you like DCHECK? > > DCHECK(success) << "Error reading..."... > > You really want to stop in debug rather than putting out a log that everyone > will ignore.
https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:21: DLOG(ERROR) << "Error reading file " << path.LossyDisplayName(); On 2016/02/23 14:29:50, Marc Treib wrote: > On 2016/02/23 14:23:45, noyau wrote: > > I'm not sure this is correct. In a release build the DLOG is completely > removed, > > so the if without curly brackets is potentially very very dangerous. > > I'm fairly sure this works, but we do have DLOG_IF for this kind of thing. > > > Why don't you like DCHECK? > > According to the coding style (http://dev.chromium.org/developers/coding-style), > (D)CHECKs should be treated like asserts, i.e. they should NEVER fail, even on > disk corruption etc. > > > DCHECK(success) << "Error reading..."... > > > > You really want to stop in debug rather than putting out a log that everyone > > will ignore. > <drive-by> As is, this code seems dangerous: in the !success case, we're returning a string that is presumably garbage without any indication that it's so. As an external observer, it seems like we should either (1) assert that this doesn't happen if we think it should never happen (i.e., DCHECK) or (2) handle it if we think it can happen in legitimate operation (e.g., propagate the success value to our own callers for them to check or whatever).
https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:63: new AccountTrackerService()); Waaait, now these will be deleted when CreateSnippetService returns, but the signin manager will retain raw pointers to them?!
The CQ bit was checked by maybelle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677073002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677073002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:21: DLOG(ERROR) << "Error reading file " << path.LossyDisplayName(); On 2016/02/23 14:29:50, Marc Treib wrote: > On 2016/02/23 14:23:45, noyau wrote: > > I'm not sure this is correct. In a release build the DLOG is completely > removed, > > so the if without curly brackets is potentially very very dangerous. > > I'm fairly sure this works, but we do have DLOG_IF for this kind of thing. > > > Why don't you like DCHECK? > > According to the coding style (http://dev.chromium.org/developers/coding-style), > (D)CHECKs should be treated like asserts, i.e. they should NEVER fail, even on > disk corruption etc. > > > DCHECK(success) << "Error reading..."... > > > > You really want to stop in debug rather than putting out a log that everyone > > will ignore. > I initially had DLOG_IF, but I was only using the success variable for it so it was compiled out in Release (and then clang complained about unused variables, which is how I ended up with this. https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:21: DLOG(ERROR) << "Error reading file " << path.LossyDisplayName(); On 2016/02/23 14:37:48, blundell wrote: > On 2016/02/23 14:29:50, Marc Treib wrote: > > On 2016/02/23 14:23:45, noyau wrote: > > > I'm not sure this is correct. In a release build the DLOG is completely > > removed, > > > so the if without curly brackets is potentially very very dangerous. > > > > I'm fairly sure this works, but we do have DLOG_IF for this kind of thing. > > > > > Why don't you like DCHECK? > > > > According to the coding style > (http://dev.chromium.org/developers/coding-style), > > (D)CHECKs should be treated like asserts, i.e. they should NEVER fail, even on > > disk corruption etc. > > > > > DCHECK(success) << "Error reading..."... > > > > > > You really want to stop in debug rather than putting out a log that > everyone > > > will ignore. > > > > <drive-by> As is, this code seems dangerous: in the !success case, we're > returning a string that is presumably garbage without any indication that it's > so. As an external observer, it seems like we should either (1) assert that this > doesn't happen if we think it should never happen (i.e., DCHECK) or (2) handle > it if we think it can happen in legitimate operation (e.g., propagate the > success value to our own callers for them to check or whatever). Good point. It's definitely a legit operation to fail, and the |data| var could potentially be garbage. I'll change it to propagate the success flag also. https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:63: new AccountTrackerService()); On 2016/02/23 15:28:21, Marc Treib wrote: > Waaait, now these will be deleted when CreateSnippetService returns, but the > signin manager will retain raw pointers to them?! You're right, I didn't look into what the FakeSigninManager actually did. Changed. https://codereview.chromium.org/1677073002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:76: std::string language_code("fr"); On 2016/02/23 14:23:45, noyau wrote: > Right now it's always passing language code "fr" so you could move this into the > CreateSnippetservice function as well. Done.
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, rogerta@chromium.org, noyau@chromium.org, treib@chromium.org, mmenke@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/1677073002/#ps280001 (title: "Review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677073002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677073002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Fetch snippets from ChromeReader and show them on the NTP BUG=584620 ========== to ========== Fetch snippets from ChromeReader and show them on the NTP BUG=584620 Committed: https://crrev.com/bd68437f4f6a2641616b66713bc15b4e287876bd Cr-Commit-Position: refs/heads/master@{#377100} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/bd68437f4f6a2641616b66713bc15b4e287876bd Cr-Commit-Position: refs/heads/master@{#377100} |
