Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2450453002:
Refactor BrowsingHistoryHandler, create BrowsingHistoryService (Closed)
DescriptionRefactor BrowsingHistoryHandler, create BrowsingHistoryService
Creates a new BrowsingHistoryService that contains the logic that
used to be in BrowsingHistoryHandler for querying and observing
HistoryService, WebHistoryService, and SyncService.
BrowsingHistoryHandler now interacts with BrowsingHistoryService and
contains WebUI specific logic.
BUG=654071
Committed: https://crrev.com/55397f22c5295f4e15530c9e156b4b78830e8963
Cr-Commit-Position: refs/heads/master@{#433200}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Rebase #Patch Set 3 : Fix rebase #Patch Set 4 : Changes from review & fix unit tests #Patch Set 5 : Add back #include<utility> #
Total comments: 2
Patch Set 6 : Rename HistoryUiService to BrowsingHistoryService #
Total comments: 4
Patch Set 7 : Make BrowsingHistoryServiceHandler its own class #Patch Set 8 : Rebase #Patch Set 9 : Rebase #Patch Set 10 : Add missing import #Messages
Total messages: 70 (46 generated)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
twellington@chromium.org changed reviewers: + tsergeant@chromium.org
ptal For the most part, code moved without modification from browsing_history_handler.cc to history_ui_service.h. I tried to highlight where there are differences. https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/history_u... File chrome/browser/ui/history_ui_service.cc (right): https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/history_u... chrome/browser/ui/history_ui_service.cc:119: history::HistoryService* local_history = HistoryServiceFactory::GetForProfile( This logic used to be in BrowsingHistoryHandler::RegisterMessages() https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/history_u... chrome/browser/ui/history_ui_service.cc:223: std::vector<std::unique_ptr<HistoryUiService::HistoryEntry>>* items) { This method changed to work with HistoryEntry items rather than ListValue args, which are parsed and turned into HistoryEntry items in BrowsingHistoryHandler::HandleRemoveVisits() https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/history_u... File chrome/browser/ui/history_ui_service.h (right): https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/history_u... chrome/browser/ui/history_ui_service.h:103: struct QueryResultsInfo { This new struct is used to pass information about the completed history query back to BrowsingHistoryHandler; that class turns the QueryResultsInfo into base::ListValue then sends that in a JS message. https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/history_u... chrome/browser/ui/history_ui_service.h:124: class HistoryUiServiceHandler { Places where BrowsingHistoryHandler called JS methods are replaced by calls to this handler interface. https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... chrome/browser/ui/webui/browsing_history_handler.cc:477: Profile* profile = Profile::FromWebUI(web_ui()); This logic used to be in BrowsingHistoryHandler::ReturnResultsToFrontEnd()
tsergeant@chromium.org changed reviewers: + dbeam@chromium.org
I'm OOO for the next week (I forgot to update my status, sorry), so passing over to Dan to review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
dang, I didn't see this for too long. my bad. i'll get to it this weekend or on monday. sorry!
On 2016/10/29 01:11:19, Dan Beam wrote: > dang, I didn't see this for too long. my bad. i'll get to it this weekend or > on monday. > > sorry! No rush, but thank you!
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
this generally seems like a useful and fine separation to me tsergeant@: thoughts? https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... chrome/browser/ui/webui/browsing_history_handler.cc:268: new HistoryUiService(Profile::FromWebUI(web_ui()), this)); nit: base::MakeUnique
This is cool, lgtm https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... File chrome/browser/ui/webui/browsing_history_handler_unittest.cc (right): https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:10: #include <utility> I might be wrong, but I don't see anything which needs this include?
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... File chrome/browser/ui/webui/browsing_history_handler_unittest.cc (right): https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:134: return std::move(service); ^ this uses <utility>
https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/2450453002/diff/1/chrome/browser/ui/webui/bro... chrome/browser/ui/webui/browsing_history_handler.cc:268: new HistoryUiService(Profile::FromWebUI(web_ui()), this)); On 2016/11/03 03:56:34, Dan Beam wrote: > nit: base::MakeUnique Done.
any way we could ... not dramatically change this class until after MD history (desktop) is on stable? we've had to do a ... few merges :( and this would really hamper that ability this is a pickle
On 2016/11/03 19:24:50, Dan Beam wrote: > any way we could ... not dramatically change this class until after MD history > (desktop) is on stable? we've had to do a ... few merges :( and this would > really hamper that ability > > this is a pickle I can wait until after branch to land this. Do you mind doing another pass and if it looks good to you I'll send it to a chrome/browser/ui/ OWNER for review? It'd be nice to have all of my ducks in a row so this can land shortly after branch.
this seems fine to me (lgtm) but it'll be hard to get a useful review with the amount of code you're changing all at once. i assume it's pretty much just slicing and dicing with no functional changes?
twellington@chromium.org changed reviewers: + sky@chromium.org
+sky@ for changes in chrome/browser/ui/
On 2016/11/05 00:53:41, Dan Beam wrote: > this seems fine to me (lgtm) but it'll be hard to get a useful review with the > amount of code you're changing all at once. i assume it's pretty much just > slicing and dicing with no functional changes? Correct, there are no functional changes. This CL is almost exclusively code being split apart and moved around. The only real difference is the introduction of the QueryResultInfo and HistoryUiServiceHandler which are used to pass results from, HistoryUiService to BrowsingHistoryHandler. I added two diff files to the bug to make it easier to see how history_ui_service.cc changed from browsing_history_handler.cc and how history_ui_service_unittest.cc changed from browsing_history_handler_unittest.cc. There's also a link to the design doc in the bug, and some comments on PatchSet 1 that highlight places where things changed and places where code moved to functions with different names.
https://codereview.chromium.org/2450453002/diff/80001/chrome/browser/ui/histo... File chrome/browser/ui/history_ui_service.h (right): https://codereview.chromium.org/2450453002/diff/80001/chrome/browser/ui/histo... chrome/browser/ui/history_ui_service.h:47: class HistoryUiService : public history::HistoryServiceObserver, Is there a reason not to keep this code in the webui directory where it used? I find this name mildly confusing given there is already a HistoryService. Can this be named to better indicate it's used for webui?
https://codereview.chromium.org/2450453002/diff/80001/chrome/browser/ui/histo... File chrome/browser/ui/history_ui_service.h (right): https://codereview.chromium.org/2450453002/diff/80001/chrome/browser/ui/histo... chrome/browser/ui/history_ui_service.h:47: class HistoryUiService : public history::HistoryServiceObserver, On 2016/11/07 20:42:52, sky wrote: > Is there a reason not to keep this code in the webui directory where it used? I'm working on changing Android to have its own history view that uses Android Views rather than the old webui. This refactoring is the first step. Currently this class is only used by the webui, but in the near future it will also be used by the Android history ui, which won't live in the webui directory. > I find this name mildly confusing given there is already a HistoryService. Can > this be named to better indicate it's used for webui? HistoryUiService seemed like a good name since it's going to be used by the existing webui and the new Android ui, but I'm open to suggestions. Maybe BrowsingHistoryService (or BrowsingHistoryUiService) to help denote that it aggregates various kinds of history?
I like BrowsingHistoryService. On Mon, Nov 7, 2016 at 1:02 PM, <twellington@chromium.org> wrote: > > https://codereview.chromium.org/2450453002/diff/80001/chrome/browser/ui/histo... > File chrome/browser/ui/history_ui_service.h (right): > > https://codereview.chromium.org/2450453002/diff/80001/chrome/browser/ui/histo... > chrome/browser/ui/history_ui_service.h:47: class HistoryUiService : > public history::HistoryServiceObserver, > On 2016/11/07 20:42:52, sky wrote: >> Is there a reason not to keep this code in the webui directory where > it used? > > I'm working on changing Android to have its own history view that uses > Android Views rather than the old webui. This refactoring is the first > step. Currently this class is only used by the webui, but in the near > future it will also be used by the Android history ui, which won't live > in the webui directory. > >> I find this name mildly confusing given there is already a > HistoryService. Can >> this be named to better indicate it's used for webui? > > HistoryUiService seemed like a good name since it's going to be used by > the existing webui and the new Android ui, but I'm open to suggestions. > Maybe BrowsingHistoryService (or BrowsingHistoryUiService) to help > denote that it aggregates various kinds of history? > > https://codereview.chromium.org/2450453002/ -- 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.
On 2016/11/07 21:55:23, sky wrote: > I like BrowsingHistoryService. > > On Mon, Nov 7, 2016 at 1:02 PM, <mailto:twellington@chromium.org> wrote: > > > > > https://codereview.chromium.org/2450453002/diff/80001/chrome/browser/ui/histo... > > File chrome/browser/ui/history_ui_service.h (right): > > > > > https://codereview.chromium.org/2450453002/diff/80001/chrome/browser/ui/histo... > > chrome/browser/ui/history_ui_service.h:47: class HistoryUiService : > > public history::HistoryServiceObserver, > > On 2016/11/07 20:42:52, sky wrote: > >> Is there a reason not to keep this code in the webui directory where > > it used? > > > > I'm working on changing Android to have its own history view that uses > > Android Views rather than the old webui. This refactoring is the first > > step. Currently this class is only used by the webui, but in the near > > future it will also be used by the Android history ui, which won't live > > in the webui directory. > > > >> I find this name mildly confusing given there is already a > > HistoryService. Can > >> this be named to better indicate it's used for webui? > > > > HistoryUiService seemed like a good name since it's going to be used by > > the existing webui and the new Android ui, but I'm open to suggestions. > > Maybe BrowsingHistoryService (or BrowsingHistoryUiService) to help > > denote that it aggregates various kinds of history? > > > > https://codereview.chromium.org/2450453002/ > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > Also, I think BrowsingHistoryService should live in c/b/history.
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactor BrowsingHistoryHandler, create HistoryUiService Creates a new HistoryUiService that contains the logic that used to be in BrowsingHistoryHandler for querying and observing HistoryService, WebHistoryService, and SyncService. BrowsingHistoryHandler now interacts with HistoryUiService and contains WebUI specific logic. BUG=654071 ========== to ========== Refactor BrowsingHistoryHandler, create BrowsingHistoryService Creates a new BrowsingHistoryService that contains the logic that used to be in BrowsingHistoryHandler for querying and observing HistoryService, WebHistoryService, and SyncService. BrowsingHistoryHandler now interacts with BrowsingHistoryService and contains WebUI specific logic. BUG=654071 ==========
On 2016/11/07 22:29:00, sky wrote: > Also, I think BrowsingHistoryService should live in c/b/history. Done.
LGTM with the following https://codereview.chromium.org/2450453002/diff/100001/chrome/browser/history... File chrome/browser/history/browsing_history_service.h (right): https://codereview.chromium.org/2450453002/diff/100001/chrome/browser/history... chrome/browser/history/browsing_history_service.h:124: class BrowsingHistoryServiceHandler { Move this into it's own header (as style guide suggests). https://codereview.chromium.org/2450453002/diff/100001/chrome/browser/history... chrome/browser/history/browsing_history_service.h:145: }; make constructor protected.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Thank you all for the reviews. I'm going to wait to rebase and land this until after next week's branch. I don't anticipate any changes from the rebase but will ping if there is anything that isn't trivial. https://codereview.chromium.org/2450453002/diff/100001/chrome/browser/history... File chrome/browser/history/browsing_history_service.h (right): https://codereview.chromium.org/2450453002/diff/100001/chrome/browser/history... chrome/browser/history/browsing_history_service.h:124: class BrowsingHistoryServiceHandler { On 2016/11/07 23:49:19, sky wrote: > Move this into it's own header (as style guide suggests). Done. https://codereview.chromium.org/2450453002/diff/100001/chrome/browser/history... chrome/browser/history/browsing_history_service.h:145: }; On 2016/11/07 23:49:18, sky wrote: > make constructor protected. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm going to land this today now that we've branched.
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, dbeam@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2450453002/#ps180001 (title: "Add missing import")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactor BrowsingHistoryHandler, create BrowsingHistoryService Creates a new BrowsingHistoryService that contains the logic that used to be in BrowsingHistoryHandler for querying and observing HistoryService, WebHistoryService, and SyncService. BrowsingHistoryHandler now interacts with BrowsingHistoryService and contains WebUI specific logic. BUG=654071 ========== to ========== Refactor BrowsingHistoryHandler, create BrowsingHistoryService Creates a new BrowsingHistoryService that contains the logic that used to be in BrowsingHistoryHandler for querying and observing HistoryService, WebHistoryService, and SyncService. BrowsingHistoryHandler now interacts with BrowsingHistoryService and contains WebUI specific logic. BUG=654071 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Refactor BrowsingHistoryHandler, create BrowsingHistoryService Creates a new BrowsingHistoryService that contains the logic that used to be in BrowsingHistoryHandler for querying and observing HistoryService, WebHistoryService, and SyncService. BrowsingHistoryHandler now interacts with BrowsingHistoryService and contains WebUI specific logic. BUG=654071 ========== to ========== Refactor BrowsingHistoryHandler, create BrowsingHistoryService Creates a new BrowsingHistoryService that contains the logic that used to be in BrowsingHistoryHandler for querying and observing HistoryService, WebHistoryService, and SyncService. BrowsingHistoryHandler now interacts with BrowsingHistoryService and contains WebUI specific logic. BUG=654071 Committed: https://crrev.com/55397f22c5295f4e15530c9e156b4b78830e8963 Cr-Commit-Position: refs/heads/master@{#433200} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/55397f22c5295f4e15530c9e156b4b78830e8963 Cr-Commit-Position: refs/heads/master@{#433200} |
