Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1983073002:
Query the existence other forms of browsing history. (Closed)
|
Created:
4 years, 7 months ago by msramek Modified:
4 years, 7 months ago CC:
albertb+watch_chromium.org, blundell+watchlist_chromium.org, chromium-reviews, dbeam+watch-options_chromium.org, droger+watchlist_chromium.org, michaelpg+watch-options_chromium.org, sdefresne+watchlist_chromium.org, sync-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionQuery the existence other forms of browsing history.
This CL implements the query for other forms of browsing history, which
is necessary to show the notices in the Clear Browsing Data dialog on
all platforms. Previously, the query was represented by a dummy method
WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an
actual endpoint on the Sync server to answer the query.
More detailed description of the change:
1. sync/protocol/
Adapt history_status.proto from cl/122116426 and add it to Chromium code.
2. components/history/
Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory()
with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of
the existing WebHistoryService insfrastructure, but ReadResponse is replaced
by MessageLite::ParseFromString() since we're receiving a protobuf.
Furthermore, the query URL is not static, as in other WebHistoryService calls,
but has to be generated from the browser user agent and channel.
3. components/browsing_data_ui/
Update the logic to include calls to QueryOtherFormsOfBrowsingHistory().
This call and QueryWebAndAppActivity() call should run in parallel to save
time, since they are both HTTP requests.
4. chrome/
Provide the user agent string and the channel info from
the Desktop and Android callsites (iOS is in a different
codebase)
BUG=614652
TBR=dbeam@chromium.org,dfalcantara@chromium.org
Committed: https://crrev.com/6d43c1934d9a3a090e16a1346a2ada29b7e1a86c
Cr-Commit-Position: refs/heads/master@{#395894}
Patch Set 1 : #
Total comments: 29
Patch Set 2 : Addressed comments. #
Total comments: 6
Patch Set 3 : Addressed comments, fixed bots #Patch Set 4 : Wrong include in pref_service_bridge #
Total comments: 2
Patch Set 5 : Addressed comments, re-added protobuf #
Total comments: 21
Patch Set 6 : #Patch Set 7 : Rebase #Patch Set 8 : Forgot one callsite #
Total comments: 6
Patch Set 9 : Addressed comments. #
Total comments: 2
Patch Set 10 : Nit. #Messages
Total messages: 43 (16 generated)
Description was changed from ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from all callsites. BUG= ========== to ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from all callsites. BUG=595332 ==========
Description was changed from ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from all callsites. BUG=595332 ========== to ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from the Desktop and Android callsites (iOS is in a different codebase) BUG=595332 ==========
Patchset #1 (id:1) has been deleted
msramek@chromium.org changed reviewers: + msarda@chromium.org, pavely@chromium.org, sdefresne@chromium.org
Hi guys, Can you have a look? The CL is more-or-less complete, but I haven't had a chance to test, since the serverside code hasn't landed yet to my knowledge. But in the meantime, please take a pass if you at least agree with the approach :) kevinliu@: FYI, as you're working on the server counterpart. pavely@: As a sync/ owner. msarda@: For browsing_data_ui/. Also, can you please update the one iOS callsite accordingly? I know I promised you the interface wouldn't change, but I didn't foresee that we would need the channel info and user agent for the queries, and we cannot get it from inside components/. sdefresne@: As a history/ owner. I wanted to be safe and reuse the existing infrastructure of WebHistoryService for this new query, especially since I'll have to merge this CL. However, if you think that it's a stretch, we can just write a separate service in browsing_data_ui/. We'd reuse some of the WebHistoryService logic though. Thanks! Martin
https://codereview.chromium.org/1983073002/diff/20001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1983073002/diff/20001/components/history/core... components/history/core/browser/web_history_service.cc:486: GetSyncServiceURL(*base::CommandLine::ForCurrentProcess(), channel); Could you confirm with Kevin that this is correct url. I think it should have some suffix. https://codereview.chromium.org/1983073002/diff/20001/components/history/core... components/history/core/browser/web_history_service.cc:488: Request* request = CreateRequest(url, completion_callback); Looks like request is not owned by any other object. Does it get leaked if WebHistoryService gets destroyed while request is still in progress? https://codereview.chromium.org/1983073002/diff/20001/sync/protocol/history_s... File sync/protocol/history_status.proto (right): https://codereview.chromium.org/1983073002/diff/20001/sync/protocol/history_s... sync/protocol/history_status.proto:13: message HistoryStatusRequest { HistoryStatusRequest is not used. Consider removing it.
I only reviewed the changes in components/browsing_data_ui/history_notice_utils*. Some nits and a comment. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:15: class MergeTwoBooleanCallbacks { Please comment this class as it is not really to understand what it does. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:23: void Callback(bool response) { Nit: s/Callback/RunCallback https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:33: base::Callback<void(bool)> merged_callback_; Optional nit: I would rename this to target_callback_ https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:63: void ShouldPopupDialogAboutOtherFormsOfBrowsingHistory( The function is not more complex: it waits for 2 callbacks to be called, merges the responses and then calls the callback. I think it deserves a unit test. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.h:51: const version_info::Channel channel, Is this missing a reference: const version_info::Channel& channel ?
https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:15: class MergeTwoBooleanCallbacks { +1 https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:26: if (++callbacks_ == 2) { Avoid hard coded magic values. BTW, I found the logic really hard to understand, can you maybe rework and rename as: void OnQueryResult(bool response) { DCHECK_LT(call_count_, expected_call_count_); final_response_ &= response; if (++call_count_ < expected_call_count_) return; callback_.Run(final_response_); base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this); } https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:33: base::Callback<void(bool)> merged_callback_; On 2016/05/18 08:58:07, msarda wrote: > Optional nit: I would rename this to target_callback_ I would call it callback_ https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:34: bool final_response_; bool final_response_ = false; https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:63: void ShouldPopupDialogAboutOtherFormsOfBrowsingHistory( On 2016/05/18 08:58:07, msarda wrote: > The function is not more complex: it waits for 2 callbacks to be called, merges > the responses and then calls the callback. I think it deserves a unit test. +1 https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.h:51: const version_info::Channel channel, On 2016/05/18 08:58:07, msarda wrote: > Is this missing a reference: const version_info::Channel& channel ? no version_info::Channel is an enum value, no need to pass by reference (it's an int) https://codereview.chromium.org/1983073002/diff/20001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1983073002/diff/20001/components/history/core... components/history/core/browser/web_history_service.cc:178: if (user_agent_ != "") { if (!user_agent_.empty()) {
https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.h:51: const version_info::Channel channel, On 2016/05/18 14:29:41, sdefresne wrote: > On 2016/05/18 08:58:07, msarda wrote: > > Is this missing a reference: const version_info::Channel& channel ? > > no version_info::Channel is an enum value, no need to pass by reference (it's an > int) Do we need the const then?
On 2016/05/18 14:32:54, msarda wrote: > https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... > File components/browsing_data_ui/history_notice_utils.h (right): > > https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... > components/browsing_data_ui/history_notice_utils.h:51: const > version_info::Channel channel, > On 2016/05/18 14:29:41, sdefresne wrote: > > On 2016/05/18 08:58:07, msarda wrote: > > > Is this missing a reference: const version_info::Channel& channel ? > > > > no version_info::Channel is an enum value, no need to pass by reference (it's > an > > int) > > Do we need the const then? Probably not.
I addressed comments and added a test for the browsing_data_ui/ component. PTAL! https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:15: class MergeTwoBooleanCallbacks { On 2016/05/18 14:29:40, sdefresne wrote: > +1 Done. Sorry about that, I hoped it would be clear from the usage, but that's of course on the very bottom of this file. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:23: void Callback(bool response) { On 2016/05/18 08:58:07, msarda wrote: > Nit: s/Callback/RunCallback Done. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:26: if (++callbacks_ == 2) { On 2016/05/18 14:29:40, sdefresne wrote: > Avoid hard coded magic values. > > BTW, I found the logic really hard to understand, can you maybe rework and > rename as: > > void OnQueryResult(bool response) { > DCHECK_LT(call_count_, expected_call_count_); > final_response_ &= response; > if (++call_count_ < expected_call_count_) > return; > > callback_.Run(final_response_); > base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this); > } Done. Also changed 2 to |expected_call_count_|. I just didn't want it to overgeneralize either, since this is just a helper class in an unnamed namespace. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:33: base::Callback<void(bool)> merged_callback_; On 2016/05/18 14:29:40, sdefresne wrote: > On 2016/05/18 08:58:07, msarda wrote: > > Optional nit: I would rename this to target_callback_ > > I would call it callback_ I would prefer keeping some kind of adjective to distinguish the inbound callbacks and the one outbound callback. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:34: bool final_response_; On 2016/05/18 14:29:40, sdefresne wrote: > bool final_response_ = false; This is currently initialized in the constructor as true. I can move the initialization here, but in any case, we're doing a boolean product, so we must start with true. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:63: void ShouldPopupDialogAboutOtherFormsOfBrowsingHistory( On 2016/05/18 14:29:40, sdefresne wrote: > On 2016/05/18 08:58:07, msarda wrote: > > The function is not more complex: it waits for 2 callbacks to be called, > merges > > the responses and then calls the callback. I think it deserves a unit test. > > +1 Done, added a unittest. Two things that I noticed during testing and fixed here: - It's easier to use a fake SyncService than ProfileSyncService, so I replaced that in the method signature. This has no effect on callers, since the latter inherits from the former. It's also cleaner, because we don't really depend on its 'profile-ness'. - So far we relied on the fact that WebHistoryService will be null when history (deletion) is not synced, because WebHistoryServiceFactory will not create it in that case. But it's easier to test if that condition is explicit. And more explicit is better anyways. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.h:51: const version_info::Channel channel, On 2016/05/18 14:32:54, msarda wrote: > On 2016/05/18 14:29:41, sdefresne wrote: > > On 2016/05/18 08:58:07, msarda wrote: > > > Is this missing a reference: const version_info::Channel& channel ? > > > > no version_info::Channel is an enum value, no need to pass by reference (it's > an > > int) > > Do we need the const then? Correct, this is an extra const, it's not needed. Done. https://codereview.chromium.org/1983073002/diff/20001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1983073002/diff/20001/components/history/core... components/history/core/browser/web_history_service.cc:178: if (user_agent_ != "") { On 2016/05/18 14:29:41, sdefresne wrote: > if (!user_agent_.empty()) { Done. https://codereview.chromium.org/1983073002/diff/20001/components/history/core... components/history/core/browser/web_history_service.cc:486: GetSyncServiceURL(*base::CommandLine::ForCurrentProcess(), channel); On 2016/05/17 22:56:48, pavely wrote: > Could you confirm with Kevin that this is correct url. I think it should have > some suffix. Fixed. Yes, that would be the very first thing I would discover during manual testing. https://codereview.chromium.org/1983073002/diff/20001/components/history/core... components/history/core/browser/web_history_service.cc:488: Request* request = CreateRequest(url, completion_callback); On 2016/05/17 22:56:48, pavely wrote: > Looks like request is not owned by any other object. Does it get leaked if > WebHistoryService gets destroyed while request is still in progress? Done. Added the call to the destructor. It would be perhaps best to refactor the class to use ScopedVector-s instead, but I don't want to touch too much in this CL. https://codereview.chromium.org/1983073002/diff/20001/sync/protocol/history_s... File sync/protocol/history_status.proto (right): https://codereview.chromium.org/1983073002/diff/20001/sync/protocol/history_s... sync/protocol/history_status.proto:13: message HistoryStatusRequest { On 2016/05/17 22:56:48, pavely wrote: > HistoryStatusRequest is not used. Consider removing it. Done. kevinliu@, perhaps you want to remove it from cl/122116426 as well. There is no data from the client to provide, so we would just send an empty POST request. But that reminds me that I still need to set e.g. whitespace as post data, otherwise WebHistoryService::Request will default to GET.
msramek@chromium.org changed reviewers: + cbentzel@chromium.org
+cbentzel@, can you please review adding "+net" to components/browsing_data_ui/DEPS? It's needed in the unittest to instantiate FakeWebHistoryService, which requires net::UrlRequestContext, and also to fake the net error codes.
Didn't have time for a second look, just wanted to reply to one of your comments. https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:34: bool final_response_; On 2016/05/18 19:38:37, msramek wrote: > On 2016/05/18 14:29:40, sdefresne wrote: > > bool final_response_ = false; > > This is currently initialized in the constructor as true. I can move the > initialization here, but in any case, we're doing a boolean product, so we must > start with true. Oh, yes I should have said "bool final_response_ = true;". My real comment was that you should either use default value in definition or initialisation in the constructor consistently (callbacks_ with default value at definition but final_response_ initialised in the constructor looks inconsistent). I'm fine one way or another, just want consistency.
LGTM with nits for for components/browsing_data_ui/history_notice_utils.* https://codereview.chromium.org/1983073002/diff/40001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils_unittest.cc (right): https://codereview.chromium.org/1983073002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils_unittest.cc:90: got_result_ = false; Nit: Can this be a local variable instead of a member of the class? I think you should be able to bind a pointer to a local variable it in base::Bind. https://codereview.chromium.org/1983073002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils_unittest.cc:155: // If we're syncing... Do not use "we" in comments. (What is we here? Chrome / The Test/ The programmer/ other?). https://codereview.chromium.org/1983073002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils_unittest.cc:178: Expect(false); When I first read the code, I though this expect will always fail (Expect(false) sounds like a failure). But then I realized, Expect is actually an internal method of the test, not a generic EXPECT statement. Consider renaming Expect to something more explicit. Maybe something like ExpectShouldPopupDialogAboutOtherFormsOfBrowsingHistoryWithResult(false) would make the code more readable.
sync/ lgtm
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
I addressed the comments, and also fixed the red bots: - missing direct deps in BUILD.gn - improved the URL recognition in FakeWebHistory service; I accidentally broke an unrelated test by comparing the URLs directly, which failed if additional query parameters were included - fixed the memory leak reported by ASAN, which seems to come from MergeBooleanCallbacks not having been destroyed soon enough https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1983073002/diff/20001/components/browsing_dat... components/browsing_data_ui/history_notice_utils.cc:34: bool final_response_; On 2016/05/18 19:42:39, sdefresne wrote: > On 2016/05/18 19:38:37, msramek wrote: > > On 2016/05/18 14:29:40, sdefresne wrote: > > > bool final_response_ = false; > > > > This is currently initialized in the constructor as true. I can move the > > initialization here, but in any case, we're doing a boolean product, so we > must > > start with true. > > Oh, yes I should have said "bool final_response_ = true;". > > My real comment was that you should either use default value in definition or > initialisation in the constructor consistently (callbacks_ with default value at > definition but final_response_ initialised in the constructor looks > inconsistent). I'm fine one way or another, just want consistency. Makes sense. Let me move it up to the constructor then; that's usually my preference, since I need to initialize some member variables from constructor params anyway. https://codereview.chromium.org/1983073002/diff/40001/components/browsing_dat... File components/browsing_data_ui/history_notice_utils_unittest.cc (right): https://codereview.chromium.org/1983073002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils_unittest.cc:90: got_result_ = false; On 2016/05/19 13:35:44, msarda wrote: > Nit: Can this be a local variable instead of a member of the class? I think you > should be able to bind a pointer to a local variable it in base::Bind. Done. https://codereview.chromium.org/1983073002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils_unittest.cc:155: // If we're syncing... On 2016/05/19 13:35:44, msarda wrote: > Do not use "we" in comments. (What is we here? Chrome / The Test/ The > programmer/ other?). Done. I agree with you on this, but it's not that uncommon to colloquially use "we" in comments in Chromium. https://codereview.chromium.org/1983073002/diff/40001/components/browsing_dat... components/browsing_data_ui/history_notice_utils_unittest.cc:178: Expect(false); On 2016/05/19 13:35:44, msarda wrote: > When I first read the code, I though this expect will always fail (Expect(false) > sounds like a failure). But then I realized, Expect is actually an internal > method of the test, not a generic EXPECT statement. > > Consider renaming Expect to something more explicit. Maybe something like > ExpectShouldPopupDialogAboutOtherFormsOfBrowsingHistoryWithResult(false) would > make the code more readable. Done. But I would argue that shorter was more readable. Once you know what "Expect" means, you can skim through the tests faster. This is of course my fault for naming the method so long, but I really don't know how else to call it...
net/ DEPS LGTM
https://codereview.chromium.org/1983073002/diff/120001/components/browsing_da... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1983073002/diff/120001/components/browsing_da... components/browsing_data_ui/history_notice_utils.h:50: void ShouldPopupDialogAboutOtherFormsOfBrowsingHistory( I just realized this. Please keep the old form of this function (and its previous implementation) so that we do not break the Chrome iOS roller downstream. We can remove it once the Chrome iOS roller has rolled this fix and once the matching downstream CL has been landed.
Patchset #5 (id:140001) has been deleted
msramek@chromium.org changed reviewers: + kevinliu@google.com
I tested this together with kevinliu@ and debugged the communication with the Sync server. It works now, and I'm prepared to land the CL. I needed to make several changes; everything is explained in the comments of Patchset #5. PTAL! kevinliu@: history_status.proto pavely@: the usage of Sync-related classes in WebHistoryService::QueryOtherFormsOfBrowsingHistory sdefresne@: the overall changes to WebHistoryService msarda@: changes in history_notice_utils.cc; and please also update iOS downstream accordingly Thanks! Martin https://codereview.chromium.org/1983073002/diff/120001/components/browsing_da... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1983073002/diff/120001/components/browsing_da... components/browsing_data_ui/history_notice_utils.h:50: void ShouldPopupDialogAboutOtherFormsOfBrowsingHistory( On 2016/05/23 12:37:21, msarda wrote: > I just realized this. Please keep the old form of this function (and its > previous implementation) so that we do not break the Chrome iOS roller > downstream. We can remove it once the Chrome iOS roller has rolled this fix and > once the matching downstream CL has been landed. Done. As discussed elsewhere, I'm still changing ProfileSyncService to SyncService to avoid extra includes and dependencies. This should not break downstream, as one inherits from the other. https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... components/history/core/browser/web_history_service.cc:202: const std::string& mime_type) override { Another new method to change the mime type to application/octet-stream, which is used by Sync. This could theoretically be SetMimeType, but it probably makes more sense to directly set it together with the data themselves. https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... components/history/core/browser/web_history_service.cc:219: std::unique_ptr<std::string> post_data_; Changed this to a std::unique_ptr<>, so that we can distinguish between an empty string and no data. This is necessary, because we have no post data to sent; the protobuf that we agreed upon is empty, and thus serializes to an empty string. The previous attempt of sending a space (" ") to represent no post data was rejected by the server. https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... components/history/core/browser/web_history_service.cc:515: // Set the Sync-specific user agent. I realized that I was setting the user agent wrong. I need the Sync-specific user agent which is obtainable from components/sync_driver, so I don't need to pass it from the embedder. However, LocalDeviceInfoProviderImpl needs to know whether this is a tablet, so I'm instead passing that one boolean from the embedder. https://codereview.chromium.org/1983073002/diff/160001/sync/protocol/history_... File sync/protocol/history_status.proto (right): https://codereview.chromium.org/1983073002/diff/160001/sync/protocol/history_... sync/protocol/history_status.proto:12: message HistoryStatusRequest { After offline discussions with kevinliu@, I'm readding the request protobuf for consistency, and explicitly serialize it to a string to be sent with the post request (although I know that the result of the serialization is an empty string).
LGTM on the proto file On Mon, May 23, 2016 at 12:18 PM, <msramek@chromium.org> wrote: > I tested this together with kevinliu@ and debugged the communication with > the > Sync server. It works now, and I'm prepared to land the CL. > > I needed to make several changes; everything is explained in the comments > of > Patchset #5. > > PTAL! > > kevinliu@: history_status.proto > pavely@: the usage of Sync-related classes in > WebHistoryService::QueryOtherFormsOfBrowsingHistory > sdefresne@: the overall changes to WebHistoryService > msarda@: changes in history_notice_utils.cc; and please also update iOS > downstream accordingly > > Thanks! > Martin > > > > https://codereview.chromium.org/1983073002/diff/120001/components/browsing_da... > File components/browsing_data_ui/history_notice_utils.h (right): > > > https://codereview.chromium.org/1983073002/diff/120001/components/browsing_da... > components/browsing_data_ui/history_notice_utils.h:50: void > ShouldPopupDialogAboutOtherFormsOfBrowsingHistory( > On 2016/05/23 12:37:21, msarda wrote: > > I just realized this. Please keep the old form of this function (and > its > > previous implementation) so that we do not break the Chrome iOS roller > > downstream. We can remove it once the Chrome iOS roller has rolled > this fix and > > once the matching downstream CL has been landed. > > Done. As discussed elsewhere, I'm still changing ProfileSyncService to > SyncService to avoid extra includes and dependencies. This should not > break downstream, as one inherits from the other. > > > https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... > File components/history/core/browser/web_history_service.cc (right): > > > https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... > components/history/core/browser/web_history_service.cc:202: const > std::string& mime_type) override { > Another new method to change the mime type to application/octet-stream, > which is used by Sync. > > This could theoretically be SetMimeType, but it probably makes more > sense to directly set it together with the data themselves. > > > https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... > components/history/core/browser/web_history_service.cc:219: > std::unique_ptr<std::string> post_data_; > Changed this to a std::unique_ptr<>, so that we can distinguish between > an empty string and no data. This is necessary, because we have no post > data to sent; the protobuf that we agreed upon is empty, and thus > serializes to an empty string. > > The previous attempt of sending a space (" ") to represent no post data > was rejected by the server. > > > https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... > components/history/core/browser/web_history_service.cc:515: // Set the > Sync-specific user agent. > I realized that I was setting the user agent wrong. > > I need the Sync-specific user agent which is obtainable from > components/sync_driver, so I don't need to pass it from the embedder. > > However, LocalDeviceInfoProviderImpl needs to know whether this is a > tablet, so I'm instead passing that one boolean from the embedder. > > > https://codereview.chromium.org/1983073002/diff/160001/sync/protocol/history_... > File sync/protocol/history_status.proto (right): > > > https://codereview.chromium.org/1983073002/diff/160001/sync/protocol/history_... > sync/protocol/history_status.proto:12: message HistoryStatusRequest { > After offline discussions with kevinliu@, I'm readding the request > protobuf for consistency, and explicitly serialize it to a string to be > sent with the post request (although I know that the result of the > serialization is an empty string). > > https://codereview.chromium.org/1983073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lg https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils.cc:122: false /* !history_service->HasOtherFormsOfBrowsingHistory() */)) { Can you remove this /* comment */? https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils.h:54: bool is_tablet, Since you already indirectly depends on ui/base (through components/history/core/browser), why not take the device form factor here instead of a boolean? #include "ui/base/device_form_factor.h" void ShouldPopupDialogAboutOtherFormsOfBrowsingHistory( const sync_driver::SyncService* sync_service, history::WebHistoryService* history_service, version_info::Channel channel, ui::DeviceFormFactor form_factor, base::Callback<void(bool)> callback); https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils.h:58: // TODO(msarda): Remove this when iOS calls the correct version. Can you create a bug and use the format "TODO(crbug/xxxxxx)" instead? https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... File components/browsing_data_ui/history_notice_utils_unittest.cc (right): https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils_unittest.cc:44: void SetSyncActive(bool active) { nit, style: this should be "set_sync_active" as it is inline. https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils_unittest.cc:48: void SetActiveDataTypes(syncer::ModelTypeSet data_types) { ditto https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils_unittest.cc:52: void SetUsingSecondaryPassphrase(bool passphrase) { ditto https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... components/history/core/browser/web_history_service.cc:201: void SetPostData(const std::string& post_data, Style guide discourage function/method overloading and encourages using explicit names. Can this method be called SetPostDataAndType()? https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... components/history/core/browser/web_history_service.cc:219: std::unique_ptr<std::string> post_data_; On 2016/05/23 19:18:12, msramek wrote: > Changed this to a std::unique_ptr<>, so that we can distinguish between an empty > string and no data. This is necessary, because we have no post data to sent; the > protobuf that we agreed upon is empty, and thus serializes to an empty string. > > The previous attempt of sending a space (" ") to represent no post data was > rejected by the server. Can you use base::Optional<std::string> which is cleaner (does not requires explanation)? https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... File components/history/core/browser/web_history_service.h (right): https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... components/history/core/browser/web_history_service.h:138: bool is_tablet, You can also use ui::DeviceFormFactor here.
LGTM for components/browsing_data_ui/history_notice_utils.*
Thanks! I addressed the comments and rebased. PTAL! https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils.cc:122: false /* !history_service->HasOtherFormsOfBrowsingHistory() */)) { On 2016/05/24 08:32:29, sdefresne wrote: > Can you remove this /* comment */? Done. I originally left it there just to demonstrate that the old implementation has not changed, as msarda@ requested. And of course, I forgot about the negation. history_service->HasOtherFormsOfBrowsingHistory() == false Therefore: !history_service->HasOtherFormsOfBrowsingHistory() == true And so the conjunction can be simplified to its first term: !testing::g_override_other_forms_of_browsing_history_query && true !testing::g_override_other_forms_of_browsing_history_query https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... File components/browsing_data_ui/history_notice_utils.h (right): https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils.h:54: bool is_tablet, On 2016/05/24 08:32:29, sdefresne wrote: > Since you already indirectly depends on ui/base (through > components/history/core/browser), why not take the device form factor here > instead of a boolean? > > #include "ui/base/device_form_factor.h" > > void ShouldPopupDialogAboutOtherFormsOfBrowsingHistory( > const sync_driver::SyncService* sync_service, > history::WebHistoryService* history_service, > version_info::Channel channel, > ui::DeviceFormFactor form_factor, > base::Callback<void(bool)> callback); Actually in that case, WebHistoryService itself can call DeviceFormFactor, it doesn't have to be passed from outside. https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils.h:58: // TODO(msarda): Remove this when iOS calls the correct version. On 2016/05/24 08:32:29, sdefresne wrote: > Can you create a bug and use the format "TODO(crbug/xxxxxx)" instead? Done. crbug.com/614319 https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... File components/browsing_data_ui/history_notice_utils_unittest.cc (right): https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils_unittest.cc:44: void SetSyncActive(bool active) { On 2016/05/24 08:32:29, sdefresne wrote: > nit, style: this should be "set_sync_active" as it is inline. Done. For consistency, I renamed the underlying variables to exactly match the method names as well. https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils_unittest.cc:48: void SetActiveDataTypes(syncer::ModelTypeSet data_types) { On 2016/05/24 08:32:29, sdefresne wrote: > ditto Done. https://codereview.chromium.org/1983073002/diff/160001/components/browsing_da... components/browsing_data_ui/history_notice_utils_unittest.cc:52: void SetUsingSecondaryPassphrase(bool passphrase) { On 2016/05/24 08:32:29, sdefresne wrote: > ditto Done. https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... components/history/core/browser/web_history_service.cc:201: void SetPostData(const std::string& post_data, On 2016/05/24 08:32:29, sdefresne wrote: > Style guide discourage function/method overloading and encourages using explicit > names. Can this method be called SetPostDataAndType()? Done. https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... File components/history/core/browser/web_history_service.h (right): https://codereview.chromium.org/1983073002/diff/160001/components/history/cor... components/history/core/browser/web_history_service.h:138: bool is_tablet, On 2016/05/24 08:32:29, sdefresne wrote: > You can also use ui::DeviceFormFactor here. Acknowledged. As mentioned in the other comment, if history/core/browser already depends on ui/base anyway, we don't need to pass this at all and can just call it inside web_history_service.cc.
lgtm with couple of comments. https://codereview.chromium.org/1983073002/diff/220001/components/history/cor... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1983073002/diff/220001/components/history/cor... components/history/core/browser/web_history_service.cc:517: browser_sync::LocalDeviceInfoProviderImpl local_device_info_provider_( Instantiating LocalDeviceInfoProviderImpl should not be required to get user agent string. Unfortunately current implementation of GetSyncUserAgent is a member function. Could you leave TODO(pavely) here to refactor LocalDeviceInfoProviderImpl? I'll prepare a change and land it after you land yours. https://codereview.chromium.org/1983073002/diff/220001/components/history/cor... components/history/core/browser/web_history_service.cc:525: std::unique_ptr<sync_pb::HistoryStatusResponse> request_proto( HistoryStatusResponse => HistoryStatusRequest Also since you just prepare request, immediately serialize it to string and discard proto, you don't need to allocate request object from heap and wrap it into unique_ptr. Just replace it with sync_pb::HistoryStatusRequest request_proto(); https://codereview.chromium.org/1983073002/diff/220001/components/history/cor... components/history/core/browser/web_history_service.cc:612: std::unique_ptr<sync_pb::HistoryStatusResponse> history_status( Same comment as above: replace unique_ptr<HistoryStatusResponse> with plain HistoryStatusResponse on the stack.
Description was changed from ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from the Desktop and Android callsites (iOS is in a different codebase) BUG=595332 ========== to ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from the Desktop and Android callsites (iOS is in a different codebase) BUG=614652 ==========
I have addressed the comments, and also updated the bug number (I'll need a separate bug to request the merge). sdefresne@: Can you please take another pass today? Thanks! https://codereview.chromium.org/1983073002/diff/220001/components/history/cor... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/1983073002/diff/220001/components/history/cor... components/history/core/browser/web_history_service.cc:517: browser_sync::LocalDeviceInfoProviderImpl local_device_info_provider_( On 2016/05/24 22:05:04, pavely wrote: > Instantiating LocalDeviceInfoProviderImpl should not be required to get user > agent string. Unfortunately current implementation of GetSyncUserAgent is a > member function. Could you leave TODO(pavely) here to refactor > LocalDeviceInfoProviderImpl? I'll prepare a change and land it after you land > yours. Done. Yes, I thought the same - we have the standalone function MakeDesktopUserAgentForSync(), but for the more general version, we need to instantiate the class. I wanted to fix this myself, but it doesn't belong in this CL. At least the method is self-contained enough that I don't need to pass the version as well. https://codereview.chromium.org/1983073002/diff/220001/components/history/cor... components/history/core/browser/web_history_service.cc:525: std::unique_ptr<sync_pb::HistoryStatusResponse> request_proto( On 2016/05/24 22:05:04, pavely wrote: > HistoryStatusResponse => HistoryStatusRequest > Also since you just prepare request, immediately serialize it to string and > discard proto, you don't need to allocate request object from heap and wrap it > into unique_ptr. Just replace it with sync_pb::HistoryStatusRequest > request_proto(); Done. Oops! At least the serialization is still the same. https://codereview.chromium.org/1983073002/diff/220001/components/history/cor... components/history/core/browser/web_history_service.cc:612: std::unique_ptr<sync_pb::HistoryStatusResponse> history_status( On 2016/05/24 22:05:04, pavely wrote: > Same comment as above: replace unique_ptr<HistoryStatusResponse> with plain > HistoryStatusResponse on the stack. Done.
lgtm https://codereview.chromium.org/1983073002/diff/240001/components/browsing_da... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1983073002/diff/240001/components/browsing_da... components/browsing_data_ui/history_notice_utils.cc:29: call_count_(0) { nit: I think this should be "call_count_(0) {}" (simpler, just run "git cl format").
Description was changed from ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from the Desktop and Android callsites (iOS is in a different codebase) BUG=614652 ========== to ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from the Desktop and Android callsites (iOS is in a different codebase) BUG=614652 TBR=dbeam@chromium.org,dfalcantara@chromium.org ==========
msramek@chromium.org changed reviewers: + dbeam@chromium.org, dfalcantara@chromium.org
Thank you! I'm adding dbeam@ and dfalcantara@ as TBR, since I'm only updating callsites on Desktop and Android. Proceeding to land. https://codereview.chromium.org/1983073002/diff/240001/components/browsing_da... File components/browsing_data_ui/history_notice_utils.cc (right): https://codereview.chromium.org/1983073002/diff/240001/components/browsing_da... components/browsing_data_ui/history_notice_utils.cc:29: call_count_(0) { On 2016/05/25 13:48:09, sdefresne wrote: > nit: I think this should be "call_count_(0) {}" (simpler, just run "git cl > format"). Done. You're correct that "git cl format" fixes this to "{}". However, it also fixes the two chains of "||" below, squeezing several operands on one line, which I think much lowers the readability of that part, so I only left this fix in.
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbentzel@chromium.org, msarda@chromium.org, pavely@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1983073002/#ps260001 (title: "Nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983073002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983073002/260001
On 2016/05/25 at 14:55:43, msramek wrote: > Thank you! > > I'm adding dbeam@ and dfalcantara@ as TBR, since I'm only updating callsites on Desktop and Android. > > Proceeding to land. > > https://codereview.chromium.org/1983073002/diff/240001/components/browsing_da... > File components/browsing_data_ui/history_notice_utils.cc (right): > > https://codereview.chromium.org/1983073002/diff/240001/components/browsing_da... > components/browsing_data_ui/history_notice_utils.cc:29: call_count_(0) { > On 2016/05/25 13:48:09, sdefresne wrote: > > nit: I think this should be "call_count_(0) {}" (simpler, just run "git cl > > format"). > > Done. > > You're correct that "git cl format" fixes this to "{}". However, it also fixes the two chains of "||" below, squeezing several operands on one line, which I think much lowers the readability of that part, so I only left this fix in. Usually I recommend to just use the output of "git cl format". No point fighting with clang-format.
Message was sent while issue was closed.
Description was changed from ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from the Desktop and Android callsites (iOS is in a different codebase) BUG=614652 TBR=dbeam@chromium.org,dfalcantara@chromium.org ========== to ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from the Desktop and Android callsites (iOS is in a different codebase) BUG=614652 TBR=dbeam@chromium.org,dfalcantara@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from the Desktop and Android callsites (iOS is in a different codebase) BUG=614652 TBR=dbeam@chromium.org,dfalcantara@chromium.org ========== to ========== Query the existence other forms of browsing history. This CL implements the query for other forms of browsing history, which is necessary to show the notices in the Clear Browsing Data dialog on all platforms. Previously, the query was represented by a dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory(), but now we have an actual endpoint on the Sync server to answer the query. More detailed description of the change: 1. sync/protocol/ Adapt history_status.proto from cl/122116426 and add it to Chromium code. 2. components/history/ Replace the dummy method WebHistoryService::HasOtherFormsOfBrowsingHistory() with the actual request QueryOtherFormsOfBrowsingHistory(). We use most of the existing WebHistoryService insfrastructure, but ReadResponse is replaced by MessageLite::ParseFromString() since we're receiving a protobuf. Furthermore, the query URL is not static, as in other WebHistoryService calls, but has to be generated from the browser user agent and channel. 3. components/browsing_data_ui/ Update the logic to include calls to QueryOtherFormsOfBrowsingHistory(). This call and QueryWebAndAppActivity() call should run in parallel to save time, since they are both HTTP requests. 4. chrome/ Provide the user agent string and the channel info from the Desktop and Android callsites (iOS is in a different codebase) BUG=614652 TBR=dbeam@chromium.org,dfalcantara@chromium.org Committed: https://crrev.com/6d43c1934d9a3a090e16a1346a2ada29b7e1a86c Cr-Commit-Position: refs/heads/master@{#395894} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6d43c1934d9a3a090e16a1346a2ada29b7e1a86c Cr-Commit-Position: refs/heads/master@{#395894} |
