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

Issue 196103: Add "New Window" and "New Incognito Window" items to the Dock Menu.... (Closed)

Created:
11 years, 3 months ago by sgk
Modified:
9 years, 5 months ago
CC:
chromium-reviews_googlegroups.com, John Grabowski, pam+watch_chromium.org, Ben Goodger (Google), Paweł Hajdan Jr.
Visibility:
Public.

Description

Add "New Window" and "New Incognito Window" items to the Dock Menu. Add a unit test module for AppController with a simple DockMenu test. BUG=21175 TEST=AppControllerTest.DockMenu Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26124

Patch Set 1 #

Total comments: 2

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -2 lines) Patch
M chrome/browser/app_controller_mac.mm View 1 2 3 chunks +26 lines, -1 line 4 comments Download
A + chrome/browser/app_controller_mac_unittest.mm View 1 chunk +1 line, -1 line 1 comment Download
M chrome/chrome.gyp View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
sgk
Resubmit with autorelease of AppController allocated in the unit test.
11 years, 3 months ago (2009-09-11 23:25:08 UTC) #1
John Grabowski
LGTM Thanks especially for watching the waterfall to see what happened. Responsible follow-up is welcomed.
11 years, 3 months ago (2009-09-12 03:34:07 UTC) #2
TVL
could also just put this into the xib and wire it to an IBOutlet http://codereview.chromium.org/196103/diff/1/2 ...
11 years, 3 months ago (2009-09-12 22:02:31 UTC) #3
Nico
This is BUG=21175 btw.
11 years, 3 months ago (2009-09-14 02:10:48 UTC) #4
sgk
Updated to use l10n_util::GetNSStringWithFixup() per TVL.
11 years, 3 months ago (2009-09-14 17:34:31 UTC) #5
pink (ping after 24hrs)
drive-by.... http://codereview.chromium.org/196103/diff/1005/1006 File chrome/browser/app_controller_mac.mm (right): http://codereview.chromium.org/196103/diff/1005/1006#newcode671 Line 671: NSMenu* result = [[[NSMenu alloc] initWithTitle: @""] ...
11 years, 3 months ago (2009-09-14 19:13:03 UTC) #6
John Grabowski
http://codereview.chromium.org/196103/diff/1005/1006 File chrome/browser/app_controller_mac.mm (right): http://codereview.chromium.org/196103/diff/1005/1006#newcode671 Line 671: NSMenu* result = [[[NSMenu alloc] initWithTitle: @""] autorelease]; ...
11 years, 3 months ago (2009-09-14 21:22:22 UTC) #7
sgk
http://codereview.chromium.org/196103/diff/1005/1006 File chrome/browser/app_controller_mac.mm (right): http://codereview.chromium.org/196103/diff/1005/1006#newcode671 Line 671: NSMenu* result = [[[NSMenu alloc] initWithTitle: @""] autorelease]; ...
11 years, 3 months ago (2009-09-14 22:10:53 UTC) #8
pink (ping after 24hrs)
11 years, 3 months ago (2009-09-15 12:22:12 UTC) #9
Oh, you're totally right. I saw the other usage and jumped to the
wrong conclusion about all usages. The correct usage for the return
value case is alloc/init/autorelease with no scoped pointer.

Sorry for the confusion.

On Mon, Sep 14, 2009 at 6:10 PM,  <sgk@chromium.org> wrote:
>
>
> http://codereview.chromium.org/196103/diff/1005/1006
> File chrome/browser/app_controller_mac.mm (right):
>
> http://codereview.chromium.org/196103/diff/1005/1006#newcode671
> Line 671: NSMenu* result = [[[NSMenu alloc] initWithTitle: @""]
> autorelease];
> On 2009/09/14 21:22:22, John Grabowski wrote:
>>
>> On 2009/09/14 19:13:03, pink wrote:
>> > rather than autoreleasing, we prefer to use scoped_nsobject<T> since
>
> we have
>>
>> it
>> > in Chromium.
>
>> For this line here, I don't think so.
>> If it was a scoped_nsobject the returned value would be invalid before
>
> being
>>
>> used by the caller.
>> Given the necessary use of autorelease, it makes some sense to follow
>
> the
>>
>> pattern of autorelease instead of scoped_nsobject in the rest of the
>
> function.
>
>> Yes, we could scope_nsobject it, then retain+autorelease before
>
> returning it,
>>
>> but it isn't clear that is better.
>
> In fact, it does fail as you'd expect (message sent to freed object) if
> you just replace this line with a scoped_nsobject initialization.
>
> Following up with a CL for the other stylistic changes.
>
> http://codereview.chromium.org/196103
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

Powered by Google App Engine
This is Rietveld 408576698