Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1454413002:
Move FakeWebHistoryService to components/history (Closed)
DescriptionMove FakeWebHistoryService to components/history
1. split to declaration and definition to .cc and .h
2. since we moved to components/, the constructor can no longer derive the needed keyed services from Profile; instead, the caller must do so and pass them to FakeWebHistoryService constructor; the constructor signature now maches that of WebHistoryService
3. added support for the "num" query parameter, i.e. the limit of returned entries
TBD: Add tests
BUG=553421
Committed: https://crrev.com/b932634b22dcde4d847a2a467a0c07c7c90b413b
Cr-Commit-Position: refs/heads/master@{#366071}
Patch Set 1 #
Total comments: 29
Patch Set 2 : Addressed comments. #
Total comments: 4
Patch Set 3 : Include net #
Total comments: 1
Patch Set 4 : Addressed most comments. #Patch Set 5 : Moved to test/ #
Total comments: 2
Patch Set 6 : Removed forward declaration. #
Messages
Total messages: 24 (8 generated)
Description was changed from ========== Move FakeWebHistoryService to components/history BUG=553421 ========== to ========== Move FakeWebHistoryService to components/history Changes: - since we moved to components/, the constructor can no longer derive the needed keyed services from Profile; instead, the caller must do so and pass them to FakeWebHistoryService constructor; the constructor signature now maches that of WebHistoryService - added support for the "num" query parameter, i.e. the limit of returned entries TBD: Add tests BUG=553421 ==========
Description was changed from ========== Move FakeWebHistoryService to components/history Changes: - since we moved to components/, the constructor can no longer derive the needed keyed services from Profile; instead, the caller must do so and pass them to FakeWebHistoryService constructor; the constructor signature now maches that of WebHistoryService - added support for the "num" query parameter, i.e. the limit of returned entries TBD: Add tests BUG=553421 ========== to ========== Move FakeWebHistoryService to components/history 1. split to declaration and definition to .cc and .h 2. since we moved to components/, the constructor can no longer derive the needed keyed services from Profile; instead, the caller must do so and pass them to FakeWebHistoryService constructor; the constructor signature now maches that of WebHistoryService 3. added support for the "num" query parameter, i.e. the limit of returned entries TBD: Add tests BUG=553421 ==========
msramek@chromium.org changed reviewers: + engedy@chromium.org
Hi Balázs, please have a look at your leisure. This is P3, so it doesn't hurry, I'm just cleaning up the tests a bit. Adding this classes own tests would be nice as well, but I guess that is even P4. Thanks, Martin
LGTM % comments. Thanks for the cleanup! https://codereview.chromium.org/1454413002/diff/1/components/history.gypi File components/history.gypi (right): https://codereview.chromium.org/1454413002/diff/1/components/history.gypi#new... components/history.gypi:51: 'history/core/browser/fake_web_history_service.cc', I was meaning to ask if you need to add this to any GN files, but I see now that the try bots answered that question for us. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... File components/history/core/browser/fake_web_history_service.cc (right): https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.cc:9: #include "net/base/completion_callback.h" nit: This is not the include you are looking for. (Whoosh.) https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... File components/history/core/browser/fake_web_history_service.h (right): https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:14: class CompletionCallback; nit: I think this is a different type, the one you need is defined inside WebHistoryService. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:24: nit: -newline https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:34: // Call |WebHistoryService::QueryHistory| to retrieve the fake server-side I would argue that no one would really want to call this manually. Instead, I would explain below for SetRequestOptions() that when the dependent classes call this method on the fake, it will use the fake response. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:36: // TODO(msramek): Currently, the only test that uses this only counts the number nit: Do you actually intend to work on these? If no, I'd suggest just adding a "Note: " to inform fellow contributors about the limitations of this class. Otherwise, I'd suggest actually filing a bug as well to track this. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:44: nit: -newline https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:45: class Request : public history::WebHistoryService::Request { nit: I think this can be moved to the implementation in the cc file. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:79: const scoped_refptr<net::URLRequestContextGetter>& request_context); nit: Same here. Does this really work with just the forward declaration, or does this work because something else already includes the header? https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:83: void SetRequestOptions(bool emulate_success, int emulate_response_code); nit: I think this name is rather confusing. How about SetupFakeResponse or something along those lines? Also, could you please elaborate in the comment about what methods in the interface will actually use the fake values? https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:95: base::Time GetTimeForKeyInQuery(const GURL& url, const std::string& key); nit: Need to include <string>, and I'd include gurl.h as well unless it's popular to fwd-declare it. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:106: typedef std::pair<std::string, base::Time> Visit; I find it hard to believe that this works with just a forward declaration of base::Time, I suspect that something else already includes the header. Could you please check?
https://codereview.chromium.org/1454413002/diff/1/components/history.gypi File components/history.gypi (right): https://codereview.chromium.org/1454413002/diff/1/components/history.gypi#new... components/history.gypi:51: 'history/core/browser/fake_web_history_service.cc', On 2015/11/19 16:02:16, engedy wrote: > I was meaning to ask if you need to add this to any GN files, but I see now that > the try bots answered that question for us. Hmm. And what's more important, this should be only compiled with tests, not with the browser. I added it to the history_core_test_support build target. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... File components/history/core/browser/fake_web_history_service.cc (right): https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.cc:9: #include "net/base/completion_callback.h" On 2015/11/19 16:02:16, engedy wrote: > nit: This is not the include you are looking for. (Whoosh.) Ah :-D I must have taken a wrong turn in the code search. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... File components/history/core/browser/fake_web_history_service.h (right): https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:14: class CompletionCallback; On 2015/11/19 16:02:16, engedy wrote: > nit: I think this is a different type, the one you need is defined inside > WebHistoryService. Done. I even managed two include two different wrong callbacks to .h and .cc. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:24: On 2015/11/19 16:02:16, engedy wrote: > nit: -newline Done. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:34: // Call |WebHistoryService::QueryHistory| to retrieve the fake server-side On 2015/11/19 16:02:16, engedy wrote: > I would argue that no one would really want to call this manually. Instead, I > would explain below for SetRequestOptions() that when the dependent classes call > this method on the fake, it will use the fake response. Done. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:36: // TODO(msramek): Currently, the only test that uses this only counts the number On 2015/11/19 16:02:16, engedy wrote: > nit: Do you actually intend to work on these? If no, I'd suggest just adding a > "Note: " to inform fellow contributors about the limitations of this class. > Otherwise, I'd suggest actually filing a bug as well to track this. No, as this is not needed anywhere yet. But that is the secondary meaning of the TODO keyword :) https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:44: On 2015/11/19 16:02:16, engedy wrote: > nit: -newline Done. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:45: class Request : public history::WebHistoryService::Request { On 2015/11/19 16:02:16, engedy wrote: > nit: I think this can be moved to the implementation in the cc file. Forward declaration is not enough here. We must specify that Request inherits from WebHistoryService::Request to check that CreateRequest (overriding WebHistoryService::CreateRequest) has the correct signature. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:79: const scoped_refptr<net::URLRequestContextGetter>& request_context); On 2015/11/19 16:02:16, engedy wrote: > nit: Same here. Does this really work with just the forward declaration, or > does this work because something else already includes the header? Hm, weird. I was also surprised that this would work with forward declaration, but apparently yes, it's just included elsewhere. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:83: void SetRequestOptions(bool emulate_success, int emulate_response_code); On 2015/11/19 16:02:16, engedy wrote: > nit: I think this name is rather confusing. How about SetupFakeResponse or > something along those lines? Also, could you please elaborate in the comment > about what methods in the interface will actually use the fake values? Agreed. IIRC this is a remnant from an earlier design when this also set other options, some of them being part of the (fake) XHR options, like time period begin and end. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:95: base::Time GetTimeForKeyInQuery(const GURL& url, const std::string& key); On 2015/11/19 16:02:16, engedy wrote: > nit: Need to include <string>, and I'd include gurl.h as well unless it's > popular to fwd-declare it. Done. Both are popular, inclusion is a bit more. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:106: typedef std::pair<std::string, base::Time> Visit; On 2015/11/19 16:02:16, engedy wrote: > I find it hard to believe that this works with just a forward declaration of > base::Time, I suspect that something else already includes the header. Could > you please check? Done. Yes, it's already included. https://codereview.chromium.org/1454413002/diff/40001/components/history/core... File components/history/core/test/BUILD.gn (right): https://codereview.chromium.org/1454413002/diff/40001/components/history/core... components/history/core/test/BUILD.gn:8: "../browser/fake_web_history_service.cc", This is a bit ugly. But I skimmed over a few BUILD files and it isn't too uncommon to include sources from different directories, so I guess it's not a problem. We could theoretically move the class to test/ as well, but that misses the point of having it put visibly right next to WebHistoryService.
LGTM % minor comments. https://codereview.chromium.org/1454413002/diff/1/components/history.gypi File components/history.gypi (right): https://codereview.chromium.org/1454413002/diff/1/components/history.gypi#new... components/history.gypi:51: 'history/core/browser/fake_web_history_service.cc', On 2015/11/30 15:17:10, msramek wrote: > On 2015/11/19 16:02:16, engedy wrote: > > I was meaning to ask if you need to add this to any GN files, but I see now > that > > the try bots answered that question for us. > > Hmm. And what's more important, this should be only compiled with tests, not > with the browser. I added it to the history_core_test_support build target. Acknowledged. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... File components/history/core/browser/fake_web_history_service.h (right): https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:45: class Request : public history::WebHistoryService::Request { On 2015/11/30 15:17:10, msramek wrote: > On 2015/11/19 16:02:16, engedy wrote: > > nit: I think this can be moved to the implementation in the cc file. > > Forward declaration is not enough here. We must specify that Request inherits > from WebHistoryService::Request to check that CreateRequest (overriding > WebHistoryService::CreateRequest) has the correct signature. You could just return the base class (i.e. WebHistoyService::Request), as opposed to using covariant return types. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:106: typedef std::pair<std::string, base::Time> Visit; On 2015/11/30 15:17:10, msramek wrote: > On 2015/11/19 16:02:16, engedy wrote: > > I find it hard to believe that this works with just a forward declaration of > > base::Time, I suspect that something else already includes the header. Could > > you please check? > > Done. Yes, it's already included. In this case, I'd explicitly include the header here, because there is no obvious reason why that should be the case. https://codereview.chromium.org/1454413002/diff/20001/components/history/core... File components/history/core/browser/fake_web_history_service.cc (right): https://codereview.chromium.org/1454413002/diff/20001/components/history/core... components/history/core/browser/fake_web_history_service.cc:12: #include "url/gurl.h" nit: Already included by header. https://codereview.chromium.org/1454413002/diff/20001/components/history/core... File components/history/core/test/BUILD.gn (right): https://codereview.chromium.org/1454413002/diff/20001/components/history/core... components/history/core/test/BUILD.gn:8: "../browser/fake_web_history_service.cc", This looks wrong. Shouldn't this file be physically in the /test folder?
https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... File components/history/core/browser/fake_web_history_service.h (right): https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:45: class Request : public history::WebHistoryService::Request { On 2015/11/30 15:47:08, engedy wrote: > On 2015/11/30 15:17:10, msramek wrote: > > On 2015/11/19 16:02:16, engedy wrote: > > > nit: I think this can be moved to the implementation in the cc file. > > > > Forward declaration is not enough here. We must specify that Request inherits > > from WebHistoryService::Request to check that CreateRequest (overriding > > WebHistoryService::CreateRequest) has the correct signature. > > You could just return the base class (i.e. WebHistoyService::Request), as > opposed to using covariant return types. Oh, right. It's still not possible to have the entire declaration (specifically the inheritance) in the implementation file if this is a member class. So I removed it from FakeWebHistoryService, put into an anonymous namespace, and renamed to FakeRequest for more clarity. https://codereview.chromium.org/1454413002/diff/1/components/history/core/bro... components/history/core/browser/fake_web_history_service.h:106: typedef std::pair<std::string, base::Time> Visit; On 2015/11/30 15:47:08, engedy wrote: > On 2015/11/30 15:17:10, msramek wrote: > > On 2015/11/19 16:02:16, engedy wrote: > > > I find it hard to believe that this works with just a forward declaration of > > > base::Time, I suspect that something else already includes the header. > Could > > > you please check? > > > > Done. Yes, it's already included. > > In this case, I'd explicitly include the header here, because there is no > obvious reason why that should be the case. Done. I actually do find it somewhat obvious that classes working with history will include time, but I get the point. https://codereview.chromium.org/1454413002/diff/20001/components/history/core... File components/history/core/browser/fake_web_history_service.cc (right): https://codereview.chromium.org/1454413002/diff/20001/components/history/core... components/history/core/browser/fake_web_history_service.cc:12: #include "url/gurl.h" On 2015/11/30 15:47:08, engedy wrote: > nit: Already included by header. Done. https://codereview.chromium.org/1454413002/diff/20001/components/history/core... File components/history/core/test/BUILD.gn (right): https://codereview.chromium.org/1454413002/diff/20001/components/history/core... components/history/core/test/BUILD.gn:8: "../browser/fake_web_history_service.cc", On 2015/11/30 15:47:08, engedy wrote: > This looks wrong. Shouldn't this file be physically in the /test folder? I actually commented on this, but accidentally put it into another patchset. So - I looked up a bunch of BUILD.gn files in the code search, and it's not *that* unusual to include something from a different directory. But since it caught your attention, and since the history/ component seems to be quite tidy in this aspect, let me move it to the test/ directory. My original intention was to simply have the original class and the fake right next to each other, for better visibility.
msramek@chromium.org changed reviewers: + markusheintz@chromium.org, sdefresne@chromium.org
+Sylvain, PTAL as the owner of history/ (added a fake testing class) +Markus, PTAL for browsing_data/ (removed code, changed interface)
Ack, LGTM. Thanks for the refactoring!
lgtm https://codereview.chromium.org/1454413002/diff/80001/components/history/core... File components/history/core/test/fake_web_history_service.cc (right): https://codereview.chromium.org/1454413002/diff/80001/components/history/core... components/history/core/test/fake_web_history_service.cc:17: class FakeWebHistoryService; nit: I think this forward-declaration is not required as you include "components/history/core/test/fake_web_history_service.h" earlier.
https://codereview.chromium.org/1454413002/diff/80001/components/history/core... File components/history/core/test/fake_web_history_service.cc (right): https://codereview.chromium.org/1454413002/diff/80001/components/history/core... components/history/core/test/fake_web_history_service.cc:17: class FakeWebHistoryService; On 2015/12/01 14:13:00, sdefresne wrote: > nit: I think this forward-declaration is not required as you include > "components/history/core/test/fake_web_history_service.h" earlier. Ah, right. Done.
Markus: Ping :-)
Ping! :)
On 2015/12/14 20:15:16, msramek wrote: > Ping! :) LGTM
Thanks, everyone! I'm landing this.
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1454413002/#ps100001 (title: "Removed forward declaration.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454413002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454413002/100001
Message was sent while issue was closed.
Description was changed from ========== Move FakeWebHistoryService to components/history 1. split to declaration and definition to .cc and .h 2. since we moved to components/, the constructor can no longer derive the needed keyed services from Profile; instead, the caller must do so and pass them to FakeWebHistoryService constructor; the constructor signature now maches that of WebHistoryService 3. added support for the "num" query parameter, i.e. the limit of returned entries TBD: Add tests BUG=553421 ========== to ========== Move FakeWebHistoryService to components/history 1. split to declaration and definition to .cc and .h 2. since we moved to components/, the constructor can no longer derive the needed keyed services from Profile; instead, the caller must do so and pass them to FakeWebHistoryService constructor; the constructor signature now maches that of WebHistoryService 3. added support for the "num" query parameter, i.e. the limit of returned entries TBD: Add tests BUG=553421 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Move FakeWebHistoryService to components/history 1. split to declaration and definition to .cc and .h 2. since we moved to components/, the constructor can no longer derive the needed keyed services from Profile; instead, the caller must do so and pass them to FakeWebHistoryService constructor; the constructor signature now maches that of WebHistoryService 3. added support for the "num" query parameter, i.e. the limit of returned entries TBD: Add tests BUG=553421 ========== to ========== Move FakeWebHistoryService to components/history 1. split to declaration and definition to .cc and .h 2. since we moved to components/, the constructor can no longer derive the needed keyed services from Profile; instead, the caller must do so and pass them to FakeWebHistoryService constructor; the constructor signature now maches that of WebHistoryService 3. added support for the "num" query parameter, i.e. the limit of returned entries TBD: Add tests BUG=553421 Committed: https://crrev.com/b932634b22dcde4d847a2a467a0c07c7c90b413b Cr-Commit-Position: refs/heads/master@{#366071} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b932634b22dcde4d847a2a467a0c07c7c90b413b Cr-Commit-Position: refs/heads/master@{#366071} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
