Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1668283002:
Set empty icon to alert dialogs on mac (Closed)
DescriptionShow empty icon on Mac alert dialogs
Thus making them look less like trusted UI. Slightly ugly with the empty space.
Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIVlVmMEJ4Z29YMVk/view?usp=sharing
BUG=584371
Committed: https://crrev.com/975bc20be048fdb4b536a0d5d4d90dd36ec3204f
Cr-Commit-Position: refs/heads/master@{#373897}
Patch Set 1 #
Total comments: 5
Patch Set 2 : No memory leakz #Messages
Total messages: 16 (7 generated)
Description was changed from ========== Set empty icon to alert dialogs on mac BUG= ========== to ========== Show empty icon on Mac alert dialogs Thus making them look less like trusted UI. Slightly ugly with the empty space. Screenshot: https://drive.google.com/corp/drive/u/1/folders/0B9q2eN9gDoUIfjdMaS1JUkM4SkZ6... BUG=584371 ==========
Description was changed from ========== Show empty icon on Mac alert dialogs Thus making them look less like trusted UI. Slightly ugly with the empty space. Screenshot: https://drive.google.com/corp/drive/u/1/folders/0B9q2eN9gDoUIfjdMaS1JUkM4SkZ6... BUG=584371 ========== to ========== Show empty icon on Mac alert dialogs Thus making them look less like trusted UI. Slightly ugly with the empty space. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIVlVmMEJ4Z29YMVk/view?usp=sharing BUG=584371 ==========
meacer@chromium.org changed reviewers: + avi@chromium.org
avi: Can you PTAL? https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:86: [alert_ setIcon:[NSImage new]]; Not sure about the ownership here, does alert own the icon?
droger@chromium.org changed reviewers: + droger@chromium.org
https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:86: [alert_ setIcon:[NSImage new]]; On 2016/02/04 22:17:58, Mustafa Emre Acer wrote: > Not sure about the ownership here, does alert own the icon? Drive-by. I expect this to be a memory leak. Does [alert_ setIcon:nil]; work? Otherwise, I'd think that something like this would be more appropriate: base::scoped_nsobject<NSImage> emptyIcon_([[NSImage alloc] init]); [alert_ setIcon:emptyIcon_.get()];
https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:86: [alert_ setIcon:[NSImage new]]; On 2016/02/04 22:17:58, Mustafa Emre Acer wrote: > Not sure about the ownership here, does alert own the icon? It'll make its own reference, so you're leaking here. Maybe: NSImage* image = [[[NSImage alloc] initWithSize:NSMakeSize(1,1)] autorelease]; [alert_ setIcon:image];
https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:86: [alert_ setIcon:[NSImage new]]; On 2016/02/05 10:33:01, droger wrote: > I expect this to be a memory leak. Gah, yes. I replied but forgot to mail my comments :( The problem with [alert_ setIcon:nil] is that icon is a null_resettable property, so setting it to nil is just going to use the default application icon. If you want a blank icon, you need to specify a blank icon.
https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/1668283002/diff/1/chrome/browser/ui/cocoa/jav... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:86: [alert_ setIcon:[NSImage new]]; On 2016/02/05 17:06:09, Avi wrote: > On 2016/02/04 22:17:58, Mustafa Emre Acer wrote: > > Not sure about the ownership here, does alert own the icon? > > It'll make its own reference, so you're leaking here. > > Maybe: > NSImage* image = [[[NSImage alloc] initWithSize:NSMakeSize(1,1)] autorelease]; > [alert_ setIcon:image]; Done, thanks for the snippet.
lgtm I'll take it.
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1668283002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1668283002/20001
Message was sent while issue was closed.
Description was changed from ========== Show empty icon on Mac alert dialogs Thus making them look less like trusted UI. Slightly ugly with the empty space. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIVlVmMEJ4Z29YMVk/view?usp=sharing BUG=584371 ========== to ========== Show empty icon on Mac alert dialogs Thus making them look less like trusted UI. Slightly ugly with the empty space. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIVlVmMEJ4Z29YMVk/view?usp=sharing BUG=584371 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Show empty icon on Mac alert dialogs Thus making them look less like trusted UI. Slightly ugly with the empty space. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIVlVmMEJ4Z29YMVk/view?usp=sharing BUG=584371 ========== to ========== Show empty icon on Mac alert dialogs Thus making them look less like trusted UI. Slightly ugly with the empty space. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIVlVmMEJ4Z29YMVk/view?usp=sharing BUG=584371 Committed: https://crrev.com/975bc20be048fdb4b536a0d5d4d90dd36ec3204f Cr-Commit-Position: refs/heads/master@{#373897} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/975bc20be048fdb4b536a0d5d4d90dd36ec3204f Cr-Commit-Position: refs/heads/master@{#373897} |
