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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 242823002: Extract GetNodeByID() method from BookmarkModel. (Closed)

Created:
6 years, 8 months ago by tfarina
Modified:
6 years, 8 months ago
Reviewers:
sky
CC:
chromium-reviews, tim+watch_chromium.org, extensions-reviews_chromium.org, tfarina, dbeam+watch-ntp_chromium.org, haitaol+watch_chromium.org, browser-components-watch_chromium.org, estade+watch_chromium.org, chromium-apps-reviews_chromium.org, pedrosimonetti+watch_chromium.org, maniscalco+watch_chromium.org, sdefresne, droger, blundell
Visibility:
Public.

Description

Extract GetNodeByID() method from BookmarkModel. This moves GetNodeByID() method out of BookmarkModel and into a standalone function in bookmark_utils. The reason is that GetNodeByID() does not use any internal state of BookmarkModel, so there is no real reason it needs to be there. Scott suggested this and is fine with moving it into bookmark_utils. BUG=359565 TEST=unit_tests, sync_integration_tests, chrome. R=sky@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265201

Patch Set 1 : #

Total comments: 4

Patch Set 2 : put it in global namespace #

Patch Set 3 : Android built and fixed - invert #

Total comments: 3

Patch Set 4 : constantify node getters #

Patch Set 5 : revert a change in bookmarks_helper.cc that colides with a wstring and cq does not like that #

Unified diffs Side-by-side diffs Delta from patch set Stats (+124 lines, -114 lines) Patch
M chrome/browser/android/bookmarks/bookmarks_bridge.cc View 1 2 3 chunks +8 lines, -11 lines 0 comments Download
M chrome/browser/android/provider/chrome_browser_provider.cc View 1 2 9 chunks +9 lines, -8 lines 0 comments Download
M chrome/browser/bookmarks/bookmark_expanded_state_tracker.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/bookmarks/bookmark_model.h View 1 2 3 3 chunks +4 lines, -10 lines 0 comments Download
M chrome/browser/bookmarks/bookmark_model.cc View 2 chunks +0 lines, -18 lines 0 comments Download
M chrome/browser/bookmarks/bookmark_node_data.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/bookmarks/bookmark_utils.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/bookmarks/bookmark_utils.cc View 1 2 3 3 chunks +18 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.cc View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/bookmarks/bookmarks_api.cc View 1 2 3 5 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/history/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/history/android/bookmark_model_sql_handler.cc View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/sync/test/integration/bookmarks_helper.cc View 1 2 3 4 10 chunks +42 lines, -34 lines 0 comments Download
M chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_view.mm View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view.mm View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.mm View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc View 1 2 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/undo/bookmark_undo_service.cc View 1 2 3 5 chunks +7 lines, -7 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
tfarina
6 years, 8 months ago (2014-04-18 07:07:29 UTC) #1
sky
https://codereview.chromium.org/242823002/diff/20001/chrome/browser/bookmarks/bookmark_utils.cc File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/242823002/diff/20001/chrome/browser/bookmarks/bookmark_utils.cc#newcode389 chrome/browser/bookmarks/bookmark_utils.cc:389: const BookmarkNode* GetNodeByID(BookmarkModel* model, int64 id) { Make this ...
6 years, 8 months ago (2014-04-18 14:07:43 UTC) #2
tfarina
https://codereview.chromium.org/242823002/diff/20001/chrome/browser/bookmarks/bookmark_utils.cc File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/242823002/diff/20001/chrome/browser/bookmarks/bookmark_utils.cc#newcode389 chrome/browser/bookmarks/bookmark_utils.cc:389: const BookmarkNode* GetNodeByID(BookmarkModel* model, int64 id) { On 2014/04/18 ...
6 years, 8 months ago (2014-04-18 22:55:45 UTC) #3
sky
https://codereview.chromium.org/242823002/diff/60001/chrome/browser/bookmarks/bookmark_utils.cc File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/242823002/diff/60001/chrome/browser/bookmarks/bookmark_utils.cc#newcode393 chrome/browser/bookmarks/bookmark_utils.cc:393: return GetNodeByID(const_cast<BookmarkModel*>(model)->root_node(), id); Ick. Can you make all the ...
6 years, 8 months ago (2014-04-21 15:26:08 UTC) #4
sky
https://codereview.chromium.org/242823002/diff/60001/chrome/browser/bookmarks/bookmark_utils.cc File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/242823002/diff/60001/chrome/browser/bookmarks/bookmark_utils.cc#newcode393 chrome/browser/bookmarks/bookmark_utils.cc:393: return GetNodeByID(const_cast<BookmarkModel*>(model)->root_node(), id); On 2014/04/21 15:26:08, sky wrote: > ...
6 years, 8 months ago (2014-04-21 15:32:21 UTC) #5
tfarina
https://codereview.chromium.org/242823002/diff/60001/chrome/browser/bookmarks/bookmark_utils.cc File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/242823002/diff/60001/chrome/browser/bookmarks/bookmark_utils.cc#newcode393 chrome/browser/bookmarks/bookmark_utils.cc:393: return GetNodeByID(const_cast<BookmarkModel*>(model)->root_node(), id); On 2014/04/21 15:26:08, sky wrote: > ...
6 years, 8 months ago (2014-04-21 20:42:05 UTC) #6
sky
LGTM
6 years, 8 months ago (2014-04-21 23:06:25 UTC) #7
tfarina
The CQ bit was checked by tfarina@chromium.org
6 years, 8 months ago (2014-04-22 01:30:35 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tfarina@chromium.org/242823002/80001
6 years, 8 months ago (2014-04-22 01:31:00 UTC) #9
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-22 02:51:42 UTC) #10
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=62582
6 years, 8 months ago (2014-04-22 02:51:43 UTC) #11
tfarina
The CQ bit was checked by tfarina@chromium.org
6 years, 8 months ago (2014-04-22 03:20:15 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tfarina@chromium.org/242823002/100001
6 years, 8 months ago (2014-04-22 03:20:29 UTC) #13
commit-bot: I haz the power
6 years, 8 months ago (2014-04-22 07:48:12 UTC) #14
Message was sent while issue was closed. 
Change committed as 265201

Powered by Google App Engine
This is Rietveld 408576698