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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 882753002: Componentize WebHistoryService (Closed)

Created:
5 years, 11 months ago by sdefresne
Modified:
5 years, 10 months ago
CC:
browser-components-watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, David Black, dhollowa+watch_chromium.org, donnd+watch_chromium.org, dougw+watch_chromium.org, extensions-reviews_chromium.org, Jered, jfweitz+watch_chromium.org, kmadhusu+watch_chromium.org, melevin+watch_chromium.org, rlp+watch_chromium.org, samarth+watch_chromium.org, skanuj+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Componentize WebHistoryService Instead of using the factories to retrieve the dependended upon keyed service, pass them explicitly to WebHistoryService from the WebHistoryServiceFactory. Fix the dependency of WebHistoryService to add the service used and to remove the service unused. Move file into //components/history/core/browser, remove unnecessary #include and fix files that did not #include all the files they used. BUG=371835 Committed: https://crrev.com/12dabec1af75b80952821fee2f37a28637fa15b7 Cr-Commit-Position: refs/heads/master@{#313926}

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 3

Patch Set 3 : Fix rogerta comment #

Patch Set 4 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+124 lines, -732 lines) Patch
M chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc View 1 2 chunks +10 lines, -2 lines 0 comments Download
M chrome/browser/history/history_service.cc View 1 2 2 chunks +1 line, -1 line 0 comments Download
D chrome/browser/history/web_history_service.h View 1 chunk +0 lines, -165 lines 0 comments Download
D chrome/browser/history/web_history_service.cc View 1 chunk +0 lines, -479 lines 0 comments Download
M chrome/browser/history/web_history_service_factory.cc View 2 chunks +18 lines, -11 lines 0 comments Download
M chrome/browser/history/web_history_service_unittest.cc View 13 chunks +25 lines, -26 lines 0 comments Download
M chrome/browser/search/hotword_audio_history_handler.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/search/hotword_service_unittest.cc View 2 chunks +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/history_ui.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/history_ui.cc View 1 2 chunks +1 line, -1 line 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M components/history.gypi View 1 2 3 3 chunks +4 lines, -0 lines 0 comments Download
M components/history/DEPS View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M components/history/core/browser/BUILD.gn View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
A + components/history/core/browser/web_history_service.h View 1 2 5 chunks +21 lines, -7 lines 0 comments Download
A + components/history/core/browser/web_history_service.cc View 1 2 10 chunks +34 lines, -35 lines 0 comments Download

Messages

Total messages: 12 (3 generated)
sdefresne
droger: please take a look at the CL rogerta: please review new DEPS on //google/apis ...
5 years, 11 months ago (2015-01-27 18:04:40 UTC) #2
droger
lgtm https://codereview.chromium.org/882753002/diff/20001/chrome/browser/history/web_history_service_unittest.cc File chrome/browser/history/web_history_service_unittest.cc (right): https://codereview.chromium.org/882753002/diff/20001/chrome/browser/history/web_history_service_unittest.cc#newcode199 chrome/browser/history/web_history_service_unittest.cc:199: Profile* profile = static_cast<Profile*>(context); You can use Profile::FromBrowserContext() ...
5 years, 10 months ago (2015-01-28 11:03:16 UTC) #3
Roger Tawa OOO till Jul 10th
google_apis and signin deps lgtm, with one nit below. https://codereview.chromium.org/882753002/diff/20001/components/history/core/browser/web_history_service.h File components/history/core/browser/web_history_service.h (right): https://codereview.chromium.org/882753002/diff/20001/components/history/core/browser/web_history_service.h#newcode76 components/history/core/browser/web_history_service.h:76: ...
5 years, 10 months ago (2015-01-28 13:11:38 UTC) #4
rpetterson
*hotword* LGTM
5 years, 10 months ago (2015-01-28 19:48:28 UTC) #5
James Hawkins
lgtm
5 years, 10 months ago (2015-01-28 20:07:45 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/882753002/60001
5 years, 10 months ago (2015-01-30 15:56:38 UTC) #8
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 10 months ago (2015-01-30 16:43:09 UTC) #9
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/12dabec1af75b80952821fee2f37a28637fa15b7 Cr-Commit-Position: refs/heads/master@{#313926}
5 years, 10 months ago (2015-01-30 16:44:11 UTC) #10
Vitaly Buka (NO REVIEWS)
5 years, 10 months ago (2015-02-02 22:27:15 UTC) #11
Message was sent while issue was closed. 
A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/889553003/ by vitalybuka@chromium.org.

The reason for reverting is: BUG=454524.

Powered by Google App Engine
This is Rietveld 408576698