External protocol dialog support for the Mac....

External protocol dialog support for the Mac.

BUG=http://crbug.com/15546
TEST=try to use an external protocol; it should put up a dialog

[Mark Mentovai, 2009-10-05 22:15:35]

http://codereview.chromium.org/256065/diff/1/5
File chrome/browser/cocoa/external_protocol_dialog.mm (right):

http://codereview.chromium.org/256065/diff/1/5#newcode56
Line 56: l10n_util::GetNSStringWithFixup(IDS_EXTERNAL_PROTOCOL_OK_BUTTON_TEXT)];
I think that we want the OK button to be to the right of the cancel button, but
we don't want it to be default.

(We can have no default.)

This puts Cancel on the right, which doesn't feel right to me.

http://codereview.chromium.org/256065/diff/1/5#newcode79
Line 79: [alert_ beginSheetModalForWindow:nil  // nil here makes it app-modal
How's this interact with other sheets?  Tab-modal sheets?  Save Page As…?

http://codereview.chromium.org/256065/diff/1/5#newcode110
Line 110: if ([[alert_ suppressionButton] state] == NSOnState) {
I don't know if this is actually good.  "Cancel" to me says that the dialog will
have no effect, *including* not remembering that the dialog had been displayed. 
But if you check the box here and click cancel, you'll be forever cancelled,
right?

http://codereview.chromium.org/256065/diff/1/5#newcode128
Line 128: CFURLRef openingApp = nil;
nil for Obj-C/Cocoa, NULL for CF and friends including LS.

http://codereview.chromium.org/256065/diff/1/5#newcode129
Line 129: OSStatus status = LSGetApplicationForURL((CFURLRef)url,
Please use ugly casts, same thing on line 137.

[Avi (use Gerrit), 2009-10-05 22:32:11]

http://codereview.chromium.org/256065/diff/1/5
File chrome/browser/cocoa/external_protocol_dialog.mm (right):

http://codereview.chromium.org/256065/diff/1/5#newcode56
Line 56: l10n_util::GetNSStringWithFixup(IDS_EXTERNAL_PROTOCOL_OK_BUTTON_TEXT)];
On 2009/10/05 22:15:35, Mark Mentovai wrote:
> I think that we want the OK button to be to the right of the cancel button,
but
> we don't want it to be default.
> 
> (We can have no default.)
> 
> This puts Cancel on the right, which doesn't feel right to me.

Done.

http://codereview.chromium.org/256065/diff/1/5#newcode79
Line 79: [alert_ beginSheetModalForWindow:nil  // nil here makes it app-modal
On 2009/10/05 22:15:35, Mark Mentovai wrote:
> How's this interact with other sheets?  Tab-modal sheets?  Save Page As…?

This is a global app modal dialog... Ugh, yes, but right now getting it in is
more important.

http://codereview.chromium.org/256065/diff/1/5#newcode110
Line 110: if ([[alert_ suppressionButton] state] == NSOnState) {
On 2009/10/05 22:15:35, Mark Mentovai wrote:
> But if you check the box here and click cancel, you'll be forever cancelled,
> right?

That's how "remember my choice" dialogs work, though..

http://codereview.chromium.org/256065/diff/1/5#newcode128
Line 128: CFURLRef openingApp = nil;
On 2009/10/05 22:15:35, Mark Mentovai wrote:
> nil for Obj-C/Cocoa, NULL for CF and friends including LS.

Done.

http://codereview.chromium.org/256065/diff/1/5#newcode129
Line 129: OSStatus status = LSGetApplicationForURL((CFURLRef)url,
On 2009/10/05 22:15:35, Mark Mentovai wrote:
> Please use ugly casts, same thing on line 137.

That means a double cast, const and static.

[Avi (use Gerrit), 2009-10-06 14:30:26]

http://codereview.chromium.org/256065/diff/1/5
File chrome/browser/cocoa/external_protocol_dialog.mm (right):

http://codereview.chromium.org/256065/diff/1/5#newcode110
Line 110: if ([[alert_ suppressionButton] state] == NSOnState) {
Let's go with your version for now.

[TVL, 2009-10-06 14:41:01]

http://codereview.chromium.org/256065/diff/1005/24
File chrome/browser/cocoa/external_protocol_dialog.mm (right):

http://codereview.chromium.org/256065/diff/1005/24#newcode71
Line 71: warningString];
drive by- maybe a comment as to why this is always save from an l10n pov? 
(ie-windows does the same thing, etc.)

[Avi (use Gerrit), 2009-10-06 14:48:42]

http://codereview.chromium.org/256065/diff/1005/24
File chrome/browser/cocoa/external_protocol_dialog.mm (right):

http://codereview.chromium.org/256065/diff/1005/24#newcode71
Line 71: warningString];
On 2009/10/06 14:41:02, TVL wrote:
> drive by- maybe a comment as to why this is always save from an l10n pov? 
> (ie-windows does the same thing, etc.)

Yes, the other platforms do the same thing, but why would this be an issue? CR
has Bidirectional Category B which is Neutral.

[TVL, 2009-10-06 14:59:13]

On Tue, Oct 6, 2009 at 10:48 AM, <avi@chromium.org> wrote:

>
> http://codereview.chromium.org/256065/diff/1005/24
> File chrome/browser/cocoa/external_protocol_dialog.mm (right):
>
> http://codereview.chromium.org/256065/diff/1005/24#newcode71
> Line 71: warningString];
> On 2009/10/06 14:41:02, TVL wrote:
>
>> drive by- maybe a comment as to why this is always save from an l10n
>>
> pov?
>
>> (ie-windows does the same thing, etc.)
>>
>
> Yes, the other platforms do the same thing, but why would this be an
> issue? CR has Bidirectional Category B which is Neutral.


Just to the fact that the items never need to be in a different order (say:
description, url, app or url/app in the middle of the description).


>
> http://codereview.chromium.org/256065
>

[Avi (use Gerrit), 2009-10-06 15:04:24]

On 2009/10/06 14:59:13, TVL wrote:
> Just to the fact that the items never need to be in a different order (say:
> description, url, app or url/app in the middle of the description).

But that's clear from the code. We have three strings, we use substitution with
two of them, and then since they're complete sentences we can append them. It's
clear what's going on, and the other platforms do the same thing.

[Avi (use Gerrit), 2009-10-06 19:18:14]

> Just to the fact that the items never need to be in a different order (say:
> description, url, app or url/app in the middle of the description).

I agree this is not a clean design; file a bug.

[Mark Mentovai, 2009-10-06 19:20:00]

LGTM

For the record, I think that remembering "don't open" is OK if the button's text
is something like "don't open" and not simply "cancel."

[Evan Stade, 2009-10-06 19:44:37]

FWIW, I think the external protocol handler ought to be changed to an infobar at
some point (there's no reason I know of to be modal). Same for the multiple
download dialog. But I don't know how we'd fit all that text into one measly
infobar.

[Avi (use Gerrit), 2009-10-06 19:49:34]

On 2009/10/06 19:44:37, Evan Stade wrote:
> FWIW, I think the external protocol handler ought to be changed to an infobar
at
> some point

There's a lot we can do to improve the UI here (three paragraphs of info text is
waaaay too much). But yeah, that'd be cool.

I just wanted to get this functionality finally in.