Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1829733002:
Retrieve the web&app activity enabled state from WebHistoryService. (Closed)
DescriptionRetrieve the web&app activity enabled state from WebHistoryService.
This replaces the placeholder method HasOtherFormsOfBrowsingHistory() with
an asynchronous QueryWebAndAppActivity(), and exposes this asynchronous
call through history_notice_utils.
For reference:
Design doc: https://docs.google.com/document/d/1ZMDSAd44KmzKhqXPjobZOf9rZezs6VqiBnqpDD0auCU/
Mocks: https://docs.google.com/presentation/d/1rpR3xB3aYFzXD0U--piuMq3y4XAmoYdawA2HC1cuEjM/
BUG=595332
Committed: https://crrev.com/ba5b1e9e5644881affcfff2b34ec765883e3d336
Cr-Commit-Position: refs/heads/master@{#383940}
Patch Set 1 #Patch Set 2 : Fix return value #Patch Set 3 : Early return. #
Total comments: 10
Patch Set 4 : Addressed comments. #
Total comments: 2
Patch Set 5 : Nit. #
Messages
Total messages: 21 (8 generated)
msramek@chromium.org changed reviewers: + msarda@chromium.org, sdefresne@chromium.org
Hi guys! Mihai - this is a followup to our discussion in https://codereview.chromium.org/1824333002/, which I'm now closing. Sorry I couldn't get to it sooner in the course of the day. Sylvain - I still have in mind your comment about the JSON parser, but I want to do this change first, to unblock Mihai to hook the logic into UI. The API used in this CL is already live, I tried it out manually. I didn't add a test yet, as it seems that we don't have infrastructure in web_history_service_unittest for anything more meaningful than Set value=1 -> generate response -> parse JSON and check that value=1. But I'll think of something. Please have a look! Thanks, Martin
(apologies for patching the obvious mistakes after I sent this to review, but my UI that calls this is in a separate CL, so I only noticed later)
https://codereview.chromium.org/1829733002/diff/40001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1829733002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:16: // TODO(msramek): The API to query other forms of browsing history is not I really dislike this. The function prototype (taking a ProfileSyncService and WebHistoryService) is making our iOS tests more complex (as we have to instantiate those services and their dependencies as they are not mockable). This is already painful, but if the function is not even using them, this is doubly so. Can you instead work on a proper design instead? Then we wouldn't have to patch things up every time and include such hacks. https://codereview.chromium.org/1829733002/diff/40001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1829733002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.h:8: #include "base/callback.h" nit: use base/callback_forward.h in the header and base/callback.h in the implementation. https://codereview.chromium.org/1829733002/diff/40001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1829733002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:453: request->Start(); You have a race condition here if the request finish (i.e. call completion_callback) before you add it to pending_web_and_app_activity_requests_ I think. I see this is the pattern used in the rest of the file though, so maybe you can leave as is. I would do it like this: // Request is deleted in the completion callback. Request* request = CreateRequest(url, completion_callback); pending_web_and_app_activity_requests_.insert(request); request->Start(); https://codereview.chromium.org/1829733002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:509: scoped_ptr<base::DictionaryValue> response_value; The pattern for the other completion handler is to do the following: pending_web_and_app_activity_requests_.erase(request); scoped_ptr<Request> request_ptr(request); This is safer as it will correctly cleanup the request even if the function is changed to have an early return. https://codereview.chromium.org/1829733002/diff/40001/components/history/core... File components/history/core/browser/web_history_service.h (right): https://codereview.chromium.org/1829733002/diff/40001/components/history/core... components/history/core/browser/web_history_service.h:194: std::set<Request*> pending_web_and_app_activity_requests_; I do not see the code cancelling those requests on shutdown. Thus I think you are leaking those objects if Shutdown is called while some requests are in flight. You probably want to add the following to the destructor: STLDeleteElements(&pending_web_and_app_activity_requests_);
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1829733002/diff/40001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1829733002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:16: // TODO(msramek): The API to query other forms of browsing history is not On 2016/03/24 09:50:07, sdefresne wrote: > I really dislike this. The function prototype (taking a ProfileSyncService and > WebHistoryService) is making our iOS tests more complex (as we have to > instantiate those services and their dependencies as they are not mockable). > This is already painful, but if the function is not even using them, this is > doubly so. > > Can you instead work on a proper design instead? Then we wouldn't have to patch > things up every time and include such hacks. ProfileSyncService* and WebHistoryService* are in the signature precisely because of iOS. I would normally just use Profile*, but we use different BrowserContextKeyedService factories on iOS. I cleared this signature with Mihai, and I have to rely on his judgement here, because I don't know what are the requirements for iOS UI. We know that the response has to be one asynchronous callback, and how exactly this method internally assembles the value is not important. I already have a Desktop UI CL ready, so I'm sure that this design works for me, and if Mihai is happy, I do not foresee any future change of the interface. Naturally, we could replace ProfileSyncService* and WebHistoryService* in this method with something else that is mockable, but then this component has no point at all - it would literally just do a boolean operation on provided values. That's not what Mihai and I intended - we have this component to assure that we are consistent in how we check the status of sync and history, and it already proved valuable, since we recently realized that it might not be as straightforward to determine what does it mean that "sync is running". Regarding the early return - the problem is that the API I need to call does not exist yet, and folks working on the serverside part were not able to tell me how it's going to look like yet. It might be bundled to client=web_app, or it might be something completely different. I admit the early return here was a bad call, since it makes the method untestable. What we can do is keep the dummy "return false;" method in WebHistoryService to represent the not-yet implemented API, which will make it testable by overriding through e.g. FakeWebHistoryService. Regardless of how that part might change when I learn how exactly the API is going to look like, the interface between this layer and Desktop/Android/iOS UI will not need to change. The tests involving WebHistoryService might still change, but that's unfortunately out of my control. Does that work for you (both)? https://codereview.chromium.org/1829733002/diff/40001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1829733002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.h:8: #include "base/callback.h" On 2016/03/24 09:50:07, sdefresne wrote: > nit: use base/callback_forward.h in the header and base/callback.h in the > implementation. Done. https://codereview.chromium.org/1829733002/diff/40001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1829733002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:453: request->Start(); On 2016/03/24 09:50:07, sdefresne wrote: > You have a race condition here if the request finish (i.e. call > completion_callback) before you add it to pending_web_and_app_activity_requests_ > I think. I see this is the pattern used in the rest of the file though, so maybe > you can leave as is. > > I would do it like this: > > // Request is deleted in the completion callback. > Request* request = CreateRequest(url, completion_callback); > pending_web_and_app_activity_requests_.insert(request); > request->Start(); Done. Yes, I used the same approach as in other Query* methods. In reality it isn't a race condition, since request->Start() will always end up posting a task on the same thread, so the callback will not run before this method finishes. And the unnecessary scoped_ptr looks like a copy-paste from QueryHistory which actually needs it. I'm happy to change the order to make sure that we don't make assumptions on the internals of Request, but I'd prefer not to change the other four occurrences in this CL. https://codereview.chromium.org/1829733002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:509: scoped_ptr<base::DictionaryValue> response_value; On 2016/03/24 09:50:07, sdefresne wrote: > The pattern for the other completion handler is to do the following: > > pending_web_and_app_activity_requests_.erase(request); > scoped_ptr<Request> request_ptr(request); > > This is safer as it will correctly cleanup the request even if the function is > changed to have an early return. Done. I don't expect early returns in these straightforward methods, but I agree that a scoped_ptr would be safer. Let me change it in ExpireHistoryCompletionCallback as well (since I used that as the template). https://codereview.chromium.org/1829733002/diff/40001/components/history/core... File components/history/core/browser/web_history_service.h (right): https://codereview.chromium.org/1829733002/diff/40001/components/history/core... components/history/core/browser/web_history_service.h:194: std::set<Request*> pending_web_and_app_activity_requests_; On 2016/03/24 09:50:07, sdefresne wrote: > I do not see the code cancelling those requests on shutdown. Thus I think you > are leaking those objects if Shutdown is called while some requests are in > flight. You probably want to add the following to the destructor: > > STLDeleteElements(&pending_web_and_app_activity_requests_); Thanks for catching this. Done.
Friendly ping. Mihai, can you please confirm whether the current interface works well for you on iOS, so we can move forward?
API LGTM.
msramek@chromium.org changed reviewers: + sky@chromium.org
Thanks. Hmm, but Sylvain is OOO, and we cannot wait a week. +Scott, can you please take over the review and look at the changes in WebHistoryService? I'll leave Sylvain as TBR if he has any more comments when he returns.
Description was changed from ========== Retrieve the web&app activity enabled state from WebHistoryService. This replaces the placeholder method HasOtherFormsOfBrowsingHistory() with an asynchronous QueryWebAndAppActivity(), and exposes this asynchronous call through history_notice_utils. BUG=595332 ========== to ========== Retrieve the web&app activity enabled state from WebHistoryService. This replaces the placeholder method HasOtherFormsOfBrowsingHistory() with an asynchronous QueryWebAndAppActivity(), and exposes this asynchronous call through history_notice_utils. For reference: Design doc: https://docs.google.com/document/d/1ZMDSAd44KmzKhqXPjobZOf9rZezs6VqiBnqpDD0auCU/ Mocks: https://docs.google.com/presentation/d/1rpR3xB3aYFzXD0U--piuMq3y4XAmoYdawA2HC... BUG=595332 ==========
lgtm
https://codereview.chromium.org/1829733002/diff/80001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1829733002/diff/80001/components/history/core... components/history/core/browser/web_history_service.cc:530: callback.Run(success && web_and_app_activity_enabled); nit: this could be simplified to "callback.Run(web_and_app_activity_enabled)" since "web_and_app_activity_enabled" is set to true only if success is true.
Thanks a lot, Sylvain! But now please get back to your vacation :) https://codereview.chromium.org/1829733002/diff/80001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1829733002/diff/80001/components/history/core... components/history/core/browser/web_history_service.cc:530: callback.Run(success && web_and_app_activity_enabled); On 2016/03/29 21:13:46, sdefresne (OOO till 4th April) wrote: > nit: this could be simplified to "callback.Run(web_and_app_activity_enabled)" > since "web_and_app_activity_enabled" is set to true only if success is true. Done.
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1829733002/#ps100001 (title: "Nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829733002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829733002/100001
Message was sent while issue was closed.
Description was changed from ========== Retrieve the web&app activity enabled state from WebHistoryService. This replaces the placeholder method HasOtherFormsOfBrowsingHistory() with an asynchronous QueryWebAndAppActivity(), and exposes this asynchronous call through history_notice_utils. For reference: Design doc: https://docs.google.com/document/d/1ZMDSAd44KmzKhqXPjobZOf9rZezs6VqiBnqpDD0auCU/ Mocks: https://docs.google.com/presentation/d/1rpR3xB3aYFzXD0U--piuMq3y4XAmoYdawA2HC... BUG=595332 ========== to ========== Retrieve the web&app activity enabled state from WebHistoryService. This replaces the placeholder method HasOtherFormsOfBrowsingHistory() with an asynchronous QueryWebAndAppActivity(), and exposes this asynchronous call through history_notice_utils. For reference: Design doc: https://docs.google.com/document/d/1ZMDSAd44KmzKhqXPjobZOf9rZezs6VqiBnqpDD0auCU/ Mocks: https://docs.google.com/presentation/d/1rpR3xB3aYFzXD0U--piuMq3y4XAmoYdawA2HC... BUG=595332 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Retrieve the web&app activity enabled state from WebHistoryService. This replaces the placeholder method HasOtherFormsOfBrowsingHistory() with an asynchronous QueryWebAndAppActivity(), and exposes this asynchronous call through history_notice_utils. For reference: Design doc: https://docs.google.com/document/d/1ZMDSAd44KmzKhqXPjobZOf9rZezs6VqiBnqpDD0auCU/ Mocks: https://docs.google.com/presentation/d/1rpR3xB3aYFzXD0U--piuMq3y4XAmoYdawA2HC... BUG=595332 ========== to ========== Retrieve the web&app activity enabled state from WebHistoryService. This replaces the placeholder method HasOtherFormsOfBrowsingHistory() with an asynchronous QueryWebAndAppActivity(), and exposes this asynchronous call through history_notice_utils. For reference: Design doc: https://docs.google.com/document/d/1ZMDSAd44KmzKhqXPjobZOf9rZezs6VqiBnqpDD0auCU/ Mocks: https://docs.google.com/presentation/d/1rpR3xB3aYFzXD0U--piuMq3y4XAmoYdawA2HC... BUG=595332 Committed: https://crrev.com/ba5b1e9e5644881affcfff2b34ec765883e3d336 Cr-Commit-Position: refs/heads/master@{#383940} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ba5b1e9e5644881affcfff2b34ec765883e3d336 Cr-Commit-Position: refs/heads/master@{#383940} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
