Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(391)

Issue 7566036: Implement SyncableServices in TemplateURLService. Add related unittests. (Closed)

Created:
9 years, 4 months ago by SteveT
Modified:
9 years, 4 months ago
CC:
chromium-reviews, Paweł Hajdan Jr., tim (not reviewing), Raghu Simha, ncarter (slow), idana
Visibility:
Public.

Description

Recommitting what was r96969. Fixed the memory leak issues. Implement SyncableServices in TemplateURLService. Add related unittests. TEST=Ensure all TemplateURLService related unit tests pass. Ensure the HeapBots are happy! BUG=15548 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97187

Patch Set 1 : Initial upload #

Total comments: 19

Patch Set 2 : '' #

Total comments: 4

Patch Set 3 : Shifted helpers to private after Peter's initial comments #

Patch Set 4 : Addressed Peter's final comment nits. #

Total comments: 45

Patch Set 5 : Addressed zea's TemplateURLService issues. #

Patch Set 6 : Addressed zea's unittest issues. #

Total comments: 8

Patch Set 7 : Addressed zea's additional issues. #

Patch Set 8 : Recommitting patch. Repaired memory leaks and adjusted unittests to reflect changes. #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+1542 lines, -13 lines) Patch
M chrome/browser/search_engines/template_url.cc View 1 2 3 4 5 6 7 3 chunks +4 lines, -5 lines 0 comments Download
M chrome/browser/search_engines/template_url_service.h View 1 2 3 4 5 6 7 9 chunks +121 lines, -1 line 4 comments Download
M chrome/browser/search_engines/template_url_service.cc View 1 2 3 4 5 6 7 16 chunks +412 lines, -7 lines 6 comments Download
A chrome/browser/search_engines/template_url_service_sync_unittest.cc View 1 2 3 4 5 6 7 1 chunk +982 lines, -0 lines 0 comments Download
M chrome/browser/search_engines/template_url_unittest.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/sync/api/sync_change.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/sync/api/sync_change.cc View 1 2 3 4 5 6 7 1 chunk +17 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
SteveT
Hey guys, Here's my initial patch for syncing TemplateURLService (aka Syncing Custom Search Engines). Please ...
9 years, 4 months ago (2011-08-09 15:26:35 UTC) #1
SteveT
Hm, just noticed that the helpers in TemplateURLService can probably be all made private and ...
9 years, 4 months ago (2011-08-09 20:20:38 UTC) #2
Peter Kasting
Make sure the following cases both work correctly: * Any URL that is managed by ...
9 years, 4 months ago (2011-08-09 21:31:04 UTC) #3
SteveT
I believe I've addressed everything inline. I will upload a separate patch where I make ...
9 years, 4 months ago (2011-08-10 15:14:48 UTC) #4
SteveT
Oops, forgot the comments. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engines/template_url_service.cc File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engines/template_url_service.cc#newcode45 chrome/browser/search_engines/template_url_service.cc:45: using syncable::SEARCH_ENGINES; Agreed. Cleaned up ...
9 years, 4 months ago (2011-08-10 15:15:12 UTC) #5
Peter Kasting
On 2011/08/10 15:14:48, SteveT wrote: > On 2011/08/09 21:31:04, Peter Kasting wrote: > > Make ...
9 years, 4 months ago (2011-08-10 17:39:11 UTC) #6
Peter Kasting
Other than the broad vague comments I've just made, the changes LGTM http://codereview.chromium.org/7566036/diff/21001/chrome/browser/search_engines/template_url_service.h File chrome/browser/search_engines/template_url_service.h ...
9 years, 4 months ago (2011-08-10 17:42:49 UTC) #7
SteveT
On 2011/08/10 17:39:11, Peter Kasting wrote: > On 2011/08/10 15:14:48, SteveT wrote: > > On ...
9 years, 4 months ago (2011-08-10 18:26:46 UTC) #8
SteveT
Fixed last couple nits and privatized helpers. Waiting on Nick's review now. http://codereview.chromium.org/7566036/diff/21001/chrome/browser/search_engines/template_url_service.h File chrome/browser/search_engines/template_url_service.h ...
9 years, 4 months ago (2011-08-10 18:27:17 UTC) #9
Nicolas Zea
Initial comments, still need to go through unit tests. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engines/template_url_service.cc File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engines/template_url_service.cc#newcode634 chrome/browser/search_engines/template_url_service.cc:634: ...
9 years, 4 months ago (2011-08-15 17:01:01 UTC) #10
Nicolas Zea
http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engines/template_url_service_sync_unittest.cc File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engines/template_url_service_sync_unittest.cc#newcode17 chrome/browser/search_engines/template_url_service_sync_unittest.cc:17: static std::string GetGUID(const SyncData& sync_data) { I think anonymous ...
9 years, 4 months ago (2011-08-15 19:33:32 UTC) #11
SteveT
Here's a patch addressing your first set of comments. Almost everything is dealt with, save ...
9 years, 4 months ago (2011-08-15 21:13:58 UTC) #12
SteveT
I think that's everything for this file... over to you. PTAL. Thanks! http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engines/template_url_service_sync_unittest.cc File chrome/browser/search_engines/template_url_service_sync_unittest.cc ...
9 years, 4 months ago (2011-08-15 22:41:45 UTC) #13
Nicolas Zea
http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engines/template_url_service.h File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engines/template_url_service.h#newcode20 chrome/browser/search_engines/template_url_service.h:20: #include "chrome/browser/sync/api/sync_change.h" On 2011/08/15 21:13:58, SteveT wrote: > So ...
9 years, 4 months ago (2011-08-15 23:40:00 UTC) #14
SteveT
Back to you, with perhaps one more thing to look at. PTAL. http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engines/template_url_service_sync_unittest.cc File chrome/browser/search_engines/template_url_service_sync_unittest.cc ...
9 years, 4 months ago (2011-08-16 00:08:59 UTC) #15
Nicolas Zea
LGTM
9 years, 4 months ago (2011-08-16 00:21:14 UTC) #16
SteveT
Hey Nick, Could you take a quick look? The original commit was reverted due to ...
9 years, 4 months ago (2011-08-17 17:06:29 UTC) #17
Nicolas Zea
LGTM
9 years, 4 months ago (2011-08-17 17:40:52 UTC) #18
sky
9 years, 4 months ago (2011-08-17 23:22:29 UTC) #19
I took at look and have a handful of comments.

  -Scott

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:500:
GetSearchProvidersUsingKeywordResult(*result,
This may end up modifying the set of TemplateURLs (at least the prepopulate
ones). You'll need to deal with that in some way. In fact it's worth a test case
to make sure migration gets handled correctly.

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:691: return error;
Are you sure we want to return here and not continue on? Say you have two
machines, A and B. A is connected, B is offline. If you delete the same TURL on
both machines, then B becomes connected, might you hit this branch? Maybe
MergeData happens first so this isn't an issue?

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:701: SyncError
TemplateURLService::MergeDataAndStartSyncing(
Is it guaranteed these methods are only invoked after loading has completed?
Could you add a DCHECK to that effect?

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:739: sync_turl->url() ?
sync_turl->url()->url() : std::string());
I believe you'll also want safe_for_autoreplace copied over too, otherwise any
synced TURL will end up with safe_for_autoreplace false.

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:874:
se_specifics->set_id(turl.id());
Is there any protection to make sure we don't end up with duplicate ids? In fact
do we even need to sync the id?

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:1679: string16
new_keyword;
nit: pull into if as you don't use it outside the if.

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:134: // The caller should
not try to delete the returned pointer; the data store
nit: Returns the TemplateURL with the specified guid, or NULL if not found.

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:475: // Given a TemplateURL
from Sync, resolves any keyword conflicts by checking
nit: add (cloud) after Sync.

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:484: SyncChangeList&
change_list);
Because this may modify change_list, it should be a pointer.

http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:500: SyncChangeList&
change_list);
change_list should be a pointer here too.

Powered by Google App Engine
This is Rietveld 408576698