Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12211076:
Refactored FakeURLFetcher to make it more flexible (Closed)
|
Created:
7 years, 10 months ago by Noam Samuel (WRONG ACCOUNT) Modified:
6 years, 12 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRefactored FakeURLFetcher to make it more flexible
Added an optional parameter to the FakeURLFetcherFactory class that is a callback used to create the FakeURLFetcher. This can be used to add information to the FakeURLFetcher when it is created, such as HTTP headers or cookies.
As part of an effort to fix https://code.google.com/p/chromium/issues/detail?id=176361 .
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182821
Patch Set 1 #Patch Set 2 : Fix lint issue #
Total comments: 4
Patch Set 3 : Implementing suggestion from commenters #Patch Set 4 : Fix linter errors #
Total comments: 29
Patch Set 5 : Incorporated reviewer feedback #
Total comments: 14
Patch Set 6 : Interface change and stylistic changes per review comments #
Total comments: 8
Patch Set 7 : Style issues & changing to scoped_ptr callback interface #
Total comments: 6
Patch Set 8 : #
Messages
Total messages: 25 (0 generated)
Hey, I'm from the cloud print team and I need to modify FakeURLFetcherFactory to make it more flexible so I can subclass it and allow for the subclass to inject headers/cookies into the response. To do this, I moved part of the CreateURLFetcher into a virtual protected method that returns a TestURLFetcher* so that subclasses can override CreateURLFetcher and use the TestURLFetcher API in doing so.
https://codereview.chromium.org/12211076/diff/5001/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/5001/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:323: virtual TestURLFetcher* CreateFakeURLFetcher(const GURL& URL, I'm confused by the need for this. FakeURLFetcherFactory already has the parameter default_factory, so you can implement URLFetcherFactory directly and have it do whatever you want, passing it into FakeURLFetcherFactory. Is there a reason why this won't work for you? Also, in general, I prefer passing in parameters over subclassing for customization.
https://codereview.chromium.org/12211076/diff/5001/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/5001/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:323: virtual TestURLFetcher* CreateFakeURLFetcher(const GURL& URL, The main scenario I'm trying to support is the ability to simulate HTTP headers using FakeURLFetcher. Without this modification, the cleanest way to do that would be to re-implement FakeURLFetcherFactory using TestURLFetcher. Another way to address this would be to add an optional "headers" parameter to SetFakeResponse, but I suspected that adding "just one more" HTTP feature to simulate is a slippery slope to feature creep. On 2013/02/08 07:47:35, akalin wrote: > I'm confused by the need for this. FakeURLFetcherFactory already has the > parameter default_factory, so you can implement URLFetcherFactory directly and > have it do whatever you want, passing it into FakeURLFetcherFactory. Is there a > reason why this won't work for you? > > Also, in general, I prefer passing in parameters over subclassing for > customization.
https://codereview.chromium.org/12211076/diff/5001/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/5001/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:323: virtual TestURLFetcher* CreateFakeURLFetcher(const GURL& URL, I don't have a strong opinion here but I would also lean towards avoiding sub-classing. Have you considered exposing the FakeUrlFetcher class in the header file and then adding a more generic function to the factory class which sets fake fetchers? E.g., void SetFakeFetcher(FakeURLFetcher* fetcher); That would give you all the flexibility you need, wouldn't it? On 2013/02/08 16:27:10, noamsml wrote: > The main scenario I'm trying to support is the ability to simulate HTTP headers > using FakeURLFetcher. Without this modification, the cleanest way to do that > would be to re-implement FakeURLFetcherFactory using TestURLFetcher. Another way > to address this would be to add an optional "headers" parameter to > SetFakeResponse, but I suspected that adding "just one more" HTTP feature to > simulate is a slippery slope to feature creep. > > On 2013/02/08 07:47:35, akalin wrote: > > I'm confused by the need for this. FakeURLFetcherFactory already has the > > parameter default_factory, so you can implement URLFetcherFactory directly and > > have it do whatever you want, passing it into FakeURLFetcherFactory. Is there > a > > reason why this won't work for you? > > > > Also, in general, I prefer passing in parameters over subclassing for > > customization. >
https://codereview.chromium.org/12211076/diff/5001/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/5001/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:323: virtual TestURLFetcher* CreateFakeURLFetcher(const GURL& URL, Not sure how that would work. The FakeURLFetcherFactory returns a new object each time, and the user of the URL fetcher is responsible for deleting it. It wouldn't really make sense in this context to set a static FakeURLFetcher that is returned, even per URL (since the tested code may instantiate multiple fetchers for the same URL). One option that avoids subclassing would be to surface a delegate interface that will be invoked on URL Fetcher creation and passed the URL and a pointer to the fetcher (as a TestURLFetcher*, allowing the test code to set additional properties). Would this be more elegant? On 2013/02/08 18:21:40, noelutz wrote: > I don't have a strong opinion here but I would also lean towards avoiding > sub-classing. Have you considered exposing the FakeUrlFetcher class in the > header file and then adding a more generic function to the factory class which > sets fake fetchers? E.g., void SetFakeFetcher(FakeURLFetcher* fetcher); That > would give you all the flexibility you need, wouldn't it? > > On 2013/02/08 16:27:10, noamsml wrote: > > The main scenario I'm trying to support is the ability to simulate HTTP > headers > > using FakeURLFetcher. Without this modification, the cleanest way to do that > > would be to re-implement FakeURLFetcherFactory using TestURLFetcher. Another > way > > to address this would be to add an optional "headers" parameter to > > SetFakeResponse, but I suspected that adding "just one more" HTTP feature to > > simulate is a slippery slope to feature creep. > > > > On 2013/02/08 07:47:35, akalin wrote: > > > I'm confused by the need for this. FakeURLFetcherFactory already has the > > > parameter default_factory, so you can implement URLFetcherFactory directly > and > > > have it do whatever you want, passing it into FakeURLFetcherFactory. Is > there > > a > > > reason why this won't work for you? > > > > > > Also, in general, I prefer passing in parameters over subclassing for > > > customization. > > >
On 2013/02/08 18:58:44, noamsml wrote:
> One option that avoids subclassing would be to surface a delegate interface
that
> will be invoked on URL Fetcher creation and passed the URL and a pointer to
the
> fetcher (as a TestURLFetcher*, allowing the test code to set additional
> properties). Would this be more elegant?
Okay, I took a closer look and I think I now understand what you're trying to
do.
I'm leaning towards your suggestion, but using a Closure instead of yet another
interface. Perhaps something like:
class FakeURLFetcher;
class FakeURLFetcherFactory {
public:
// Takes the URL, delegate, response data, and success, and creates a
FakeURLFetcher out of it.
typedef base::Closure<FakeURLFetcher*(const GURL&, URLFetcherDelegate*, const
std::string&, bool)> FakeURLFetcherCreator;
FakeURLFetcherFactory(const FakeURLFetcherCreator& fake_url_fetcher_creator,
URLFetcherFactory* default_factory);
...
}
You can provide a default one or something that just creates a new
FakeURLFetcher, but to customize it you can make a function that creates a new
one and then tweaks the new FakeURLFetcher's parameters as needed.
What do you think?
Implemented akalin's suggestion.
Overall approach looks good, but got some nits. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:13: remove extra newline https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:23: remove extra newline https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:298: typedef base::Callback< FakeURLFetcher*( no space after < https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:298: typedef base::Callback< FakeURLFetcher*( add comment describing what the parameters are (since they don't have names) https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:298: typedef base::Callback< FakeURLFetcher*( since the caller should take ownership of the returned FakeURLFetcher*, perhaps have it return a scoped_ptr<FakeURLFetcher>? https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:302: bool) > FakeURLFetcherCreator; no space before > https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:304: FakeURLFetcherFactory(); see if you can get away with only having a single constructor. There are only a few places that uses FakeURLFetcherFactory so it shouldn't be too hard. You may need to expose a "MakeFakeURLFetcher()" function that simply calls new FakeURLFetcher.... https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:309: // FakeURLFetcherFactory that will delegate creating URLFetcher for unknown this comment isn't a complete sentence and is hard to understand. Perhaps something like: // |creator| is a callback that ... // |default_factory|, which can be NULL, is a ... https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:334: remove extra newline https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:340: FakeURLFetcherCreator creator_; make it const https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:343: URLFetcherFactory* default_factory_; make this const, like URLFetcherFactory* const https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:356: // This class is used by the FakeURLFetcherFactory above. Try to describe what the FakeURLFetcher does without referring to FakeURLFetcherFactory. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:357: class FakeURLFetcher : public TestURLFetcher { you can move this declaration up above FakeURLFetcherFactory instead of just forward-declaring.
https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:13: On 2013/02/12 23:10:10, akalin wrote: > remove extra newline Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:23: On 2013/02/12 23:10:10, akalin wrote: > remove extra newline Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:298: typedef base::Callback< FakeURLFetcher*( On 2013/02/12 23:10:10, akalin wrote: > no space after < Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:298: typedef base::Callback< FakeURLFetcher*( On 2013/02/12 23:10:10, akalin wrote: > since the caller should take ownership of the returned FakeURLFetcher*, perhaps > have it return a scoped_ptr<FakeURLFetcher>? Since this callback is only called by the factory itself, which should return a URLFetcher* (per its interface), I do not think this is the case. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:298: typedef base::Callback< FakeURLFetcher*( On 2013/02/12 23:10:10, akalin wrote: > add comment describing what the parameters are (since they don't have names) Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:302: bool) > FakeURLFetcherCreator; On 2013/02/12 23:10:10, akalin wrote: > no space before > Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:304: FakeURLFetcherFactory(); Since the callback has an obvious but nontrivial default value, wouldn't it make sense to leave the option of not passing it? The creation of this class may be confusing otherwise. On 2013/02/12 23:10:10, akalin wrote: > see if you can get away with only having a single constructor. There are only a > few places that uses FakeURLFetcherFactory so it shouldn't be too hard. You may > need to expose a "MakeFakeURLFetcher()" function that simply calls new > FakeURLFetcher.... https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:309: // FakeURLFetcherFactory that will delegate creating URLFetcher for unknown On 2013/02/12 23:10:10, akalin wrote: > this comment isn't a complete sentence and is hard to understand. Perhaps > something like: > > // |creator| is a callback that ... > // |default_factory|, which can be NULL, is a ... Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:334: On 2013/02/12 23:10:10, akalin wrote: > remove extra newline Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:340: FakeURLFetcherCreator creator_; On 2013/02/12 23:10:10, akalin wrote: > make it const Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:343: URLFetcherFactory* default_factory_; On 2013/02/12 23:10:10, akalin wrote: > make this const, like > > URLFetcherFactory* const Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:356: // This class is used by the FakeURLFetcherFactory above. On 2013/02/12 23:10:10, akalin wrote: > Try to describe what the FakeURLFetcher does without referring to > FakeURLFetcherFactory. Done. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:357: class FakeURLFetcher : public TestURLFetcher { On 2013/02/12 23:10:10, akalin wrote: > you can move this declaration up above FakeURLFetcherFactory instead of just > forward-declaring. Done.
https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:298: typedef base::Callback< FakeURLFetcher*( On 2013/02/13 00:25:52, Noam Samuel wrote: > On 2013/02/12 23:10:10, akalin wrote: > > since the caller should take ownership of the returned FakeURLFetcher*, > perhaps > > have it return a scoped_ptr<FakeURLFetcher>? > > Since this callback is only called by the factory itself, which should return a > URLFetcher* (per its interface), I do not think this is the case. I'd argue that the factory method should also return a scoped_ptr<>. That might require touching too many files, though, so I'm okay with leaving the scoped_ptr<> in the callback and calling release. https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:304: FakeURLFetcherFactory(); On 2013/02/13 00:25:52, Noam Samuel wrote: > Since the callback has an obvious but nontrivial default > value, wouldn't it make sense to leave the option of not > passing it? The creation of this class may be confusing > otherwise. I suppose. Although I would at least like to get rid of the default constructor (and have callers pass in NULL). https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.cc:342: remove extra newline https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.cc:344: FakeURLFetcher::FakeURLFetcher(const GURL& url, move this above the FakeURLFetcherFactory methods (in general, try to keep the order the same as in the header file) https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:274: // ExampleUsage: ExampleUsage -> Example usage https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:303: DISALLOW_COPY_AND_ASSIGN(FakeURLFetcher); #include "base/basictypes.h" for this https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:352: remove extra newline https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:356: // set to MakeFakeURLFetcher append period https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:393: remove extra newline (please try to keep only one space between functions, etc.)
https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/3002/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:304: FakeURLFetcherFactory(); On 2013/02/13 20:44:01, akalin wrote: > On 2013/02/13 00:25:52, Noam Samuel wrote: > > Since the callback has an obvious but nontrivial default > > value, wouldn't it make sense to leave the option of not > > passing it? The creation of this class may be confusing > > otherwise. > > I suppose. Although I would at least like to get rid of the default constructor > (and have callers pass in NULL). Added to latest patch set. If it's too cumbersome as part of this CL, it can also be factored into its own CL. https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.cc:342: On 2013/02/13 20:44:01, akalin wrote: > remove extra newline Done. https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.cc:344: FakeURLFetcher::FakeURLFetcher(const GURL& url, On 2013/02/13 20:44:01, akalin wrote: > move this above the FakeURLFetcherFactory methods (in general, try to keep the > order the same as in the header file) Done. https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:274: // ExampleUsage: On 2013/02/13 20:44:01, akalin wrote: > ExampleUsage -> Example usage Done. https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:303: DISALLOW_COPY_AND_ASSIGN(FakeURLFetcher); On 2013/02/13 20:44:01, akalin wrote: > #include "base/basictypes.h" for this Done. https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:352: On 2013/02/13 20:44:01, akalin wrote: > remove extra newline Done. https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:356: // set to MakeFakeURLFetcher On 2013/02/13 20:44:01, akalin wrote: > append period Done. https://codereview.chromium.org/12211076/diff/7005/net/url_request/test_url_f... net/url_request/test_url_fetcher_factory.h:393: On 2013/02/13 20:44:01, akalin wrote: > remove extra newline > > (please try to keep only one space between functions, etc.) Done.
https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.cc:283: URLFetcherDelegate* d, indentation https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.h:280: // // Will schedule a call some_delegate->OnURLFetchComplete(&fake_fetcher) a call -> a call to Add period at end of sentence https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.h:343: typedef base::Callback<FakeURLFetcher*( you didn't reply to my comment re. scoped_ptr? https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.h:363: URLFetcherFactory* default_factory); i feel like default_factory should go first, since it's the "required" param
https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.cc:283: URLFetcherDelegate* d, On 2013/02/13 22:17:49, akalin wrote: > indentation Done. https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.h:280: // // Will schedule a call some_delegate->OnURLFetchComplete(&fake_fetcher) On 2013/02/13 22:17:49, akalin wrote: > a call -> a call to > > Add period at end of sentence Done. https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.h:343: typedef base::Callback<FakeURLFetcher*( On 2013/02/13 22:17:49, akalin wrote: > you didn't reply to my comment re. scoped_ptr? Whoops, my bad. Implemented your suggestion. https://codereview.chromium.org/12211076/diff/13002/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.h:363: URLFetcherFactory* default_factory); On 2013/02/13 22:17:49, akalin wrote: > i feel like default_factory should go first, since it's the "required" param Done.
lgtm, but please update the CL title and description before committing https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.cc:331: return scoped_ptr<FakeURLFetcher> (new FakeURLFetcher(url, delegate, no space after > make sure to fix indent on next line to match https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.cc:355: return fake_fetcher.release(); add TODO to make CreateURLFetcher return a scoped_ptr<> https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.h:296: virtual ~FakeURLFetcher(); newline before private:
OK, fixed the description and CL name. https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.cc:331: return scoped_ptr<FakeURLFetcher> (new FakeURLFetcher(url, delegate, On 2013/02/14 14:52:06, akalin wrote: > no space after > > > make sure to fix indent on next line to match Done. https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.cc:355: return fake_fetcher.release(); On 2013/02/14 14:52:06, akalin wrote: > add TODO to make CreateURLFetcher return a scoped_ptr<> Done. https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/12211076/diff/18001/net/url_request/test_url_... net/url_request/test_url_fetcher_factory.h:296: virtual ~FakeURLFetcher(); On 2013/02/14 14:52:06, akalin wrote: > newline before private: Done.
still lgtm although it would be nice to have a bug for your larger task and reference it here
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@google.com/12211076/16003
Presubmit check for 12211076-16003 failed and returned exit status 1.
INFO:root:Found 5 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
net
chrome/browser/extensions
chrome/browser/safe_browsing
Presubmit checks took 1.1s to calculate.
Hey, could people who are owners in net/, chrome/browser/safe_browsing/, and chrome/browser/extensions/ take a look at this CL?
On 2013/02/14 22:14:48, Noam Samuel wrote: > Hey, could people who are owners in net/, chrome/browser/safe_browsing/, and > chrome/browser/extensions/ take a look at this CL? You should look in the OWNERS files for those dirs and call out specific people. (You're more likely to get a response that way.)
Since I forgot +aa@chromium.org for chrome/browser/extensions/ OWNERS +mattm@chromium.org for net/ OWNERS and chrome/browser/safe_browsing/ OWNERS
In the description you can put the bug number in the BUG= line. net & safe_browsing lgtm.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@google.com/12211076/16003
Message was sent while issue was closed.
Change committed as 182821 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
