Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(60)

Issue 501073: Native Cocoa bookmark manager, part 1 (Closed)

Created:
9 years, 8 months ago by Jens Alfke
Modified:
8 years, 3 months ago
CC:
chromium-reviews_googlegroups.com, John Grabowski, pam+watch_chromium.org, ben+cc_chromium.org, Paweł Hajdan Jr.
Visibility:
Public.

Description

Native Cocoa bookmark manager, part 1 Most features are implemented, but Recents, Search and Undo are still missing. MainMenu.xib: Just added "Bookmark Manager" menu item at top of "Bookmarks" menu. BookmarkManager.xib: New file, containing Bookmark Manager panel, owned by BookmarkManagerController class. BUG=13149 TEST=New unit tests for each new class. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35546

Patch Set 1 #

Total comments: 121

Patch Set 2 : Lots of coding-style tweaks, and fixed some bugs noted in earlier comments. #

Total comments: 93

Patch Set 3 : Responding to latest comments. #

Patch Set 4 : Preliminary unit tests. Minor bug fixes. #

Total comments: 13

Patch Set 5 : Style fixes, and copy/paste unit tests #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+3572 lines, -176 lines) Patch
M chrome/app/generated_resources.grd View 1 1 chunk +3 lines, -0 lines 0 comments Download
A chrome/app/nibs/BookmarkManager.xib View 1 2 3 1 chunk +1517 lines, -0 lines 0 comments Download
M chrome/app/nibs/MainMenu.xib View 1 2 3 4 31 chunks +153 lines, -172 lines 0 comments Download
M chrome/browser/browser.cc View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
A chrome/browser/cocoa/bookmark_groups_controller.h View 1 2 3 1 chunk +52 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/bookmark_groups_controller.mm View 1 2 3 1 chunk +165 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/bookmark_groups_controller_unittest.mm View 4 1 chunk +79 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/bookmark_manager_controller.h View 2 3 4 1 chunk +63 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/bookmark_manager_controller.mm View 2 3 4 1 chunk +261 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/bookmark_manager_controller_unittest.mm View 4 1 chunk +73 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/bookmark_tree_controller.h View 1 2 3 4 1 chunk +68 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/bookmark_tree_controller.mm View 1 2 3 1 chunk +253 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm View 1 2 3 4 1 chunk +439 lines, -0 lines 1 comment Download
A chrome/browser/cocoa/bookmark_tree_controller_unittest.mm View 4 1 chunk +186 lines, -0 lines 1 comment Download
chrome/browser/cocoa/browser_window_cocoa.mm View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/cocoa/nsmenuitem_additions_unittest.mm View 1 chunk +5 lines, -1 line 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 chunks +12 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 4 1 chunk +3 lines, -0 lines 0 comments Download
A third_party/apple/ImageAndTextCell.h View 1 chunk +61 lines, -0 lines 0 comments Download
A third_party/apple/ImageAndTextCell.m View 1 chunk +171 lines, -0 lines 0 comments Download
A third_party/apple/README.chromium View 2 1 chunk +6 lines, -0 lines 1 comment Download

Messages

Total messages: 23 (0 generated)
Jens Alfke
Here's the native bookmark manager. It's running well aside from missing a few features; I ...
9 years, 8 months ago (2009-12-17 01:04:51 UTC) #1
viettrungluu
Looking -- thanks. Quick comments: 1. You should also add stuff in app_controller_mac.mm to enable ...
9 years, 8 months ago (2009-12-17 01:11:40 UTC) #2
viettrungluu
Jens, could you go through and correct all the stylistic nits? That way we can ...
9 years, 8 months ago (2009-12-17 01:40:37 UTC) #3
John Grabowski
Not a single unit test in this entire CL? I realize this is supposed to ...
9 years, 8 months ago (2009-12-17 04:31:14 UTC) #4
Nico
http://codereview.chromium.org/501073/diff/1/16 File third_party/apple/ImageAndTextCell.h (right): http://codereview.chromium.org/501073/diff/1/16#newcode1 third_party/apple/ImageAndTextCell.h:1: /* On 2009/12/17 04:31:14, John Grabowski wrote: > Neither ...
9 years, 8 months ago (2009-12-18 21:49:37 UTC) #5
Robert Sesek
On 2009/12/18 21:49:37, Nico wrote: > http://codereview.chromium.org/501073/diff/1/16 > File third_party/apple/ImageAndTextCell.h (right): > > http://codereview.chromium.org/501073/diff/1/16#newcode1 > ...
9 years, 8 months ago (2009-12-22 15:28:12 UTC) #6
Nico
I think he can't cause he needs an editable cell. Sent from my mobile computing ...
9 years, 8 months ago (2009-12-22 16:10:32 UTC) #7
Nico
I think he can't cause he needs an editable cell. Sent from my mobile computing ...
9 years, 8 months ago (2009-12-22 16:11:23 UTC) #8
Jens Alfke
Sorry for the delay responding, I've been out with a bad cold since Thursday. I'm ...
9 years, 8 months ago (2009-12-22 17:59:51 UTC) #9
Nico
http://codereview.chromium.org/501073/diff/1/16 File third_party/apple/ImageAndTextCell.h (right): http://codereview.chromium.org/501073/diff/1/16#newcode1 third_party/apple/ImageAndTextCell.h:1: /* On 2009/12/22 17:59:51, Jens Alfke wrote: > On ...
9 years, 8 months ago (2009-12-22 18:15:23 UTC) #10
John Grabowski
> > > I'm aware this needs unit tests -- I'd like to ask for ...
9 years, 8 months ago (2009-12-22 19:38:07 UTC) #11
viettrungluu
Still reading (esp. bookmark_tree_controller_pasteboard.mm and at the "big picture"), but I may as well send ...
9 years, 8 months ago (2009-12-22 20:02:42 UTC) #12
viettrungluu
My second batch of comments.... http://codereview.chromium.org/501073/diff/8001/44 File chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm (right): http://codereview.chromium.org/501073/diff/8001/44#newcode12 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:12: A general FYI: we ...
9 years, 8 months ago (2009-12-22 23:13:45 UTC) #13
viettrungluu
Now, to actually respond to some of what you said.... On 2009/12/22 17:59:51, Jens Alfke ...
9 years, 8 months ago (2009-12-22 23:38:32 UTC) #14
Jens Alfke
http://codereview.chromium.org/501073/diff/8001/38 File chrome/browser/cocoa/bookmark_groups_controller.h (right): http://codereview.chromium.org/501073/diff/8001/38#newcode24 chrome/browser/cocoa/bookmark_groups_controller.h:24: @property (copy) NSArray* groups; On 2009/12/22 20:02:43, viettrungluu wrote: ...
9 years, 7 months ago (2009-12-23 17:02:45 UTC) #15
viettrungluu
Some quick comments from SFO.... http://codereview.chromium.org/501073/diff/13001/14007 File chrome/browser/cocoa/bookmark_groups_controller_unittest.mm (right): http://codereview.chromium.org/501073/diff/13001/14007#newcode17 chrome/browser/cocoa/bookmark_groups_controller_unittest.mm:17: I don't know if ...
9 years, 7 months ago (2009-12-24 05:04:23 UTC) #16
viettrungluu
I wish I could hit "reply" *and* have it send all my other comments.... On ...
9 years, 7 months ago (2009-12-24 05:09:45 UTC) #17
Jens Alfke
I've fixed the noted issues, and I'm about to upload a patch that also adds ...
9 years, 7 months ago (2010-01-04 20:50:06 UTC) #18
viettrungluu
Thanks. LGTM with: - tiny nits (below) fixed, - description of xib file changes put ...
9 years, 7 months ago (2010-01-04 21:19:01 UTC) #19
Nico
Just so this doesn't get lost: > Tnanks. Added a README.chromium; the sample code already ...
9 years, 7 months ago (2010-01-05 00:20:41 UTC) #20
Jens Alfke
On 2010/01/05 00:20:41, Nico wrote: > It is necessary, see > http://wiki.corp.google.com/twiki/bin/view/Main/ThirdParty#Document_the_Code_... ImageAndTextCell is actually ...
9 years, 7 months ago (2010-01-05 00:31:51 UTC) #21
Nico
On 2010/01/05 00:31:51, Jens Alfke wrote: > On 2010/01/05 00:20:41, Nico wrote: > > It ...
9 years, 7 months ago (2010-01-05 00:34:09 UTC) #22
John Grabowski
9 years, 7 months ago (2010-01-05 00:48:51 UTC) #23
LGTM
Glad to see this come together.

http://codereview.chromium.org/501073/diff/16008/16029
File third_party/apple/README.chromium (right):

http://codereview.chromium.org/501073/diff/16008/16029#newcode6
third_party/apple/README.chromium:6: * ImageAndTextCell.h: Changed 'image'
property to 'retain' mode to fix a crash.
ha.  Be sure to upstream this.  :-)

Powered by Google App Engine
This is Rietveld 408576698