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

Issue 1629010: [Mac] PDF icons for omnibox. (Closed)

Created:
10 years, 8 months ago by Scott Hess - ex-Googler
Modified:
9 years, 6 months ago
Reviewers:
TVL, viettrungluu
CC:
chromium-reviews, Peter Kasting
Visibility:
Public.

Description

[Mac] PDF icons for omnibox. Replaces the various resources used for omnibox icons with PDF images. The use of copies is because the field is flipped while the popup is unflipped (and the icons get flipped to match). BUG=37865 TEST=Icons don't look suck, look awesome when scale factor is changed under Quartz Debug. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44152

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fix flipping in popup. #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+616 lines, -18 lines) Patch
A chrome/app/theme/omnibox_history.pdf View Binary file 0 comments Download
A chrome/app/theme/omnibox_http.pdf View Binary file 0 comments Download
A chrome/app/theme/omnibox_https_green.pdf View Binary file 0 comments Download
A chrome/app/theme/omnibox_https_invalid.pdf View Binary file 0 comments Download
chrome/app/theme/omnibox_https_valid.pdf View 1 chunk +316 lines, -0 lines 0 comments Download
A chrome/app/theme/omnibox_https_warning.pdf View Binary file 0 comments Download
A chrome/app/theme/omnibox_more.pdf View Binary file 0 comments Download
A chrome/app/theme/omnibox_search.pdf View Binary file 0 comments Download
chrome/app/theme/omnibox_star.pdf View 1 chunk +231 lines, -2 lines 0 comments Download
A chrome/app/theme/omnibox_star_lit.pdf View Binary file 0 comments Download
M chrome/browser/autocomplete/autocomplete_edit_view_mac.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/autocomplete/autocomplete_edit_view_mac.mm View 1 3 chunks +41 lines, -0 lines 6 comments Download
chrome/browser/autocomplete/autocomplete_popup_view_mac.mm View 1 3 chunks +8 lines, -11 lines 0 comments Download
chrome/browser/cocoa/autocomplete_text_field_cell.mm View 1 1 chunk +2 lines, -2 lines 0 comments Download
chrome/browser/cocoa/location_bar_view_mac.mm View 1 2 chunks +4 lines, -3 lines 0 comments Download
chrome/chrome_dll.gypi View 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Scott Hess - ex-Googler
Trung, sending this to you because it is SO AWESOME.
10 years, 8 months ago (2010-04-08 21:53:33 UTC) #1
TVL
drive by http://codereview.chromium.org/1629010/diff/1/21001 File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right): http://codereview.chromium.org/1629010/diff/1/21001#newcode152 chrome/browser/autocomplete/autocomplete_edit_view_mac.mm:152: return [[image copy] autorelease]; why copy it? ...
10 years, 8 months ago (2010-04-09 01:22:42 UTC) #2
viettrungluu
+1 on TVL's questions -- I don't think you should be copying. Otherwise, looks fine.
10 years, 8 months ago (2010-04-09 01:50:12 UTC) #3
Scott Hess - ex-Googler
On 2010/04/09 01:22:42, TVL wrote: > drive by > > http://codereview.chromium.org/1629010/diff/1/21001 > File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right): ...
10 years, 8 months ago (2010-04-09 02:45:49 UTC) #4
Scott Hess - ex-Googler
On 2010/04/09 02:45:49, shess wrote: > Since neither of you complained about the general approach, ...
10 years, 8 months ago (2010-04-09 20:25:10 UTC) #5
Scott Hess - ex-Googler
On 2010/04/09 20:25:10, shess wrote: > On 2010/04/09 02:45:49, shess wrote: > > Since neither ...
10 years, 8 months ago (2010-04-09 23:05:16 UTC) #6
viettrungluu
After-the-fact LGTM, I guess (with some post mortem nits). http://codereview.chromium.org/1629010/diff/31001/16002 File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right): http://codereview.chromium.org/1629010/diff/31001/16002#newcode124 chrome/browser/autocomplete/autocomplete_edit_view_mac.mm:124: ...
10 years, 8 months ago (2010-04-09 23:38:55 UTC) #7
Scott Hess - ex-Googler
10 years, 8 months ago (2010-04-09 23:48:59 UTC) #8
I figured you were under water and decided to just ship that puppy off rather
than bug you to double-check a trivial change that passed trybots on an item
you'd already reviewed.

Since you came up for air, though... I'll post you a new CL :-).

http://codereview.chromium.org/1629010/diff/31001/16002
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/1629010/diff/31001/16002#newcode124
chrome/browser/autocomplete/autocomplete_edit_view_mac.mm:124: NSImage*
AutocompleteEditViewMac::ImageForResource(int resource_id) {
On 2010/04/09 23:38:55, viettrungluu wrote:
> A "// static" comment would have been nice.

Done.

http://codereview.chromium.org/1629010/diff/31001/16002#newcode130
chrome/browser/autocomplete/autocomplete_edit_view_mac.mm:130: case
IDR_OMNIBOX_STAR : image_name = @"omnibox_star.pdf"; break;
On 2010/04/09 23:38:55, viettrungluu wrote:
> No space before ':'.....

Done.

http://codereview.chromium.org/1629010/diff/31001/16002#newcode154
chrome/browser/autocomplete/autocomplete_edit_view_mac.mm:154: DCHECK(image)
On 2010/04/09 23:38:55, viettrungluu wrote:
> Wha? Why don't you just NOTREACHED() here?

Done.

Powered by Google App Engine
This is Rietveld 408576698