Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2660883002:
Introduce a Doodle Fetcher for NTP (Closed)
|
Created:
3 years, 10 months ago by fhorschig Modified:
3 years, 10 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, donnd+watch_chromium.org, sdefresne+watchlist_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered, Marc Treib, jshneier Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce a Doodle Fetcher for NTP
This fetcher queries the standard Doodle endpoint [1] and
converts the config into a struct.
Attributes are checked for sanity and converted into
fitting formats (e.g. expiry_date based on TTL value).
[1] www.google.com/async/ddljson.
BUG=690467
Review-Url: https://codereview.chromium.org/2660883002
Cr-Commit-Position: refs/heads/master@{#450954}
Committed: https://chromium.googlesource.com/chromium/src/+/5ba57aa78c96c678de1cdf6c34ffe9fefae21a6e
Patch Set 1 #Patch Set 2 : Image URL successfully fetched. #Patch Set 3 : Proper parsing of the ddljson properties. #Patch Set 4 : DoodleState informs about availability of a doodle. #Patch Set 5 : Add namespace and relative URLs #Patch Set 6 : Tests work. #
Total comments: 17
Patch Set 7 : Add injectable JSON parser and tested expiry_time. #
Total comments: 43
Patch Set 8 : Thread-safe multiple callbacks. Weak tests. #
Total comments: 4
Patch Set 9 : Change interface for |FetchDoodle| #
Total comments: 59
Patch Set 10 : CMD line BaseUrl. [Rebase, Weak tests] #
Total comments: 14
Patch Set 11 : Tests for URL and TTL. Stubs/Fakes instead nullptr. #
Total comments: 6
Patch Set 12 : Tests use TestURLFetcher instead of waiting calls. #Patch Set 13 : Rebase. #
Total comments: 28
Patch Set 14 : Set TTL to 30 days #
Total comments: 14
Patch Set 15 : Inline JSON responses in tests #
Total comments: 20
Patch Set 16 : Address comments #
Total comments: 16
Patch Set 17 : Revert and refactor condition for running requests #
Total comments: 2
Patch Set 18 : Address some more comments #
Total comments: 2
Patch Set 19 : Removed unnecessary constructor. #
Total comments: 15
Patch Set 20 : Clean dependencies and s/ptr/scoped_refptr #
Total comments: 2
Patch Set 21 : Remove unused context_getter #
Messages
Total messages: 86 (49 generated)
Description was changed from ========== Introduce a Doodle Fetcher for Desktop and mobile NTP This CL is a draft. No real test coverage, much debugging stuff. BUG= ========== to ========== Introduce a Doodle Fetcher for Desktop and mobile NTP This CL is a draft. No real test coverage, much debugging stuff. BUG= ==========
Patchset #2 (id:20001) has been deleted
treib@chromium.org changed reviewers: + treib@chromium.org
Bunch of small comments :) https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.cc:39: safe_json::SafeJsonParser::Parse(json_sp.as_string(), success_callback, This is okay for now, but eventually it'll have to be injected, because iOS doesn't have SafeJsonParser :-/ https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.cc:63: fetcher_ = URLFetcher::Create(GURL(kDoodleConfigUrl), URLFetcher::GET, this); This should use the passed-in base_url too https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.cc:123: ddljson->GetString("search_url", &temp); What if this (or any of the other url fields) is empty? https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.cc:127: ddljson->GetDouble("time_to_live_ms", &timediff); nit: Add a comment on why double https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.h:29: UNKNOWN, Is this required? Would it ever be returned? https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.h:39: int width; Make a ctor that initializes everything. Recent experience shows that uninitialized variables are Evil :) https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.h:53: std::string doodle_type; What's this? Should it be an enum? https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.h:54: base::Time time_to_live_ms; // The passed "int" doesn't fit int32. nit: Remove comment, it doesn't belong here. Also, "time_to_live" implies it's a TimeDelta. It's actually more of an "expiry_date".
https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.h:53: std::string doodle_type; On 2017/01/31 09:46:34, Marc Treib wrote: > What's this? Should it be an enum? It's populated from this enum: enum DoodleType { SIMPLE = 1; RANDOM = 2; VIDEO = 3; INTERACTIVE = 4; INLINE_INTERACTIVE = 5; SLIDESHOW = 6; }
https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.cc:39: safe_json::SafeJsonParser::Parse(json_sp.as_string(), success_callback, On 2017/01/31 09:46:34, Marc Treib wrote: > This is okay for now, but eventually it'll have to be injected, because iOS > doesn't have SafeJsonParser :-/ Now,there is a constructor that allows injecting a Callback. https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.cc:63: fetcher_ = URLFetcher::Create(GURL(kDoodleConfigUrl), URLFetcher::GET, this); On 2017/01/31 09:46:34, Marc Treib wrote: > This should use the passed-in base_url too Done. https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.cc:123: ddljson->GetString("search_url", &temp); On 2017/01/31 09:46:34, Marc Treib wrote: > What if this (or any of the other url fields) is empty? I now return an empty GURL, so is_valid() will fail. Tests exist. https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.cc:127: ddljson->GetDouble("time_to_live_ms", &timediff); On 2017/01/31 09:46:34, Marc Treib wrote: > nit: Add a comment on why double Done. https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.h:29: UNKNOWN, On 2017/01/31 09:46:34, Marc Treib wrote: > Is this required? Would it ever be returned? Removed. It used to be a default value. https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.h:39: int width; On 2017/01/31 09:46:34, Marc Treib wrote: > Make a ctor that initializes everything. Recent experience shows that > uninitialized variables are Evil :) Done. https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.h:53: std::string doodle_type; On 2017/01/31 09:46:34, Marc Treib wrote: > What's this? Should it be an enum? Why not. https://codereview.chromium.org/2660883002/diff/140001/components/doodle/dood... components/doodle/doodle_fetcher.h:54: base::Time time_to_live_ms; // The passed "int" doesn't fit int32. On 2017/01/31 09:46:34, Marc Treib wrote: > nit: Remove comment, it doesn't belong here. > Also, "time_to_live" implies it's a TimeDelta. It's actually more of an > "expiry_date". Done.
https://codereview.chromium.org/2660883002/diff/180001/components/doodle/OWNERS File components/doodle/OWNERS (right): https://codereview.chromium.org/2660883002/diff/180001/components/doodle/OWNE... components/doodle/OWNERS:1: fhorschig@chromium.org You can't be an owner before you're a committer ;) https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.cc:24: const char kDoodleConfigUrl[] = "/async/ddljson?ogdeb=ct~2000000007"; It's not actually a URL, but a path https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.cc:65: return DoodleType::SIMPLE; Hm. Should we treat all unknown types as SIMPLE? That doesn't seem very safe... https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.cc:134: // ParseJSONCallback Success callback nit: Comment not needed here (you could put something similar in the header though) https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.cc:214: void DoodleFetcher::ParseRelativeUrl(const base::DictionaryValue* dict_value, nit: Could this just return the resulting GURL? https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:51: ~DoodleImage(); nit: ctors first, then dtor https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:58: std::string background_color; If we actually need this, we should find a proper Color type (but I don't think we need it). https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:60: bool is_cta; Hm, what exactly do these two mean? Could they both be true? https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:68: ~DoodleConfig(); Also here: dtor after ctors https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:70: DoodleConfig(const DoodleConfig& config); Is this required? Won't the default ctor actually do the right thing here? If so, remove this and just add a comment // Copying and assignment allowed. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:72: DoodleState state; Hm, so if state isn't AVAILABLE, then all the other fields are meaningless? Should this be outside the config? https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:89: // base_url of a user's account. base_url isn't really a thing, it's google_base_url. But I don't think that needs to be mentioned here. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:102: DoodleFetcher(net::URLRequestContextGetter* download_context, GURL base_url); Eventually, the version without ParseJSONCallback should probably be removed? https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:104: GURL base_url, Hm, could we get the Google base URL within this class, on demand? GoogleURLTracker seems to be there for just that: https://cs.chromium.org/chromium/src/components/google/core/browser/google_ur... (Fun fact: The Google base URL can change at runtime. So if it's passed in, then the client would have to watch for that, and re-build the DoodleFetcher if it changes...) https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:113: void SetClockForTesting(std::unique_ptr<base::Clock> clock) { Haven't we decided against this kind of thing in the last testing meeting? ;) (I don't really mind though. If we pass the clock in the ctor, then we kinda need a factory, which doesn't seem worth the hassle...) https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:125: bool ParseImage(const base::DictionaryValue* image_dict, Could these all be static? Or, better yet, local functions in the .cc (in an anonymous namespace)? nit: If image_dict can't be null, prefer const ref over pointer https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:150: extern const char kDoodleConfigUrl[]; Why is this here? For testing?
https://codereview.chromium.org/2660883002/diff/200001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/200001/components/doodle/dood... components/doodle/doodle_fetcher.cc:102: callback_lock_.Acquire(); Is this required? I don't see why we would ever use one DoodleFetcher instance on multiple threads - with very few exceptions, we generally don't share objects across threads in Chromium. https://codereview.chromium.org/2660883002/diff/200001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/200001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:40: ")]}'{" You could use a raw string literal here, to avoid all the escaping. Example: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/printing/printer...
Patchset #12 (id:240001) has been deleted
Patchset #11 (id:220001) has been deleted
https://codereview.chromium.org/2660883002/diff/180001/components/doodle/OWNERS File components/doodle/OWNERS (right): https://codereview.chromium.org/2660883002/diff/180001/components/doodle/OWNE... components/doodle/OWNERS:1: fhorschig@chromium.org On 2017/01/31 14:46:38, Marc Treib wrote: > You can't be an owner before you're a committer ;) This makes me sad. But okay, I am also never sheriff ... (Removed.) https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.cc:24: const char kDoodleConfigUrl[] = "/async/ddljson?ogdeb=ct~2000000007"; On 2017/01/31 14:46:38, Marc Treib wrote: > It's not actually a URL, but a path Done. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.cc:65: return DoodleType::SIMPLE; On 2017/01/31 14:46:38, Marc Treib wrote: > Hm. Should we treat all unknown types as SIMPLE? That doesn't seem very safe... UNKNOWN Added. This only affects tests and invalid responses that have bigger problems and should be flagged with a DoodleState. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.cc:134: // ParseJSONCallback Success callback On 2017/01/31 14:46:38, Marc Treib wrote: > nit: Comment not needed here (you could put something similar in the header > though) Done. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.cc:214: void DoodleFetcher::ParseRelativeUrl(const base::DictionaryValue* dict_value, On 2017/01/31 14:46:38, Marc Treib wrote: > nit: Could this just return the resulting GURL? Absolutely. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:51: ~DoodleImage(); On 2017/01/31 14:46:38, Marc Treib wrote: > nit: ctors first, then dtor Done. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:58: std::string background_color; On 2017/01/31 14:46:38, Marc Treib wrote: > If we actually need this, we should find a proper Color type (but I don't think > we need it). Removed for now. Are you sure about that? I guess the only use case where it matters is on desktop. We use HTML/CSS on dektop which needs exactly those strings. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:60: bool is_cta; On 2017/01/31 14:46:39, Marc Treib wrote: > Hm, what exactly do these two mean? Could they both be true? They can but for our sample doodle, they never are. is_animated_gif states whether the file is a *.gif is_cta states "Whether this is a call to action. This may mean that there is a play button baked in to the image." [1] ... so i guess it's for videos? [1] There are several sources of truth on cs/is_cta https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:68: ~DoodleConfig(); On 2017/01/31 14:46:38, Marc Treib wrote: > Also here: dtor after ctors Done. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:70: DoodleConfig(const DoodleConfig& config); On 2017/01/31 14:46:39, Marc Treib wrote: > Is this required? Won't the default ctor actually do the right thing here? If > so, remove this and just add a comment > // Copying and assignment allowed. Added that comment to DoodleImage. It's needed for the DoodleConfig, otherwise, it fails with: [chromium-style] Complex class/struct needs an explicit out-of-line copy constructor. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:72: DoodleState state; On 2017/01/31 14:46:38, Marc Treib wrote: > Hm, so if state isn't AVAILABLE, then all the other fields are meaningless? > Should this be outside the config? It's now part of the Callback. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:89: // base_url of a user's account. On 2017/01/31 14:46:39, Marc Treib wrote: > base_url isn't really a thing, it's google_base_url. But I don't think that > needs to be mentioned here. Done. (Deprecated since UrlTracker anyway.) https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:102: DoodleFetcher(net::URLRequestContextGetter* download_context, GURL base_url); On 2017/01/31 14:46:39, Marc Treib wrote: > Eventually, the version without ParseJSONCallback should probably be removed? Done. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:104: GURL base_url, On 2017/01/31 14:46:38, Marc Treib wrote: > Hm, could we get the Google base URL within this class, on demand? > GoogleURLTracker seems to be there for just that: > https://cs.chromium.org/chromium/src/components/google/core/browser/google_ur... > > (Fun fact: The Google base URL can change at runtime. So if it's passed in, then > the client would have to watch for that, and re-build the DoodleFetcher if it > changes...) Okay, we now depend on the tracker to fetch a URL. This means a new DEPS entry as well, which I assume is fine? (//components/google/core/browser:browser) https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:113: void SetClockForTesting(std::unique_ptr<base::Clock> clock) { On 2017/01/31 14:46:39, Marc Treib wrote: > Haven't we decided against this kind of thing in the last testing meeting? ;) > (I don't really mind though. If we pass the clock in the ctor, then we kinda > need a factory, which doesn't seem worth the hassle...) I would like to add a TODO for now as my reasoning was the same. A factory might make sense if the dependencies grow further. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:125: bool ParseImage(const base::DictionaryValue* image_dict, On 2017/01/31 14:46:38, Marc Treib wrote: > Could these all be static? Or, better yet, local functions in the .cc (in an > anonymous namespace)? > > nit: If image_dict can't be null, prefer const ref over pointer These functions depend on base_url_ and clock_. Each function would need four parameters in an unnamed namespace, but if you prefer that over the crowded header, I can do that. nit: Done. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:150: extern const char kDoodleConfigUrl[]; On 2017/01/31 14:46:39, Marc Treib wrote: > Why is this here? For testing? Mainly. https://codereview.chromium.org/2660883002/diff/200001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/200001/components/doodle/dood... components/doodle/doodle_fetcher.cc:102: callback_lock_.Acquire(); On 2017/02/01 10:55:58, Marc Treib wrote: > Is this required? I don't see why we would ever use one DoodleFetcher instance > on multiple threads - with very few exceptions, we generally don't share objects > across threads in Chromium. Removed. https://codereview.chromium.org/2660883002/diff/200001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/200001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:40: ")]}'{" On 2017/02/01 10:55:58, Marc Treib wrote: > You could use a raw string literal here, to avoid all the escaping. > Example: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/printing/printer... Done.
Lots of comments, but mostly small stuff. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/OWNERS File components/doodle/OWNERS (right): https://codereview.chromium.org/2660883002/diff/180001/components/doodle/OWNE... components/doodle/OWNERS:1: fhorschig@chromium.org On 2017/02/01 11:16:13, fhorschig wrote: > On 2017/01/31 14:46:38, Marc Treib wrote: > > You can't be an owner before you're a committer ;) > > This makes me sad. But okay, I am also never sheriff ... > (Removed.) Well, it makes sense: Ownership implies a higher level of trust than committership. But you can probably become a committer soon. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:58: std::string background_color; On 2017/02/01 11:16:13, fhorschig wrote: > On 2017/01/31 14:46:38, Marc Treib wrote: > > If we actually need this, we should find a proper Color type (but I don't > think > > we need it). > > Removed for now. Are you sure about that? > I guess the only use case where it matters is on desktop. > We use HTML/CSS on dektop which needs exactly those strings. Does the desktop NTP actually use the background color value? Let's figure this one out once we actually work on desktop. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:60: bool is_cta; On 2017/02/01 11:16:13, fhorschig wrote: > On 2017/01/31 14:46:39, Marc Treib wrote: > > Hm, what exactly do these two mean? Could they both be true? > > They can but for our sample doodle, they never are. > > is_animated_gif states whether the file is a *.gif > > is_cta states "Whether this is a call to action. This > may mean that there is a play button baked in to the > image." [1] ... so i guess it's for videos? > > [1] There are several sources of truth on cs/is_cta Okay, I'll try to figure it out with the doodle folks. Eventually, we should add a comment here that explains what exactly these fields mean. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:70: DoodleConfig(const DoodleConfig& config); On 2017/02/01 11:16:13, fhorschig wrote: > On 2017/01/31 14:46:39, Marc Treib wrote: > > Is this required? Won't the default ctor actually do the right thing here? If > > so, remove this and just add a comment > > // Copying and assignment allowed. > > Added that comment to DoodleImage. > It's needed for the DoodleConfig, otherwise, it fails with: > [chromium-style] Complex class/struct needs an explicit out-of-line copy > constructor. Ah, I see. Okay then. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:104: GURL base_url, On 2017/02/01 11:16:13, fhorschig wrote: > On 2017/01/31 14:46:38, Marc Treib wrote: > > Hm, could we get the Google base URL within this class, on demand? > > GoogleURLTracker seems to be there for just that: > > > https://cs.chromium.org/chromium/src/components/google/core/browser/google_ur... > > > > (Fun fact: The Google base URL can change at runtime. So if it's passed in, > then > > the client would have to watch for that, and re-build the DoodleFetcher if it > > changes...) > > Okay, we now depend on the tracker to fetch a URL. This means > a new DEPS entry as well, which I assume is fine? > (//components/google/core/browser:browser) Nice! Yes, the dep should be fine. (Let's hope that one isn't "special" on ios...) https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:113: void SetClockForTesting(std::unique_ptr<base::Clock> clock) { On 2017/02/01 11:16:13, fhorschig wrote: > On 2017/01/31 14:46:39, Marc Treib wrote: > > Haven't we decided against this kind of thing in the last testing meeting? ;) > > (I don't really mind though. If we pass the clock in the ctor, then we kinda > > need a factory, which doesn't seem worth the hassle...) > > I would like to add a TODO for now as my reasoning was the same. > A factory might make sense if the dependencies grow further. SGTM https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:125: bool ParseImage(const base::DictionaryValue* image_dict, On 2017/02/01 11:16:13, fhorschig wrote: > On 2017/01/31 14:46:38, Marc Treib wrote: > > Could these all be static? Or, better yet, local functions in the .cc (in an > > anonymous namespace)? > > > > nit: If image_dict can't be null, prefer const ref over pointer > > These functions depend on base_url_ and clock_. > Each function would need four parameters in an unnamed namespace, > but if you prefer that over the crowded header, I can do that. Ah right, makes sense. Then it's fine as is. > nit: Done. https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:150: extern const char kDoodleConfigUrl[]; On 2017/02/01 11:16:13, fhorschig wrote: > On 2017/01/31 14:46:39, Marc Treib wrote: > > Why is this here? For testing? > > Mainly. Then please add a comment saying that. It'd be even nicer if the tests didn't need access to this, but oh well. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/BUIL... File components/doodle/BUILD.gn (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/BUIL... components/doodle/BUILD.gn:13: "//components/google/core/browser:browser", nit: the final ":browser" isn't required https://codereview.chromium.org/2660883002/diff/260001/components/doodle/BUIL... components/doodle/BUILD.gn:26: "//components/google/core/browser:browser", Also here (though is the explicit dependency actually needed at all?) https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:64: base::Optional<DoodleConfig> NoConfig() { I think you can just write base::nullopt instead of this. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:75: : doodle_type(DoodleType::UNKNOWN), expiry_date(base::Time::Now()) {} nit: I'd leave expiry_date without explicit initialization. Then it'll have a special "zero" value, which makes it easier to see that it wasn't properly initialized. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:87: DoodleFetcher::~DoodleFetcher() {} nit: empty line before https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:92: return; // With 2 or more callbacks, a fetcher is already running. nit: I'd formulate this as "If there already was a callback, ..." (Took me a few seconds to understand why "2 or more") https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:94: fetcher_ = URLFetcher::Create(GetGoogleBaseUrl().Resolve(kDoodleConfigPath), Should we DCHECK here that fetcher_ was null? https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:167: if (!(image && image_parent.GetDictionary(image_name, &image_dict))) { You have DCHECKed image before, no reason to null-check again https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:170: image->url = ParseRelativeUrl(*image_dict, "url"); Should this return false if the url is missing or invalid? https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:189: // The JSON doesn't garantuee the number to fit into an int. s/garantuee/guarantee/ https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:190: double ttl; initialize, and/or check if parsing actually worked? https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:215: // tests, too. I think this class should assume that the tracker always exists. It should never be null in live code, and tests should inject a fake. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:104: ParseJSONCallback json_parsing_callback); nit: Pass by const ref https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:109: // provides a valid DoodleConfig object. This is now kinda outdated. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:110: // If a fetch is already running, ...? :) https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:140: // Parameter set from constructor. nitty nit: Parameters, plural :) https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:148: std::list<FinishedCallback> callbacks_; nit: I'd just use a vector. Rule of thumb: Don't use list unless you have a good reason for it. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:31: const char kSampleResponse[] = R"json()]}'{ nitty nit: I'd insert a newline after R"json( to make clear where the actual data begins https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:73: }})json"; similar here: newline after the {{ https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:77: std::string AbsoluteUrl(const std::string& relative_url) { MakeAbsoluteUrl, or just Resolve? https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:100: url_fetcher_factory_(nullptr), Add a comment on what the nullptr is? (I first interpreted this as "url_fetcher_factory_ is a pointer", and wondered where it gets initialized.) https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:101: mock_task_runner_(new base::TestMockTimeTaskRunner()), Hm, I'm not very familiar with this, but it looks like you only need the TestMockTimeTaskRunner if the tests depend on delayed tasks? Actually, I'm not convinced that the tests need a taskrunner at all, since nothing in DoodleFetcher actually posts any tasks. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:109: void RespondWithData(const std::string& data) { nit: Maybe SetResponse or something like that? IMO the current name is a bit misleading, since it implies that it would actually cause a response right now. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:165: base::ThreadTaskRunnerHandle mock_task_runner_handle_; Is this needed?
https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:31: const char kSampleResponse[] = R"json()]}'{ On 2017/02/01 11:53:14, Marc Treib wrote: > nitty nit: I'd insert a newline after R"json( to make clear where the actual > data begins Hm, on second thought: That would put an extra newline into the string, which currently we don't handle correctly :D
https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:217: return google_url_tracker_->google_url(); Turns out the tracker doesn't respect the command line param to override the Google base URL :-/ So we'll have to do something like this first: GURL url = google_util::CommandLineGoogleBaseURL(); if (url.is_valid()) { return url; }
https://codereview.chromium.org/2660883002/diff/260001/components/doodle/BUIL... File components/doodle/BUILD.gn (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/BUIL... components/doodle/BUILD.gn:13: "//components/google/core/browser:browser", On 2017/02/01 11:53:13, Marc Treib wrote: > nit: the final ":browser" isn't required Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/BUIL... components/doodle/BUILD.gn:26: "//components/google/core/browser:browser", On 2017/02/01 11:53:13, Marc Treib wrote: > Also here (though is the explicit dependency actually needed at all?) Not yet. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:64: base::Optional<DoodleConfig> NoConfig() { On 2017/02/01 11:53:14, Marc Treib wrote: > I think you can just write base::nullopt instead of this. Nice. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:75: : doodle_type(DoodleType::UNKNOWN), expiry_date(base::Time::Now()) {} On 2017/02/01 11:53:13, Marc Treib wrote: > nit: I'd leave expiry_date without explicit initialization. Then it'll have a > special "zero" value, which makes it easier to see that it wasn't properly > initialized. Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:87: DoodleFetcher::~DoodleFetcher() {} On 2017/02/01 11:53:14, Marc Treib wrote: > nit: empty line before Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:92: return; // With 2 or more callbacks, a fetcher is already running. On 2017/02/01 11:53:14, Marc Treib wrote: > nit: I'd formulate this as "If there already was a callback, ..." > (Took me a few seconds to understand why "2 or more") Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:94: fetcher_ = URLFetcher::Create(GetGoogleBaseUrl().Resolve(kDoodleConfigPath), On 2017/02/01 11:53:13, Marc Treib wrote: > Should we DCHECK here that fetcher_ was null? Okay. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:167: if (!(image && image_parent.GetDictionary(image_name, &image_dict))) { On 2017/02/01 11:53:13, Marc Treib wrote: > You have DCHECKed image before, no reason to null-check again True. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:170: image->url = ParseRelativeUrl(*image_dict, "url"); On 2017/02/01 11:53:14, Marc Treib wrote: > Should this return false if the url is missing or invalid? Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:189: // The JSON doesn't garantuee the number to fit into an int. On 2017/02/01 11:53:14, Marc Treib wrote: > s/garantuee/guarantee/ Thank you https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:190: double ttl; On 2017/02/01 11:53:13, Marc Treib wrote: > initialize, and/or check if parsing actually worked? Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:215: // tests, too. On 2017/02/01 11:53:14, Marc Treib wrote: > I think this class should assume that the tracker always exists. It should never > be null in live code, and tests should inject a fake. Yes, it's a TODO I have in tests. Adding it here, too. Although I am a little suspicious about every KeyedService that relies on profiles. We have some cases where they just become nullptr when not properly unregistered. (Do you remember the crashes in NTPSnippetsBridge?) https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:217: return google_url_tracker_->google_url(); On 2017/02/01 15:22:02, Marc Treib wrote: > Turns out the tracker doesn't respect the command line param to override the > Google base URL :-/ > > So we'll have to do something like this first: > > GURL url = google_util::CommandLineGoogleBaseURL(); > if (url.is_valid()) { > return url; > } Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:104: ParseJSONCallback json_parsing_callback); On 2017/02/01 11:53:14, Marc Treib wrote: > nit: Pass by const ref Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:109: // provides a valid DoodleConfig object. On 2017/02/01 11:53:14, Marc Treib wrote: > This is now kinda outdated. Updated. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:110: // If a fetch is already running, On 2017/02/01 11:53:14, Marc Treib wrote: > ...? :) Looks like an urgent lunch break ... Added the rest. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:140: // Parameter set from constructor. On 2017/02/01 11:53:14, Marc Treib wrote: > nitty nit: Parameters, plural :) Wow, I should really look at my comments more closely. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.h:148: std::list<FinishedCallback> callbacks_; On 2017/02/01 11:53:14, Marc Treib wrote: > nit: I'd just use a vector. Rule of thumb: Don't use list unless you have a good > reason for it. Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:31: const char kSampleResponse[] = R"json()]}'{ On 2017/02/01 14:08:05, Marc Treib wrote: > On 2017/02/01 11:53:14, Marc Treib wrote: > > nitty nit: I'd insert a newline after R"json( to make clear where the actual > > data begins > > Hm, on second thought: That would put an extra newline into the string, which > currently we don't handle correctly :D I guess it would be weird to add code that just unbreaks tests and has no productive value. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:73: }})json"; On 2017/02/01 11:53:14, Marc Treib wrote: > similar here: newline after Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:77: std::string AbsoluteUrl(const std::string& relative_url) { On 2017/02/01 11:53:14, Marc Treib wrote: > MakeAbsoluteUrl, or just Resolve? I am ashamed. Changed. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:100: url_fetcher_factory_(nullptr), On 2017/02/01 11:53:14, Marc Treib wrote: > Add a comment on what the nullptr is? (I first interpreted this as > "url_fetcher_factory_ is a pointer", and wondered where it gets initialized.) Comment added. I will probably use a failing fake default when testing the BaseUrl functionality. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:101: mock_task_runner_(new base::TestMockTimeTaskRunner()), On 2017/02/01 11:53:14, Marc Treib wrote: > Hm, I'm not very familiar with this, but it looks like you only need the > TestMockTimeTaskRunner if the tests depend on delayed tasks? > Actually, I'm not convinced that the tests need a taskrunner at all, since > nothing in DoodleFetcher actually posts any tasks. The URLFetcher posts tasks and needs a context. The TestMockTimeTaskRunner also provides me with that context and also allows me to use its MockClock. If you insist, I could try using a TestSimpleTaskRunner and look for another MockClock. (I think net:: has one, but I wouldn't like to depend on more classes than I absolutely need to). https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:109: void RespondWithData(const std::string& data) { On 2017/02/01 11:53:14, Marc Treib wrote: > nit: Maybe SetResponse or something like that? IMO the current name is a bit > misleading, since it implies that it would actually cause a response right now. Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:165: base::ThreadTaskRunnerHandle mock_task_runner_handle_; On 2017/02/01 11:53:14, Marc Treib wrote: > Is this needed? Yes. (Its constructor replace the base::ThreadTaskRunnerHandle. That way, when the static base::ThreadTaskRunnerHandle::Get() is called, it will use the handle of mock_task_runner_.)
https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:215: // tests, too. On 2017/02/03 13:21:04, fhorschig wrote: > On 2017/02/01 11:53:14, Marc Treib wrote: > > I think this class should assume that the tracker always exists. It should > never > > be null in live code, and tests should inject a fake. > > Yes, it's a TODO I have in tests. > Adding it here, too. > > Although I am a little suspicious about every KeyedService > that relies on profiles. > We have some cases where they just become nullptr when not properly > unregistered. (Do you remember the crashes in NTPSnippetsBridge?) I'm not sure I follow. They don't "become" nullptr; once initialized, they stay around until the profile is destroyed. It's true that they can be null in some cases; incognito is a common example, though I'm not sure if that applies here. Either way, we should decide whether this can be null or not, and document the decision (with DCHECK and/or comments). (FWIW, the crashes in the bridge (or at least some of them, namely the hard-to-track-down ones) weren't caused by a null pointer, but rather by an *uninitialized" pointer. So null checks didn't help :D) https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:31: const char kSampleResponse[] = R"json()]}'{ On 2017/02/03 13:21:05, fhorschig wrote: > On 2017/02/01 14:08:05, Marc Treib wrote: > > On 2017/02/01 11:53:14, Marc Treib wrote: > > > nitty nit: I'd insert a newline after R"json( to make clear where the actual > > > data begins > > > > Hm, on second thought: That would put an extra newline into the string, which > > currently we don't handle correctly :D > > I guess it would be weird to add code that just unbreaks tests and has no > productive value. Well, we could decide that it's okay if the response contains whitespace before the preamble. Then it wouldn't strictly be test-only code. But I don't feel strongly either way. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:100: url_fetcher_factory_(nullptr), On 2017/02/03 13:21:05, fhorschig wrote: > On 2017/02/01 11:53:14, Marc Treib wrote: > > Add a comment on what the nullptr is? (I first interpreted this as > > "url_fetcher_factory_ is a pointer", and wondered where it gets initialized.) > > Comment added. I will probably use a failing fake default > when testing the BaseUrl functionality. FWIW, there's two different versions of URLFetcher things for testing, the "Fake" ones and the "Test" ones. I *think* the other one doesn't need all the TaskRunner stuff. Let's maybe discuss this next week when you're back. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:101: mock_task_runner_(new base::TestMockTimeTaskRunner()), On 2017/02/03 13:21:05, fhorschig wrote: > On 2017/02/01 11:53:14, Marc Treib wrote: > > Hm, I'm not very familiar with this, but it looks like you only need the > > TestMockTimeTaskRunner if the tests depend on delayed tasks? > > Actually, I'm not convinced that the tests need a taskrunner at all, since > > nothing in DoodleFetcher actually posts any tasks. > > The URLFetcher posts tasks and needs a context. > The TestMockTimeTaskRunner also provides me with that context > and also allows me to use its MockClock. > > If you insist, I could try using a TestSimpleTaskRunner and look for > another MockClock. > > (I think net:: has one, but I wouldn't like to depend on more classes > than I absolutely need to). Couldn't you just instantiate a SimpleTestClock yourself? Generally, I'm a bit wary of unit tests that need task runners, message loops, RunUntilIdle and all that stuff. Sometimes it can't be avoided, but I think here it maybe can. Anyway, this mostly boils down to the comment above. Let's discuss in person. https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:195: } The old search_provider_logos code also clamps the TTL to some reasonable max range, so that if we once get a bad value (very far in the future), that doodle won't be shown forever and ever. Maybe we should do something similar? (Not sure if it's necessary) https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:212: std::move(callback).Run(state, std::move(config)); This will break if there's more than one callback: The second one won't get a config anymore, since it's already been moved out. Add a test for this? :) https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:220: // TODO(fhorschig): DCHECK(google_url_tracker_) and inject a fake in tests. The nice thing is that we won't need the kDefaultGoogleHomepage anymore https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:224: GURL cmd_line_url = google_util::CommandLineGoogleBaseURL(); This needs to go before the tracker.
Patchset #13 (id:300001) has been deleted
https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/180001/components/doodle/dood... components/doodle/doodle_fetcher.h:150: extern const char kDoodleConfigUrl[]; On 2017/02/01 11:53:13, Marc Treib wrote: > On 2017/02/01 11:16:13, fhorschig wrote: > > On 2017/01/31 14:46:39, Marc Treib wrote: > > > Why is this here? For testing? > > > > Mainly. > > Then please add a comment saying that. It'd be even nicer if the tests didn't > need access to this, but oh well. Removed. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher.cc:215: // tests, too. On 2017/02/03 15:45:48, Marc Treib wrote: > On 2017/02/03 13:21:04, fhorschig wrote: > > On 2017/02/01 11:53:14, Marc Treib wrote: > > > I think this class should assume that the tracker always exists. It should > > never > > > be null in live code, and tests should inject a fake. > > > > Yes, it's a TODO I have in tests. > > Adding it here, too. > > > > Although I am a little suspicious about every KeyedService > > that relies on profiles. > > We have some cases where they just become nullptr when not properly > > unregistered. (Do you remember the crashes in NTPSnippetsBridge?) > > I'm not sure I follow. They don't "become" nullptr; once initialized, they stay > around until the profile is destroyed. > It's true that they can be null in some cases; incognito is a common example, > though I'm not sure if that applies here. > Either way, we should decide whether this can be null or not, and document the > decision (with DCHECK and/or comments). > > (FWIW, the crashes in the bridge (or at least some of them, namely the > hard-to-track-down ones) weren't caused by a null pointer, but rather by an > *uninitialized" pointer. So null checks didn't help :D) Thank you for the clarification. (As discussed offline, we use the nullptr for integration tests using the tracker factory, which will then return a nullptr.) https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:195: } On 2017/02/03 15:45:48, Marc Treib wrote: > The old search_provider_logos code also clamps the TTL to some reasonable max > range, so that if we once get a bad value (very far in the future), that doodle > won't be shown forever and ever. Maybe we should do something similar? (Not sure > if it's necessary) It's easy to do but hard to define "too long". The 125th birthday of doodle would be there too long if it stays for a week. The olympic fruits doodle would be rightfully there for three weeks. There are checks and tests verifying that the doodle doesn't exceeds 30 days or expires back in time. https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:212: std::move(callback).Run(state, std::move(config)); On 2017/02/03 15:45:48, Marc Treib wrote: > This will break if there's more than one callback: The second one won't get a > config anymore, since it's already been moved out. > Add a test for this? :) It's const& now. There is no need to move that at all. https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:220: // TODO(fhorschig): DCHECK(google_url_tracker_) and inject a fake in tests. On 2017/02/03 15:45:48, Marc Treib wrote: > The nice thing is that we won't need the kDefaultGoogleHomepage anymore It was already gone, resp. replaced by the public default of the GoogleBaseUrlTracker. https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:224: GURL cmd_line_url = google_util::CommandLineGoogleBaseURL(); On 2017/02/03 15:45:48, Marc Treib wrote: > This needs to go before the tracker. Done.
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some last comments; looking pretty good now! One more thing to do is to replace all those LOG(ERROR)s with (e.g.) DLOG(WARNING). https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:195: } On 2017/02/06 11:48:22, fhorschig wrote: > On 2017/02/03 15:45:48, Marc Treib wrote: > > The old search_provider_logos code also clamps the TTL to some reasonable max > > range, so that if we once get a bad value (very far in the future), that > doodle > > won't be shown forever and ever. Maybe we should do something similar? (Not > sure > > if it's necessary) > > It's easy to do but hard to define "too long". > The 125th birthday of doodle would be there too long if it stays > for a week. > The olympic fruits doodle would be rightfully there for three weeks. > > There are checks and tests verifying that the doodle doesn't exceeds > 30 days or expires back in time. I think just replicating the 30 day limit is fine (it seems to have worked well in practice so far). https://codereview.chromium.org/2660883002/diff/320001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/320001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:41: "interactive_html":"\u003cscript\u003e\u003c\/script\u003e", Are the Unicode escape sequences required?! https://codereview.chromium.org/2660883002/diff/320001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:107: // Required to instantiate a BaseUrlTracker in GoogleURLTracker::UNIT_TEST_MODE. s/BaseUrlTracker/GoogleURLTracker/ ? UNIT_TEST_MODE brrrrr... but since we do have a tracker in tests, it looks like the null check in the fetcher isn't actually required? (If any other tests want to instantiate a fetcher, then they'll also have to provide a tracker. IMO that's fine.) https://codereview.chromium.org/2660883002/diff/320001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:159: mock_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(80082)); I just now understood this particular number. Might want to choose a different one... ;)
jshneier@google.com changed reviewers: + jshneier@google.com
https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:195: } On 2017/02/06 17:26:35, Marc Treib wrote: > On 2017/02/06 11:48:22, fhorschig wrote: > > On 2017/02/03 15:45:48, Marc Treib wrote: > > > The old search_provider_logos code also clamps the TTL to some reasonable > max > > > range, so that if we once get a bad value (very far in the future), that > > doodle > > > won't be shown forever and ever. Maybe we should do something similar? (Not > > sure > > > if it's necessary) > > > > It's easy to do but hard to define "too long". > > The 125th birthday of doodle would be there too long if it stays > > for a week. > > The olympic fruits doodle would be rightfully there for three weeks. > > > > There are checks and tests verifying that the doodle doesn't exceeds > > 30 days or expires back in time. > > I think just replicating the 30 day limit is fine (it seems to have worked well > in practice so far). 1 day should be sufficient in just about all cases. What's the behavior wrt the ttl? Does it still fetch a new config every time, or does it cache it for this amount of time? Is the ttl only for cases where you go offline?
https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:195: } On 2017/02/06 19:02:40, jshneier wrote: > On 2017/02/06 17:26:35, Marc Treib wrote: > > On 2017/02/06 11:48:22, fhorschig wrote: > > > On 2017/02/03 15:45:48, Marc Treib wrote: > > > > The old search_provider_logos code also clamps the TTL to some reasonable > > max > > > > range, so that if we once get a bad value (very far in the future), that > > > doodle > > > > won't be shown forever and ever. Maybe we should do something similar? > (Not > > > sure > > > > if it's necessary) > > > > > > It's easy to do but hard to define "too long". > > > The 125th birthday of doodle would be there too long if it stays > > > for a week. > > > The olympic fruits doodle would be rightfully there for three weeks. > > > > > > There are checks and tests verifying that the doodle doesn't exceeds > > > 30 days or expires back in time. > > > > I think just replicating the 30 day limit is fine (it seems to have worked > well > > in practice so far). > > 1 day should be sufficient in just about all cases. What's the behavior wrt the > ttl? Does it still fetch a new config every time, or does it cache it for this > amount of time? Is the ttl only for cases where you go offline? IIUC, the current behavior is: On each request from the UI, hand out the old config unless it's expired, but then immediately fetch an update. So I think the actual TTL limit doesn't matter too much.
Patchset #14 (id:340001) has been deleted
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
As soon as there is an "interactive_js" key, it will be parsed. (Also LOGs replaced by DLOG) https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:31: const char kSampleResponse[] = R"json()]}'{ On 2017/02/03 15:45:48, Marc Treib wrote: > On 2017/02/03 13:21:05, fhorschig wrote: > > On 2017/02/01 14:08:05, Marc Treib wrote: > > > On 2017/02/01 11:53:14, Marc Treib wrote: > > > > nitty nit: I'd insert a newline after R"json( to make clear where the > actual > > > > data begins > > > > > > Hm, on second thought: That would put an extra newline into the string, > which > > > currently we don't handle correctly :D > > > > I guess it would be weird to add code that just unbreaks tests and has no > > productive value. > > Well, we could decide that it's okay if the response contains whitespace before > the preamble. Then it wouldn't strictly be test-only code. But I don't feel > strongly either way. Acknowledged. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:100: url_fetcher_factory_(nullptr), On 2017/02/03 15:45:48, Marc Treib wrote: > On 2017/02/03 13:21:05, fhorschig wrote: > > On 2017/02/01 11:53:14, Marc Treib wrote: > > > Add a comment on what the nullptr is? (I first interpreted this as > > > "url_fetcher_factory_ is a pointer", and wondered where it gets > initialized.) > > > > Comment added. I will probably use a failing fake default > > when testing the BaseUrl functionality. > > FWIW, there's two different versions of URLFetcher things for testing, the > "Fake" ones and the "Test" ones. I *think* the other one doesn't need all the > TaskRunner stuff. > > Let's maybe discuss this next week when you're back. Done. https://codereview.chromium.org/2660883002/diff/260001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:101: mock_task_runner_(new base::TestMockTimeTaskRunner()), On 2017/02/03 15:45:48, Marc Treib wrote: > On 2017/02/03 13:21:05, fhorschig wrote: > > On 2017/02/01 11:53:14, Marc Treib wrote: > > > Hm, I'm not very familiar with this, but it looks like you only need the > > > TestMockTimeTaskRunner if the tests depend on delayed tasks? > > > Actually, I'm not convinced that the tests need a taskrunner at all, since > > > nothing in DoodleFetcher actually posts any tasks. > > > > The URLFetcher posts tasks and needs a context. > > The TestMockTimeTaskRunner also provides me with that context > > and also allows me to use its MockClock. > > > > If you insist, I could try using a TestSimpleTaskRunner and look for > > another MockClock. > > > > (I think net:: has one, but I wouldn't like to depend on more classes > > than I absolutely need to). > > Couldn't you just instantiate a SimpleTestClock yourself? > > Generally, I'm a bit wary of unit tests that need task runners, message loops, > RunUntilIdle and all that stuff. Sometimes it can't be avoided, but I think here > it maybe can. > > Anyway, this mostly boils down to the comment above. Let's discuss in person. As discussed, tests redesigned to use TestUrlFetcherFactory instead. https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:195: } On 2017/02/07 09:21:09, Marc Treib wrote: > On 2017/02/06 19:02:40, jshneier wrote: > > On 2017/02/06 17:26:35, Marc Treib wrote: > > > On 2017/02/06 11:48:22, fhorschig wrote: > > > > On 2017/02/03 15:45:48, Marc Treib wrote: > > > > > The old search_provider_logos code also clamps the TTL to some > reasonable > > > max > > > > > range, so that if we once get a bad value (very far in the future), that > > > > doodle > > > > > won't be shown forever and ever. Maybe we should do something similar? > > (Not > > > > sure > > > > > if it's necessary) > > > > > > > > It's easy to do but hard to define "too long". > > > > The 125th birthday of doodle would be there too long if it stays > > > > for a week. > > > > The olympic fruits doodle would be rightfully there for three weeks. > > > > > > > > There are checks and tests verifying that the doodle doesn't exceeds > > > > 30 days or expires back in time. > > > > > > I think just replicating the 30 day limit is fine (it seems to have worked > > well > > > in practice so far). > > > > 1 day should be sufficient in just about all cases. What's the behavior wrt > the > > ttl? Does it still fetch a new config every time, or does it cache it for > this > > amount of time? Is the ttl only for cases where you go offline? > > IIUC, the current behavior is: On each request from the UI, hand out the old > config unless it's expired, but then immediately fetch an update. So I think the > actual TTL limit doesn't matter too much. Changed to 1 day. https://codereview.chromium.org/2660883002/diff/320001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/320001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:41: "interactive_html":"\u003cscript\u003e\u003c\/script\u003e", On 2017/02/06 17:26:36, Marc Treib wrote: > Are the Unicode escape sequences required?! No. But they are part of the response and have to work anyway. https://codereview.chromium.org/2660883002/diff/320001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:107: // Required to instantiate a BaseUrlTracker in GoogleURLTracker::UNIT_TEST_MODE. On 2017/02/06 17:26:36, Marc Treib wrote: > s/BaseUrlTracker/GoogleURLTracker/ ? > > UNIT_TEST_MODE brrrrr... but since we do have a tracker in tests, it looks like > the null check in the fetcher isn't actually required? > (If any other tests want to instantiate a fetcher, then they'll also have to > provide a tracker. IMO that's fine.) Okay, removed. https://codereview.chromium.org/2660883002/diff/320001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:159: mock_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(80082)); On 2017/02/06 17:26:35, Marc Treib wrote: > I just now understood this particular number. Might want to choose a different > one... ;) Changed. Sorry, couldn't resist...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm officially nitpicking now ;) https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:195: } On 2017/02/08 09:10:23, fhorschig wrote: > On 2017/02/07 09:21:09, Marc Treib wrote: > > On 2017/02/06 19:02:40, jshneier wrote: > > > On 2017/02/06 17:26:35, Marc Treib wrote: > > > > On 2017/02/06 11:48:22, fhorschig wrote: > > > > > On 2017/02/03 15:45:48, Marc Treib wrote: > > > > > > The old search_provider_logos code also clamps the TTL to some > > reasonable > > > > max > > > > > > range, so that if we once get a bad value (very far in the future), > that > > > > > doodle > > > > > > won't be shown forever and ever. Maybe we should do something similar? > > > (Not > > > > > sure > > > > > > if it's necessary) > > > > > > > > > > It's easy to do but hard to define "too long". > > > > > The 125th birthday of doodle would be there too long if it stays > > > > > for a week. > > > > > The olympic fruits doodle would be rightfully there for three weeks. > > > > > > > > > > There are checks and tests verifying that the doodle doesn't exceeds > > > > > 30 days or expires back in time. > > > > > > > > I think just replicating the 30 day limit is fine (it seems to have worked > > > well > > > > in practice so far). > > > > > > 1 day should be sufficient in just about all cases. What's the behavior wrt > > the > > > ttl? Does it still fetch a new config every time, or does it cache it for > > this > > > amount of time? Is the ttl only for cases where you go offline? > > > > IIUC, the current behavior is: On each request from the UI, hand out the old > > config unless it's expired, but then immediately fetch an update. So I think > the > > actual TTL limit doesn't matter too much. > > Changed to 1 day. TBH, I'd prefer to keep the previous numbers. There's a small chance this change will break something unexpectedly. Let's try to keep the differences between the old and new systems limited to only the things we explicitly want to change. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher.cc:84: weak_ptr_factory_(this) {} DCHECK(google_url_tracker_) ? (To make the requirements clear) https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher.cc:203: DLOG(WARNING) << "Clamping Doodle image TTL to 30 days!"; nit: Comment is inaccurate now (but per my other comment, maybe we should stay with 30 days anyway) https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher.h:77: std::string interactive_js; Note that as of now, the interactive_js is just an idea. It's fine to have it here while the CL is in "prototype" state, but we'll have to revisit this before landing. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:134: last_response_(base::nullopt), Isn't this the default value anyway? https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:136: base::ThreadTaskRunnerHandle::Get())), I think it'd be clearer to explicitly pass in the TaskRunner from message_loop_ (it should work out to the exact same thing, but IMO makes clearer what's going on). https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:146: doodle_fetcher_.SetClockForTesting(std::move(clock)); I guess we never need to set anything in the clock we pass to the fetcher? https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:172: // All created TestURLFetchers have ID 0 by default. I guess there's never more than one URLFetcher at a time? More to the point: What would happen if there's more than one? https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:178: void TriggerFetch() { I think this is a case where it's better to put this code into the test itself. IMO the helper makes it harder to follow the tests. If we remove this, then also the last_* members shouldn't be required anymore. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:216: EXPECT_TRUE(last_response().has_value()); This is what I mean above: Here we're EXPECTing stuff that isn't mentioned anywhere in the test before. So it's not really clear why the setup above should produce these results. WDYT? (OTOH, I think the Respond* helpers are fine. Those abstract away details which aren't really interesting for the test itself.) https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:219: TEST_F(DoodleFetcherTest, ReturnsFrom404FetchWithError) { RespondWithError doesn't actually produce a 404. How about passing the error code to RespondWithError as a param? https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:242: EXPECT_THAT(config.fullpage_interactive_url, Eq(GURL())); nit: EXPECT_TRUE(...is_empty()) ? https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:359: Eq(GURL(GoogleURLTracker::kDefaultGoogleHomepage) google_url_tracker_->google_url() ?
https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/280001/components/doodle/dood... components/doodle/doodle_fetcher.cc:195: } On 2017/02/08 10:51:36, Marc Treib wrote: > On 2017/02/08 09:10:23, fhorschig wrote: > > On 2017/02/07 09:21:09, Marc Treib wrote: > > > On 2017/02/06 19:02:40, jshneier wrote: > > > > On 2017/02/06 17:26:35, Marc Treib wrote: > > > > > On 2017/02/06 11:48:22, fhorschig wrote: > > > > > > On 2017/02/03 15:45:48, Marc Treib wrote: > > > > > > > The old search_provider_logos code also clamps the TTL to some > > > reasonable > > > > > max > > > > > > > range, so that if we once get a bad value (very far in the future), > > that > > > > > > doodle > > > > > > > won't be shown forever and ever. Maybe we should do something > similar? > > > > (Not > > > > > > sure > > > > > > > if it's necessary) > > > > > > > > > > > > It's easy to do but hard to define "too long". > > > > > > The 125th birthday of doodle would be there too long if it stays > > > > > > for a week. > > > > > > The olympic fruits doodle would be rightfully there for three weeks. > > > > > > > > > > > > There are checks and tests verifying that the doodle doesn't exceeds > > > > > > 30 days or expires back in time. > > > > > > > > > > I think just replicating the 30 day limit is fine (it seems to have > worked > > > > well > > > > > in practice so far). > > > > > > > > 1 day should be sufficient in just about all cases. What's the behavior > wrt > > > the > > > > ttl? Does it still fetch a new config every time, or does it cache it for > > > this > > > > amount of time? Is the ttl only for cases where you go offline? > > > > > > IIUC, the current behavior is: On each request from the UI, hand out the old > > > config unless it's expired, but then immediately fetch an update. So I think > > the > > > actual TTL limit doesn't matter too much. > > > > Changed to 1 day. > > TBH, I'd prefer to keep the previous numbers. There's a small chance this change > will break something unexpectedly. Let's try to keep the differences between the > old and new systems limited to only the things we explicitly want to change. Done. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher.cc:84: weak_ptr_factory_(this) {} On 2017/02/08 10:51:36, Marc Treib wrote: > DCHECK(google_url_tracker_) ? > (To make the requirements clear) Done. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher.cc:203: DLOG(WARNING) << "Clamping Doodle image TTL to 30 days!"; On 2017/02/08 10:51:36, Marc Treib wrote: > nit: Comment is inaccurate now (but per my other comment, maybe we should stay > with 30 days anyway) It's 30 days again. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher.h:77: std::string interactive_js; On 2017/02/08 10:51:36, Marc Treib wrote: > Note that as of now, the interactive_js is just an idea. It's fine to have it > here while the CL is in "prototype" state, but we'll have to revisit this before > landing. Okay. Removing this is as easy as to add this. So if you think that introducing this in a follow-up CL when the attribute actually exists makes more sense, let me know. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:134: last_response_(base::nullopt), On 2017/02/08 10:51:36, Marc Treib wrote: > Isn't this the default value anyway? Documentation says you are right. Removed. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:136: base::ThreadTaskRunnerHandle::Get())), On 2017/02/08 10:51:36, Marc Treib wrote: > I think it'd be clearer to explicitly pass in the TaskRunner from message_loop_ > (it should work out to the exact same thing, but IMO makes clearer what's going > on). Reverted to how I did this intermediately. It does work the same way. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:146: doodle_fetcher_.SetClockForTesting(std::move(clock)); On 2017/02/08 10:51:37, Marc Treib wrote: > I guess we never need to set anything in the clock we pass to the fetcher? Exactly. I just noticed that we have exactly one fetcher that lives for the duration of the test, so there is no reason to make a copy of the clock. A pointer to this clock suffices. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:172: // All created TestURLFetchers have ID 0 by default. On 2017/02/08 10:51:36, Marc Treib wrote: > I guess there's never more than one URLFetcher at a time? > More to the point: What would happen if there's more than one? A test would break (RespondsToMultipleRequestsWithSameFetcher) and also the old fetcher would silently be overridden. Other tests would break but not crash. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:178: void TriggerFetch() { On 2017/02/08 10:51:36, Marc Treib wrote: > I think this is a case where it's better to put this code into the test itself. > IMO the helper makes it harder to follow the tests. > If we remove this, then also the last_* members shouldn't be required anymore. I did this to make the tests readable like natural language :/ It's now inlined but the callback is still wrapped. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:216: EXPECT_TRUE(last_response().has_value()); On 2017/02/08 10:51:37, Marc Treib wrote: > This is what I mean above: Here we're EXPECTing stuff that isn't mentioned > anywhere in the test before. So it's not really clear why the setup above should > produce these results. > WDYT? > > (OTOH, I think the Respond* helpers are fine. Those abstract away details which > aren't really interesting for the test itself.) I think I get your thought. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:219: TEST_F(DoodleFetcherTest, ReturnsFrom404FetchWithError) { On 2017/02/08 10:51:36, Marc Treib wrote: > RespondWithError doesn't actually produce a 404. > How about passing the error code to RespondWithError as a param? Done. (And thanks for pointing out the wrong code.) https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:242: EXPECT_THAT(config.fullpage_interactive_url, Eq(GURL())); On 2017/02/08 10:51:37, Marc Treib wrote: > nit: EXPECT_TRUE(...is_empty()) ? Done. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:359: Eq(GURL(GoogleURLTracker::kDefaultGoogleHomepage) On 2017/02/08 10:51:36, Marc Treib wrote: > google_url_tracker_->google_url() ? Done. Kind of.
Thanks! LGTM with some more test nits/suggestions (but please hold off on landing this for now). https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:172: // All created TestURLFetchers have ID 0 by default. On 2017/02/08 18:37:25, fhorschig wrote: > On 2017/02/08 10:51:36, Marc Treib wrote: > > I guess there's never more than one URLFetcher at a time? > > More to the point: What would happen if there's more than one? > > A test would break (RespondsToMultipleRequestsWithSameFetcher) > and also the old fetcher would silently be overridden. > Other tests would break but not crash. Thanks! (I was mostly wondering what GetFetcherByID would return - I guess it's just the most-recently-created one.) https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:178: void TriggerFetch() { On 2017/02/08 18:37:26, fhorschig wrote: > On 2017/02/08 10:51:36, Marc Treib wrote: > > I think this is a case where it's better to put this code into the test > itself. > > IMO the helper makes it harder to follow the tests. > > If we remove this, then also the last_* members shouldn't be required anymore. > > I did this to make the tests readable like natural language :/ > It's now inlined but the callback is still wrapped. I think that's okay. The alternative would be to use EXPECT_CALL, but then we'd probably need a bunch of custom matchers (which would be fine for checking if the returned config has a value at all, but probably not for e.g. ResponseContainsValidBaseInformation). https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:140: // Random difference to 0 ensures that expiry_dates are really relative. s/ensures/ensure/ https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:193: GURL GoogleTrackerBaseUrl() { return GURL(google_url_tracker_.google_url()); } GetGoogleBaseURL? Also .google_url() is already a GURL, no need for GURL(..) https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:239: RespondWithData("{}"); nit: I think this is technically valid json :D https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:258: EXPECT_THAT(config.search_url, Eq(Resolve("/search?q\u003dtest"))); This is another case where the test "input" (kSampleResponse) is far away from the expectations. Not sure what's the best thing to do about this. Maybe a BuildSampleResponse helper, which takes the things that are verified here as parameters? It'd be a lot of parameters though, so I don't know... or maybe just inline the json response in the test? Then kSampleResponse could contain only the minimal required fields, for those tests that don't really care. https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:273: TEST_F(DoodleFetcherTest, ResponseContainsExpiresWithinThirtyDays) { I don't understand the test name https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:319: TEST_F(DoodleFetcherTest, ExpectToReturnNoDoodleForMalfomedImageUrls) { ReturnsNoDoodleForMalfomedImageUrls ? https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:394: EXPECT_THAT(first_created_fetcher, Eq(second_created_fetcher)); Maybe add a test that both callbacks get called?
Okay, I will wait with involving OWNERS until we actually need this change landed. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:172: // All created TestURLFetchers have ID 0 by default. On 2017/02/09 10:36:24, Marc Treib wrote: > On 2017/02/08 18:37:25, fhorschig wrote: > > On 2017/02/08 10:51:36, Marc Treib wrote: > > > I guess there's never more than one URLFetcher at a time? > > > More to the point: What would happen if there's more than one? > > > > A test would break (RespondsToMultipleRequestsWithSameFetcher) > > and also the old fetcher would silently be overridden. > > Other tests would break but not crash. > > Thanks! (I was mostly wondering what GetFetcherByID would return - I guess it's > just the most-recently-created one.) Exactly. https://codereview.chromium.org/2660883002/diff/370008/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:178: void TriggerFetch() { On 2017/02/09 10:36:24, Marc Treib wrote: > On 2017/02/08 18:37:26, fhorschig wrote: > > On 2017/02/08 10:51:36, Marc Treib wrote: > > > I think this is a case where it's better to put this code into the test > > itself. > > > IMO the helper makes it harder to follow the tests. > > > If we remove this, then also the last_* members shouldn't be required > anymore. > > > > I did this to make the tests readable like natural language :/ > > It's now inlined but the callback is still wrapped. > > I think that's okay. The alternative would be to use EXPECT_CALL, but then we'd > probably need a bunch of custom matchers (which would be fine for checking if > the returned config has a value at all, but probably not for e.g. > ResponseContainsValidBaseInformation). I agree, the benefit would not be very huge. https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:140: // Random difference to 0 ensures that expiry_dates are really relative. On 2017/02/09 10:36:25, Marc Treib wrote: > s/ensures/ensure/ Done. https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:193: GURL GoogleTrackerBaseUrl() { return GURL(google_url_tracker_.google_url()); } On 2017/02/09 10:36:24, Marc Treib wrote: > GetGoogleBaseURL? > Also .google_url() is already a GURL, no need for GURL(..) Thanks. https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:239: RespondWithData("{}"); On 2017/02/09 10:36:24, Marc Treib wrote: > nit: I think this is technically valid json :D Invalid with regards to our specification ;) You are if course right: Renamed that one. Added a separate test for invalid JSON. https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:258: EXPECT_THAT(config.search_url, Eq(Resolve("/search?q\u003dtest"))); On 2017/02/09 10:36:24, Marc Treib wrote: > This is another case where the test "input" (kSampleResponse) is far away from > the expectations. Not sure what's the best thing to do about this. Maybe a > BuildSampleResponse helper, which takes the things that are verified here as > parameters? It'd be a lot of parameters though, so I don't know... or maybe just > inline the json response in the test? Then kSampleResponse could contain only > the minimal required fields, for those tests that don't really care. Cropped JSON strings inlined. They are sufficiently readable. The reason why I dumped this huge sample request in there was just, that I wanted to test with data as close to the actually used as possible. https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:273: TEST_F(DoodleFetcherTest, ResponseContainsExpiresWithinThirtyDays) { On 2017/02/09 10:36:24, Marc Treib wrote: > I don't understand the test name Me neither. Renamed. https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:319: TEST_F(DoodleFetcherTest, ExpectToReturnNoDoodleForMalfomedImageUrls) { On 2017/02/09 10:36:24, Marc Treib wrote: > ReturnsNoDoodleForMalfomedImageUrls ? Done. https://codereview.chromium.org/2660883002/diff/430001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:394: EXPECT_THAT(first_created_fetcher, Eq(second_created_fetcher)); On 2017/02/09 10:36:24, Marc Treib wrote: > Maybe add a test that both callbacks get called? Done.
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Introduce a Doodle Fetcher for Desktop and mobile NTP This CL is a draft. No real test coverage, much debugging stuff. BUG= ========== to ========== Introduce a Doodle Fetcher for Desktop and mobile NTP This CL is a draft. No real test coverage, much debugging stuff. BUG=690467 ==========
Description was changed from ========== Introduce a Doodle Fetcher for Desktop and mobile NTP This CL is a draft. No real test coverage, much debugging stuff. BUG=690467 ========== to ========== Introduce a Doodle Fetcher for Desktop and mobile NTP This fetcher queries the standard Doodle endpoint [1] and converts the config into struct. Attributes are checked for sanity and converted into fitting formats (e.g. expiry_date based on TTL value). [1] www.google.com/async/ddljson. BUG=690467 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Did one more pass and found a few more nits ;) Otherwise, I think we can get started on landing this now. Description nit: I wouldn't talk about desktop yet, since for now it'll only be used on mobile. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:19: #include "url/gurl.h" nit: already included in the header https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:88: DoodleFetcher::~DoodleFetcher() {} nit: = default https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:92: if (callbacks_.size() > 1) { Would "if (fetcher_)" be a more straightforward way of checking whether a fetch is currently in progress? fetcher_ should be non-null exactly while a fetch is ongoing, correct? https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:106: // net::URLFetcherDelegate implementation. nit: comment not required here (there's one in the header) https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:107: void DoodleFetcher::OnURLFetchComplete(const net::URLFetcher* source) { nit: There's a "using net::URLFetcher" above, so the "net::" prefix isn't required. (Though TBH, I might just remove the "using" and specify the prefix everywhere... up to you) https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:143: void DoodleFetcher::ParseDoodle(std::unique_ptr<base::DictionaryValue> config) { nit: This is different from the other "Parse*" methods - it does more than just parsing. Can we find a better name and/or split off the actual parsing from the rest (calling callbacks)? https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.h:77: std::string interactive_js; I think we should leave out interactive_html and _js for now, since there are no *immediate* plans to use them. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.h:99: using ParseJSONCallback = base::Callback<void( I think this is worth a comment: // Callback for JSON parsing to allow injecting platform-dependent code. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.h:107: DoodleFetcher(DoodleFetcher&&); Is this required? I don't see when we would ever want to move the fetcher. Also it doesn't seem to be implemented :) https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.h:111: // provides a valid DoodleConfig object. Comment is outdated.
Description was changed from ========== Introduce a Doodle Fetcher for Desktop and mobile NTP This fetcher queries the standard Doodle endpoint [1] and converts the config into struct. Attributes are checked for sanity and converted into fitting formats (e.g. expiry_date based on TTL value). [1] www.google.com/async/ddljson. BUG=690467 ========== to ========== Introduce a Doodle Fetcher for NTP This fetcher queries the standard Doodle endpoint [1] and converts the config into struct. Attributes are checked for sanity and converted into fitting formats (e.g. expiry_date based on TTL value). [1] www.google.com/async/ddljson. BUG=690467 ==========
Description was changed from ========== Introduce a Doodle Fetcher for NTP This fetcher queries the standard Doodle endpoint [1] and converts the config into struct. Attributes are checked for sanity and converted into fitting formats (e.g. expiry_date based on TTL value). [1] www.google.com/async/ddljson. BUG=690467 ========== to ========== Introduce a Doodle Fetcher for NTP This fetcher queries the standard Doodle endpoint [1] and converts the config into a struct. Attributes are checked for sanity and converted into fitting formats (e.g. expiry_date based on TTL value). [1] www.google.com/async/ddljson. BUG=690467 ==========
Title and description adjusted. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:19: #include "url/gurl.h" On 2017/02/10 10:28:47, Marc Treib wrote: > nit: already included in the header Gone. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:88: DoodleFetcher::~DoodleFetcher() {} On 2017/02/10 10:28:47, Marc Treib wrote: > nit: = default Done. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:92: if (callbacks_.size() > 1) { On 2017/02/10 10:28:47, Marc Treib wrote: > Would "if (fetcher_)" be a more straightforward way of checking whether a fetch > is currently in progress? fetcher_ should be non-null exactly while a fetch is > ongoing, correct? Hmm, yes. Changed condition and the fetcher_ to be deleted later. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:106: // net::URLFetcherDelegate implementation. On 2017/02/10 10:28:48, Marc Treib wrote: > nit: comment not required here (there's one in the header) Gone. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:107: void DoodleFetcher::OnURLFetchComplete(const net::URLFetcher* source) { On 2017/02/10 10:28:47, Marc Treib wrote: > nit: There's a "using net::URLFetcher" above, so the "net::" prefix isn't > required. (Though TBH, I might just remove the "using" and specify the prefix > everywhere... up to you) net:: Gone. (I like the using for readability of the fetcher creation) https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.cc:143: void DoodleFetcher::ParseDoodle(std::unique_ptr<base::DictionaryValue> config) { On 2017/02/10 10:28:47, Marc Treib wrote: > nit: This is different from the other "Parse*" methods - it does more than just > parsing. Can we find a better name and/or split off the actual parsing from the > rest (calling callbacks)? Done. |OnJsonParsed| is where callback are called and ParseDoodle returns only the result. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.h:77: std::string interactive_js; On 2017/02/10 10:28:48, Marc Treib wrote: > I think we should leave out interactive_html and _js for now, since there are no > *immediate* plans to use them. As discussed, interactive_js is gone. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.h:99: using ParseJSONCallback = base::Callback<void( On 2017/02/10 10:28:48, Marc Treib wrote: > I think this is worth a comment: > // Callback for JSON parsing to allow injecting platform-dependent code. Accepted as proposed. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.h:107: DoodleFetcher(DoodleFetcher&&); On 2017/02/10 10:28:48, Marc Treib wrote: > Is this required? I don't see when we would ever want to move the fetcher. > Also it doesn't seem to be implemented :) Gone. https://codereview.chromium.org/2660883002/diff/450001/components/doodle/dood... components/doodle/doodle_fetcher.h:111: // provides a valid DoodleConfig object. On 2017/02/10 10:28:48, Marc Treib wrote: > Comment is outdated. Done.
fhorschig@chromium.org changed reviewers: + sdefresne@chromium.org
Hi sdefresne@, would you please have a look whether the doodle fetcher fits into components/?
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.cc:94: DCHECK(!fetcher_.get()); nit: Now the DCHECK isn't required anymore, since we're checking this just above https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.cc:139: if (doodle.has_value()) { nit: I'd swap this around - if there *isn't* a value, then return NO_DOODLE. That's more consistent with the other ifs above: Early-out in the error case. https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.cc:223: std::unique_ptr<net::URLFetcher> free_fetcher = std::move(fetcher_); This doesn't really belong here IMO - the fetcher has nothing to do with calling the callbacks. How about putting this into OnURLFetchComplete? https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.h:130: const base::DictionaryValue& ddljson); const? https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.h:134: void ParseBaseInformation(const base::DictionaryValue& ddljson, Should this return a bool too? false if any required info is missing, e.g. ttl or target_url?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:120001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #14 (id:400001) has been deleted
Patchset #13 (id:380001) has been deleted
https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.cc:94: DCHECK(!fetcher_.get()); On 2017/02/13 09:48:40, Marc Treib wrote: > nit: Now the DCHECK isn't required anymore, since we're checking this just above Reverted condition as discussed. https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.cc:139: if (doodle.has_value()) { On 2017/02/13 09:48:40, Marc Treib wrote: > nit: I'd swap this around - if there *isn't* a value, then return NO_DOODLE. > That's more consistent with the other ifs above: Early-out in the error case. Okay. https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.cc:223: std::unique_ptr<net::URLFetcher> free_fetcher = std::move(fetcher_); On 2017/02/13 09:48:40, Marc Treib wrote: > This doesn't really belong here IMO - the fetcher has nothing to do with calling > the callbacks. How about putting this into OnURLFetchComplete? Moved up again as discussed. https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.h:130: const base::DictionaryValue& ddljson); On 2017/02/13 09:48:40, Marc Treib wrote: > const? Why should I change the input data? Is it possible that you considered this the output? I added an output parameter and returned a bool which makes it a little more consistent in |OnJsonParsed|. https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.h:134: void ParseBaseInformation(const base::DictionaryValue& ddljson, On 2017/02/13 09:48:40, Marc Treib wrote: > Should this return a bool too? false if any required info is missing, e.g. ttl > or target_url? According to our Jonathan, only a missing large image means there is no doodle. Everything else was not specified and should not fail. As the warnings are different for no TLL and too big TTL, I could not make use of the return value otherwise. So adding it when really needed seems more useful to me.
https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.cc:223: std::unique_ptr<net::URLFetcher> free_fetcher = std::move(fetcher_); On 2017/02/13 11:01:26, fhorschig wrote: > On 2017/02/13 09:48:40, Marc Treib wrote: > > This doesn't really belong here IMO - the fetcher has nothing to do with > calling > > the callbacks. How about putting this into OnURLFetchComplete? > > Moved up again as discussed. Nope, it's still here ;P https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.h:130: const base::DictionaryValue& ddljson); On 2017/02/13 11:01:27, fhorschig wrote: > On 2017/02/13 09:48:40, Marc Treib wrote: > > const? > > Why should I change the input data? I meant that the method itself should probably be "const" (i.e. it doesn't change the fetcher object). I see you've done that now :) > Is it possible that you considered this the output? > I added an output parameter and returned a bool which > makes it a little more consistent in |OnJsonParsed|. FWIW, I liked the Optional return value; I prefer to avoid "out" params where possible. https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.h:134: void ParseBaseInformation(const base::DictionaryValue& ddljson, On 2017/02/13 11:01:28, fhorschig wrote: > On 2017/02/13 09:48:40, Marc Treib wrote: > > Should this return a bool too? false if any required info is missing, e.g. ttl > > or target_url? > > According to our Jonathan, only a missing large image means there > is no doodle. Everything else was not specified and should not fail. > > As the warnings are different for no TLL and too big TTL, I could > not make use of the return value otherwise. So adding it when > really needed seems more useful to me. I assumed it would be really needed here :) But if all the "base" fields are really considered optional, then so be it. https://codereview.chromium.org/2660883002/diff/490001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/490001/components/doodle/dood... components/doodle/doodle_fetcher.h:145: bool IsFetchInProgress() const { return callbacks_.size() > 0; } !callbacks_.empty();
https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.cc:223: std::unique_ptr<net::URLFetcher> free_fetcher = std::move(fetcher_); On 2017/02/13 11:12:49, Marc Treib wrote: > On 2017/02/13 11:01:26, fhorschig wrote: > > On 2017/02/13 09:48:40, Marc Treib wrote: > > > This doesn't really belong here IMO - the fetcher has nothing to do with > > calling > > > the callbacks. How about putting this into OnURLFetchComplete? > > > > Moved up again as discussed. > > Nope, it's still here ;P Weird. Moved up again. (And double-checked) https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.h:130: const base::DictionaryValue& ddljson); On 2017/02/13 11:12:49, Marc Treib wrote: > On 2017/02/13 11:01:27, fhorschig wrote: > > On 2017/02/13 09:48:40, Marc Treib wrote: > > > const? > > > > Why should I change the input data? > > I meant that the method itself should probably be "const" (i.e. it doesn't > change the fetcher object). I see you've done that now :) Oh, sorry. > > Is it possible that you considered this the output? > > I added an output parameter and returned a bool which > > makes it a little more consistent in |OnJsonParsed|. > > FWIW, I liked the Optional return value; I prefer to avoid "out" params where > possible. Reverted as I am indifferent. https://codereview.chromium.org/2660883002/diff/470001/components/doodle/dood... components/doodle/doodle_fetcher.h:134: void ParseBaseInformation(const base::DictionaryValue& ddljson, On 2017/02/13 11:12:49, Marc Treib wrote: > On 2017/02/13 11:01:28, fhorschig wrote: > > On 2017/02/13 09:48:40, Marc Treib wrote: > > > Should this return a bool too? false if any required info is missing, e.g. > ttl > > > or target_url? > > > > According to our Jonathan, only a missing large image means there > > is no doodle. Everything else was not specified and should not fail. > > > > As the warnings are different for no TLL and too big TTL, I could > > not make use of the return value otherwise. So adding it when > > really needed seems more useful to me. > > I assumed it would be really needed here :) > But if all the "base" fields are really considered optional, then so be it. As far as I am informed, that is correct. https://codereview.chromium.org/2660883002/diff/490001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/490001/components/doodle/dood... components/doodle/doodle_fetcher.h:145: bool IsFetchInProgress() const { return callbacks_.size() > 0; } On 2017/02/13 11:12:50, Marc Treib wrote: > !callbacks_.empty(); Done.
lgtm https://codereview.chromium.org/2660883002/diff/510001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/510001/components/doodle/dood... components/doodle/doodle_fetcher.h:55: explicit DoodleImage(GURL url); Two questions: 1. why is this taking GURL by copy and not by const-reference? 2. this appears to not be implemented in doodle_fetcher.cc, should it be removed?
Thank you for the timely response! https://codereview.chromium.org/2660883002/diff/510001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/510001/components/doodle/dood... components/doodle/doodle_fetcher.h:55: explicit DoodleImage(GURL url); On 2017/02/13 15:55:26, sdefresne wrote: > Two questions: > > 1. why is this taking GURL by copy and not by const-reference? > 2. this appears to not be implemented in doodle_fetcher.cc, should it be > removed? Removed the unnecessary constructor.
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fhorschig@chromium.org changed reviewers: + mattm@chromium.org
Hi mattm@, Would you please review my CL? This fetcher introduces dependencies to net/.
https://codereview.chromium.org/2660883002/diff/530001/components/doodle/DEPS File components/doodle/DEPS (right): https://codereview.chromium.org/2660883002/diff/530001/components/doodle/DEPS... components/doodle/DEPS:3: "+net", Prefer to keep include rules more focused, eg: +net/base +net/http/http_status_code.h +net/url_request https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.cc:87: DoodleFetcher::~DoodleFetcher() = default; Just food for thought, but is it okay to leave hanging callbacks if there was a request in progress? Would you want to run any waiting callbacks with some error code? https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.cc:100: net::LOAD_DO_NOT_SAVE_COOKIES); Also LOAD_DO_NOT_SEND_AUTH_DATA https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.cc:102: fetcher_->Start(); Do you need a data_use_measurement::DataUseUserData::AttachToFetcher? (I'm not really familiar with it, but all the fetchers seem to have it these days..) https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.cc:199: double ttl = 0; // Expires immediately if the parameter is missing. Are fetches only initiated by the user opening the new tab page? Or is there any need for some sort of rate limiting / back-off in case of errors or 0 ttl? https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.h:103: DoodleFetcher(net::URLRequestContextGetter* download_context, In practice, which context will this be? The profile context? If there are multiple profiles/incognito, is there a separate doodle fetcher for each? https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.h:147: net::URLRequestContextGetter* const download_context_; scoped_refptr
https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.cc:87: DoodleFetcher::~DoodleFetcher() = default; On 2017/02/14 22:37:14, mattm wrote: > Just food for thought, but is it okay to leave hanging callbacks if there was a > request in progress? Would you want to run any waiting callbacks with some error > code? IMO this is okay and actually quite a common pattern. The common use case is: My class owns a DoodleFetcher, and binds some instance method to the callback. When my class is destroyed, that means I don't care anymore about pending callbacks. Running the callbacks at this point would be dangerous, since my class might already be in a half-destructed state. https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.cc:102: fetcher_->Start(); On 2017/02/14 22:37:14, mattm wrote: > Do you need a data_use_measurement::DataUseUserData::AttachToFetcher? (I'm not > really familiar with it, but all the fetchers seem to have it these days..) Good catch! Yes, we should do that. I'd prefer doing it in a separate CL though; this one is big enough already. https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.h:103: DoodleFetcher(net::URLRequestContextGetter* download_context, On 2017/02/14 22:37:14, mattm wrote: > In practice, which context will this be? The profile context? If there are > multiple profiles/incognito, is there a separate doodle fetcher for each? Yes, this will be per-profile. Right now, we don't show Doodles in incognito profiles, and I don't think there are any plans in that direction. But if that ever changes, things should Just Work.
Patchset #20 (id:550001) has been deleted
https://codereview.chromium.org/2660883002/diff/530001/components/doodle/DEPS File components/doodle/DEPS (right): https://codereview.chromium.org/2660883002/diff/530001/components/doodle/DEPS... components/doodle/DEPS:3: "+net", On 2017/02/14 22:37:14, mattm wrote: > Prefer to keep include rules more focused, eg: > +net/base > +net/http/http_status_code.h > +net/url_request Done. https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... File components/doodle/doodle_fetcher.cc (right): https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.cc:100: net::LOAD_DO_NOT_SAVE_COOKIES); On 2017/02/14 22:37:14, mattm wrote: > Also LOAD_DO_NOT_SEND_AUTH_DATA Done. https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.cc:102: fetcher_->Start(); On 2017/02/15 09:07:22, Marc Treib wrote: > On 2017/02/14 22:37:14, mattm wrote: > > Do you need a data_use_measurement::DataUseUserData::AttachToFetcher? (I'm not > > really familiar with it, but all the fetchers seem to have it these days..) > > Good catch! Yes, we should do that. I'd prefer doing it in a separate CL though; > this one is big enough already. Okay,follow-up Cl created: https://codereview.chromium.org/2698643002/ https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.cc:199: double ttl = 0; // Expires immediately if the parameter is missing. On 2017/02/14 22:37:14, mattm wrote: > Are fetches only initiated by the user opening the new tab page? Or is there any > need for some sort of rate limiting / back-off in case of errors or 0 ttl? The fetcher is rather simple as it only fetches the doodle on demand. The responsibility of rate limiting is considered in higher layers. We plan to have a public interface for accessing doodles, different from the fetcher itself. https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... File components/doodle/doodle_fetcher.h (right): https://codereview.chromium.org/2660883002/diff/530001/components/doodle/dood... components/doodle/doodle_fetcher.h:147: net::URLRequestContextGetter* const download_context_; On 2017/02/14 22:37:14, mattm wrote: > scoped_refptr Done.
lgtm w/nit https://codereview.chromium.org/2660883002/diff/570001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/570001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:145: scoped_refptr<net::TestURLRequestContextGetter> context_getter_; unused now?
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2660883002/diff/570001/components/doodle/dood... File components/doodle/doodle_fetcher_unittest.cc (right): https://codereview.chromium.org/2660883002/diff/570001/components/doodle/dood... components/doodle/doodle_fetcher_unittest.cc:145: scoped_refptr<net::TestURLRequestContextGetter> context_getter_; On 2017/02/15 20:17:27, mattm wrote: > unused now? Gone.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, sdefresne@chromium.org, mattm@chromium.org Link to the patchset: https://codereview.chromium.org/2660883002/#ps590001 (title: "Remove unused context_getter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 590001, "attempt_start_ts": 1487255022332140,
"parent_rev": "f833b45c4615349644227cac888baf86c1963020", "commit_rev":
"5ba57aa78c96c678de1cdf6c34ffe9fefae21a6e"}
Message was sent while issue was closed.
Description was changed from ========== Introduce a Doodle Fetcher for NTP This fetcher queries the standard Doodle endpoint [1] and converts the config into a struct. Attributes are checked for sanity and converted into fitting formats (e.g. expiry_date based on TTL value). [1] www.google.com/async/ddljson. BUG=690467 ========== to ========== Introduce a Doodle Fetcher for NTP This fetcher queries the standard Doodle endpoint [1] and converts the config into a struct. Attributes are checked for sanity and converted into fitting formats (e.g. expiry_date based on TTL value). [1] www.google.com/async/ddljson. BUG=690467 Review-Url: https://codereview.chromium.org/2660883002 Cr-Commit-Position: refs/heads/master@{#450954} Committed: https://chromium.googlesource.com/chromium/src/+/5ba57aa78c96c678de1cdf6c34ff... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:590001) as https://chromium.googlesource.com/chromium/src/+/5ba57aa78c96c678de1cdf6c34ff... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
