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

Issue 384039: Mac: Use native folder icons for the bookmark bar. (Closed)

Created:
11 years, 1 month ago by Nico
Modified:
9 years, 7 months ago
Reviewers:
Avi (use Gerrit), TVL
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Mac: Use native folder icons for the bookmark bar. Based on http://codereview.chromium.org/342103 , but load the icon on a background thread. Check http://build.chromium.org/buildbot/perf/mac-release-10.5/startup/report.html?history=100 – this was checked in in r31647 and reverted in r31652. No startup performance impact can be seen on the normal startup test, but the theme startup time still regresses. Hence, this can't be checked in. BUG=26457 TEST=Look at bookmark bar's "Other bookmarks". Should have blue folder.

Patch Set 1 #

Total comments: 3

Patch Set 2 : less flaky tests? #

Patch Set 3 : GRRRRRRRRAAAAAAAAAHHHHH #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -1 line) Patch
M chrome/browser/cocoa/bookmark_bar_controller.mm View 1 2 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 4 (0 generated)
TVL
drive by (saw the cl on the waterfall and peeked) http://codereview.chromium.org/384039/diff/1/3 File chrome/browser/cocoa/bookmark_bar_controller.mm (right): http://codereview.chromium.org/384039/diff/1/3#newcode83 ...
11 years, 1 month ago (2009-11-11 14:16:17 UTC) #1
TVL
drive by (saw the cl on the waterfall and peeked) http://codereview.chromium.org/384039/diff/1/3 File chrome/browser/cocoa/bookmark_bar_controller.mm (right): http://codereview.chromium.org/384039/diff/1/3#newcode83 ...
11 years, 1 month ago (2009-11-11 14:16:36 UTC) #2
Avi (use Gerrit)
http://codereview.chromium.org/384039/diff/1/3 File chrome/browser/cocoa/bookmark_bar_controller.mm (right): http://codereview.chromium.org/384039/diff/1/3#newcode83 Line 83: [[NSWorkspace sharedWorkspace] iconForFileType: On 2009/11/11 14:16:17, TVL wrote: ...
11 years, 1 month ago (2009-11-11 15:06:08 UTC) #3
Avi (use Gerrit)
11 years, 1 month ago (2009-11-11 15:08:31 UTC) #4
http://codereview.chromium.org/384039/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/384039/diff/1/3#newcode83
Line 83: [[NSWorkspace sharedWorkspace] iconForFileType:
On 2009/11/11 14:16:17, TVL wrote:
> nsworkspace isn't documented to be thread safe.  :(

You are correct that it's not documented, but it _is_ safe. From an
email from an Apple engineer:

"You're right to be cautious.  However, the method iconForFile: is
indeed thread safe on every version of Mac OS X, so it's fine to use in
a background thread.

"This would ordinarily be a sound plan, but NSWorkspace does not ever
expect to have multiple instances of itself created.  You're better off
just using +[NSWorkspace sharedWorkspace], which is thread safe as
well."

The proposed code is correct as written.

http://codereview.chromium.org/384039

Powered by Google App Engine
This is Rietveld 408576698