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

Issue 1205603002: Remove dependency of HistoryServiceFactory on ChromeBookmarkClientFactory (Closed)

Created:
5 years, 6 months ago by sdefresne
Modified:
5 years, 6 months ago
Reviewers:
sky
CC:
browser-components-watch_chromium.org, chromium-reviews, noyau+watch_chromium.org, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@split_history_client
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove dependency of HistoryServiceFactory on ChromeBookmarkClientFactory Add a method Init() to HistoryClient called by HistoryService so that the client can connect HistoryService and BookmarkModel events. HistoryClient implements BaseBookmarkModelObserver interface and forward the URLs removed from bookmarks event to HistoryService. Symmetrically it register a callback that will send to BookmarkModel the events about changes to favicons. Remove the hackish access to ChromeBookmarkClientFactory from the HistoryServiceFactory. BUG=359565 Committed: https://crrev.com/64823f69cd200d77e7ffc8ac949f6df7168f9183 Cr-Commit-Position: refs/heads/master@{#336346}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Remove change to BookmarkModelObserver and use BaseBookmarkModelObserver #

Total comments: 6

Patch Set 3 : Remove superfluous #include & rename Init to OnHistoryServiceCreated #

Patch Set 4 : Fix unit_tests on android (only register as BookmarkModelObserver if HistoryService is initialized) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -70 lines) Patch
M chrome/browser/bookmarks/chrome_bookmark_client.h View 1 4 chunks +2 lines, -24 lines 0 comments Download
M chrome/browser/bookmarks/chrome_bookmark_client.cc View 1 5 chunks +10 lines, -32 lines 0 comments Download
M chrome/browser/history/chrome_history_client.h View 1 2 3 1 chunk +33 lines, -1 line 0 comments Download
M chrome/browser/history/chrome_history_client.cc View 1 2 3 4 chunks +52 lines, -2 lines 0 comments Download
M chrome/browser/history/history_service_factory.cc View 5 chunks +8 lines, -11 lines 0 comments Download
M components/history/core/browser/history_client.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M components/history/core/browser/history_service.cc View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M components/history/core/test/history_client_fake_bookmarks.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M components/history/core/test/history_client_fake_bookmarks.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 15 (5 generated)
sdefresne
Please take a look https://codereview.chromium.org/1198373002
5 years, 6 months ago (2015-06-23 13:41:36 UTC) #2
sdefresne
On 2015/06/23 at 13:41:36, sdefresne wrote: > Please take a look https://codereview.chromium.org/1198373002 I meant: Please ...
5 years, 6 months ago (2015-06-23 13:42:21 UTC) #3
sky
https://codereview.chromium.org/1205603002/diff/1/components/bookmarks/browser/bookmark_model_observer.h File components/bookmarks/browser/bookmark_model_observer.h (left): https://codereview.chromium.org/1205603002/diff/1/components/bookmarks/browser/bookmark_model_observer.h#oldcode126 components/bookmarks/browser/bookmark_model_observer.h:126: const std::set<GURL>& removed_urls) = 0; The idea with these ...
5 years, 6 months ago (2015-06-23 16:43:42 UTC) #4
sdefresne
Could you take another look? I've also updated the description accordingly. https://codereview.chromium.org/1205603002/diff/1/components/bookmarks/browser/bookmark_model_observer.h File components/bookmarks/browser/bookmark_model_observer.h (left): ...
5 years, 6 months ago (2015-06-23 17:10:21 UTC) #5
sky
https://codereview.chromium.org/1205603002/diff/20001/components/history/core/browser/history_client.h File components/history/core/browser/history_client.h (right): https://codereview.chromium.org/1205603002/diff/20001/components/history/core/browser/history_client.h#newcode8 components/history/core/browser/history_client.h:8: #include <set> Seems like you shouldn't need either of ...
5 years, 6 months ago (2015-06-23 17:38:51 UTC) #6
sdefresne
Please take another look. https://codereview.chromium.org/1205603002/diff/20001/components/history/core/browser/history_client.h File components/history/core/browser/history_client.h (right): https://codereview.chromium.org/1205603002/diff/20001/components/history/core/browser/history_client.h#newcode8 components/history/core/browser/history_client.h:8: #include <set> On 2015/06/23 at ...
5 years, 6 months ago (2015-06-23 21:46:33 UTC) #7
sky
LGTM - please ping you're other dependant cls and I'll review them.
5 years, 6 months ago (2015-06-23 21:50:54 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1205603002/60001
5 years, 6 months ago (2015-06-26 08:27:34 UTC) #13
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 6 months ago (2015-06-26 08:57:01 UTC) #14
commit-bot: I haz the power
5 years, 6 months ago (2015-06-26 08:58:04 UTC) #15
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/64823f69cd200d77e7ffc8ac949f6df7168f9183
Cr-Commit-Position: refs/heads/master@{#336346}

Powered by Google App Engine
This is Rietveld 408576698