218271 - Enable Camino to set itself to be the default browser

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Jasper]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5b) Gecko/20030902 Camino/0.7+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5b) Gecko/20030902 Camino/0.7+

In Panther the user is no longer able to set the default web browser through the
internet system preference pane. We should add this option to Camino itself
before Panther comes out otherwise user are not abe to set Camino as their
default browser.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Jennifer]

You can use the prefpane of Safari to set the default browser you want, at least
in bujild 7B85, though users shouldn't have to keep Safari around just to do
this. The pref should have stayed in the system prefpanes. If this, along with
bug 210553, get fixed, Firebird will be my default browser again.

Comment 2

[Mike Pinkerton (not reading bugmail)]

should we just provide a "make camino default" button in one of the pref panels
or should we ask at startup? or both? what pref panel should it go into?

Comment 3

[Andy Boyd]

Do both - ask on first startup and include it as an option in a pref pane, but
it is ambiguous.  Setting as default browser is a one off for many users, this
option wouldn't fit well into Navigation, certainly not Appearance or Security,
perhaps Privacy or Web Features.

Comment 4

[Josh Aas]

We should probably get moving on this soon so we can get some good user testing
in before 0.8. I think we should ask people if they want to set Camino to their
default browser the first time they run it. Then we should have a pref option
where they can change their default browser to Camino at a later time. Do we
need to let them select other browsers as their default from the pref UI? I can
whip up a patch for this once we decide what exactly we want to do in terms of
the ability  to set other browsers as the default.

Comment 5

[Jasper]

I think a pop-up menu like the one Safari uses would be the most user friendly. 

(Note that we also need to change the "homepage > use system prefernces' and
"download to" options in the preferences!)

Comment 6

[Mike Pinkerton (not reading bugmail)]

i was thinking about just a menu item in the app menu that says "Make Default
Browser" and prompts with a "are you sure you want to do this?" since it's
destructive. 

a button in prefs wouldn't work right now because we really don't have any place
for it. We sorta need to wait for the 0.9 prefs restructuring to make a place
for it.

any objections?

Comment 7

[Jasper]

nope

Comment 8

[Mike Pinkerton (not reading bugmail)]

this is non-trivial. there's no launchServices api to do it and so you need to
use IC, but there's no documentation on the key in IC that's used.

anyone know?

Comment 9

[Mike Pinkerton (not reading bugmail)]

over to josh

Comment 10

[Simon Fraser [no longer active]]

When you change the default browser on 10.2 from the Safari prefs, the launch
services plist shows changes in the file associations for html, xhtml and html
files, and for the http and https handlers.

I think changes made via the IC APIs are supposed to reflect back into Launch
Services.

Comment 11

[Steve Dagley]

Yes, the IC prefs are still being reflected into LS.  One of these days Apple
will figure out that LS is still lacking some important calls from IC and add
them.  Hopefully before they remove the IC APIs.

Comment 12

[Mike Pinkerton (not reading bugmail)]

but knowing this doesn't solve the problem of how to change it in IC. i can't
find the appropriate key in any header.

Comment 13

[Simon Fraser [no longer active]]

You need to mess with ICMapEntry routines.

Comment 14

[Stuart Morgan]

*** Bug 169779 has been marked as a duplicate of this bug. ***

Comment 15

[Stuart Morgan]

Updating summary; no reason to make this 10.3-specific

Comment 16

[Simon Fraser [no longer active]]

Note from mailing list:
The nightly of this day crash again when don't start safari after the OS X
installation (the file of internet pref is not present at this moment. Safari
create it on his first launch. why not Camino ? )

Comment 17

[Benjamin Huot]

I set Camino to be the default browser under 10.2.8 with all security updates
and ran repair disk permissions but it still uses Safari to launch html files.
It actually switched it to Safari after I ran Camino because I had Camino as my
default browser before. It now list Camino .8.2 as the default browser but
launching an html file still lanches Safari. This should be fixed before .9
comes out.

Comment 18

[Josh Aas]

Attachment: fix v0.9

This is what I have completed so far. I am posting it so people can give me
feedback before I finish it. I need to rethink adding multiple versions of
particular browsers in the list as it doesn't mesh well with the way protocol
and extension defaults are handled by the system. Except for that issue,
everything should work as you would expect it to.

Please note that I am not asking for formal reviews as I do not think this code
is finished. However, I don't want to discourage people from reviewing this
patch informally and posting suggestions here since it is a quite complicated
system that should be looked at by as many eyes as possible.

A build with this patch can be found at:
http://josh.trancesoftware.com/Camino_def_0.9.zip

Comment 19

[Jasper]

Some feedback:
1) I think it would be good to set this pref at the top of the pane.
2) Makes sure the keyboard navigation access in that pane is correct, default
browser wasn't hooked op to that yet.
3) When ever I change the setting in Camino the list in Safari isn't updated
untill quit and visa versa. Non the less it works properly, just a bit confusing. 

And do we want a check if we are default browser on first launch and show a
window alloig users to set it to default?

Comment 20

[Mike Pinkerton (not reading bugmail)]

i think it should just be a big button saying "make camino the default browser"
and be done with it. we're not in this to make it easy for people to set other
browsers as the default.

Comment 21

[Simon Fraser [no longer active]]

Comment on attachment 172328 [details]
fix v0.9

This patch ain't plain text, for sure.

Can we just see the code changes?

Comment 22

[Jasper]

The problem with using a big button is that if a user has multiple copies of
Camino (I for one have a zillion) the user can't choose which I think canresult
in  a big mess.

Comment 23

[Mike Pinkerton (not reading bugmail)]

can you actually differentiate between multiple versions with the same version
number?

Comment 24

[Jasper]

yes.

Comment 25

[Josh Aas]

Attachment: text only part of the patch v0.9

Comment 26

[Simon Fraser [no longer active]]

Attachment: Screenshot

I know my machine isn't exactly typical, but I wonder if it's worth
differentiating between different copies of the same application? For example,
it's almost impossible for me to remember which copy of Firefox is which, but
if it said "Firefox (0.8) in folder Applications on Panther" it would be more
helpful.

I agree with Josh in that a popup like this is preferable to a single button.
Without that, you give users no way to back out of their decision.

Also, some of these applications that it finds are probably in another user's
folder. Are we making sure that the the applications that it finds are
executable by the user?

Comment 27

[Simon Fraser [no longer active]]

The code looks reasonable at first glance, but it has tabs in it.

Comment 28

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

FWIW, I like the argument in comment 20 (particularly if there were some way for
the button/code to know that it's the currently open Camino that should be set
as the default, not any of the others.  The drop-down list is bound to be
"obtuse" to the only people who would really benefit from it--Camino devs and
testers--if there's no way to indicate which 0.8+ build one is selecting). 

If one wants to set another browser as one's default, one should do it from
within that other browser, use a third-party utility (MisFox, RCDefaultApp,
MoreInternet, etc.,) or set it in Safari (Internet prefpane in 10.2).

FYI, Eudora uses a checkbox (which isn't a real toggle so it should really be a
button), but I'm not aware of any other browsers offering this option (OmniWeb
perhaps?) on Mac OS X.  IIRC, Moz/Gecko browsers on that "other platform" also
have just a button to make them the default (plus a check box for checking every
launch to see if they still are the default...).

Comment 29

[Josh Aas]

Attachment: patch v1.0

Comment 30

[Simon Fraser [no longer active]]

General:

It seems that wrapping LSCopyDisplayNameForURL() in a class helper method would
clean up some of the code.

> +#define PREFERENCES [NSUserDefaults standardUserDefaults]

This just obfuscates, I think. Copy into a local variable if you want to.

> +  BOOL alreadyExists = NO;
> +  for (unsigned int i = 0; i < [userChosenBrowsers count]; i++) {
> +    if ([chosenBrowser isEqualToString:[userChosenBrowsers objectAtIndex:i]]) {
> +      alreadyExists = YES;
> +      break;
> +    }
> +  }

Could you use [NSArray containsObject:] here instead? That's supposed to call
the isEqual: methods of the objects.


> +  if (!alreadyExists) {
> +    [userChosenBrowsers addObject:chosenBrowser];
> +    [PREFERENCES setObject:userChosenBrowsers forKey:@"UserChosenBrowsers"];
> +  }
> +  // make it the default browser
> +  [self setDefaultBrowser:chosenBrowser];
> +  // update browser menu - this will cause the app to show up and be selected
> +  [self updateDefaultBrowserMenu];
> +}

> +	NSURL *httpURL = [NSURL URLWithString:@"http:"];
> +	NSURL *httpsURL = [NSURL URLWithString:@"https:"];
> +	NSArray *apps;
> +	NSMutableArray *browsers = [NSMutableArray arrayWithCapacity:10];
> +	NSURL *anApp;
> +	Boolean canHandleHTTPS;
> +  NSString *name1;
> +  NSString *name2;

Move these declarations into more local scope. (and remove the tabs everywhere).
This applies to other methods too.

Comment 31

[Simon Fraser [no longer active]]

> "Application Does Not Exist" = "Application Does Not Exist";

Only capitalize the first word.

> "BrowserDoesNotExistExplanation" = "The default browser you have chosen no
longer exists. Please choose again.";

Sounds too similar to the lame "please try again". How about "Please choose
another browser."

Comment 32

[Wevah (Nate Weaver)]

The cancel button isn't working in the sheet for choosing the browser; I get
"-[NSCFArray addObject:]: attempt to insert nil", and then the OK button stops
working, too.

Comment 33

[Wevah (Nate Weaver)]

Also, Safari differentiates between different versions of the same browser,
possibly (probably?) with the short version string (CFBundleShortVersionString).
The correct copy *is* launched when I switch between them.

Comment 34

[Wevah (Nate Weaver)]

> +  _LSSetWeakBindingForType(0, 0, (CFStringRef)weblocString, kLSRolesAll,
&browserFSRef);

I don't think we should be associating .webloc files with camino; they should
stay associated with the Finder, which should then ask Launch Services for the
proper application to handle the protocol of the URL within the .webloc file.

Comment 35

[Josh Aas]

Attachment: patch v1.1

Fixes everything commented on from version 1.0 except for version
differentiation, which I have yet to decide if/how to do. Not requesting
reviews on this version until I decide whether or not it is the one I want to
land.

Comment 36

[Josh Aas]

Attachment: button fix v1.0

I'm opting for a more simple single button fix for this. Turns out there are
some nasty issues with doing what Safari does and doing it right. This patch
works fine, but the UI might need some work. Maybe some explanation, and/or
maybe it should say what the current default browser is. Right now I just
disable the button if Camino is already default, but we can probably do better.
I'm not going to polish the UI until there is some discussion.

Comment 37

[Jasper]

I'd like to recommend that you make it a check box instead of a button. If you
make it a checkbox users will be able to see if camino already is or isn't the
default browser. Right now you can't tell in any way if we are already. We need
some feedback.

Comment 38

[Wevah (Nate Weaver)]

But the user shouldn't be able to uncheck it (since the default browser has to
be *something*); I think a checkbox might be even more confusing, personally.

Comment 39

[Jasper]

Hm that true, but I find that a button doesn't indicate anything. How are simple
users to know Camino already is the default browser. Somehow they should be
given feedback. I guess that's why all other apps use a po-up menu. Oh well.

Comment 40

[Mike Pinkerton (not reading bugmail)]

how about a button that is disabled if we are the default browser, with some
text that appears under it that says "Camino is already the default browser"?

Comment 41

[Jasper]

Hey that was exactly what I was thinking about aswell. Disabling the button if
we are default alread, with indeed some explanatory text beneath would just do
the trick.

Comment 42

[Simon Fraser [no longer active]]

The disabled button thing just seems weird to me.

What none of the non-popup solutions allow the user to do is to undo their
changes. Almost every other preference change can be reversed by going back to
the place where you set the pref, and resetting an option. If setting the
default browser uses a simple button, the user can't undo their browser
selection _by going back to the place where they changed it_. Sure, they can
launch Safari and look in its prefs, but that's not exactly obvious.

So, I'd prefer the popup if it can be done.

Comment 43

[Josh Aas]

The previous patch v1.1 does work quite well. It just doesn't differentiate
between versions of the same app that have different bundle IDs. It
differentiates based on app name for UI and some funny carbon/cocoa app
interaction reasons. If we want a popup version, "patch v1.1" will work just
fine for 99.9% of users.

Now that I think about what Simon is saying I'm thinking the popup button's
flaws might be negligible compared to its benefits.

Comment 44

[Josh Aas]

Comment on attachment 172647 [details]
button fix v1.0

this patch should not be getting any more serious reviews...

Comment 45

[Josh Aas]

Attachment: patch v1.1 text only

Comment 46

[Josh Aas]

Comment on attachment 172536 [details]
patch v1.1

I think we should just land this version of the patch and see what happens for
a while. I'm pretty sure it will be fine.

The nibs in this patch are out of date, but if you just test with this I'll
update the nib if/when it lands. This nib has been changing a lot lately :)

Comment 47

[Simon Fraser [no longer active]]

Comment on attachment 172536 [details]
patch v1.1

General comments:
I think you should wrap LSCopyDisplayNameForURL() in a cocoa method to keep the
code cleaner.
Please use C++ style variable declarations (declare on first use), mostly in
updateDefaultBrowserMenu.

Finally, we need to decide on the button vs. popup in the UI. I'm against the
button, because of lack of undoability.

Comment 48

[Josh Aas]

Attachment: patch v1.2 text only

Comment 49

[Josh Aas]

Attachment: patch v1.2 (full)

Comment 50

[Josh Aas]

Comment on attachment 174267 [details]
patch v1.2 (full)

addresses smfr's comments

Comment 51

[Simon Fraser [no longer active]]

> Index: PreferencePanes/Navigation/Navigation.mm
> ===================================================================

> ++ (NSString*)displayNameForApp:(NSString*)app {
> +  NSString *name;
> +  LSCopyDisplayNameForURL((CFURLRef)[NSURL fileURLWithPath:app], (CFStringRef
*)&name);
> +  return [name autorelease];
> +}

Open brace on new line please.

>  - (id)initWithBundle:(NSBundle *)bundle
>  {
>    self = [super initWithBundle:bundle];
> +  [[NSUserDefaults standardUserDefaults] registerDefaults:[NSDictionary
dictionaryWithObject:[NSArray array] forKey:@"UserChosenBrowsers"]];
>    return self;
>  }

Please use the format we use everywhere else (or should):
    if ((self = [super init...]))
    {
        // do init
    }
    return self;

> +- (void)setDefaultBrowser:(NSString*)browser
> +{
> +  NSString *htmString = @"htm";
> +  NSString *htmlString = @"html";
> +  NSString *urlString = @"url";
> +  NSURL *browserURL = [NSURL fileURLWithPath:browser];
> +  
> +  FSRef browserFSRef;
> +  CFURLGetFSRef((CFURLRef)browserURL, &browserFSRef);
> +  
> +  _LSSetDefaultSchemeHandlerURL(@"http", browserURL);
> +  _LSSetDefaultSchemeHandlerURL(@"https", browserURL);
> +  _LSSetWeakBindingForType(0, 0, (CFStringRef)htmString, kLSRolesAll,
&browserFSRef);
> +  _LSSetWeakBindingForType(0, 0, (CFStringRef)htmlString, kLSRolesAll,
&browserFSRef);
> +  _LSSetWeakBindingForType(0, 0, (CFStringRef)urlString, kLSRolesAll,
&browserFSRef);

Note sure why you can't just cast a literal string:

  _LSSetWeakBindingForType(0, 0, (CFStringRef)@"html", kLSRolesAll, &browserFSRef);

> +- (void)openPanelDidEnd:(NSOpenPanel *)sheet returnCode:(int)returnCode
contextInfo:(void *)contextInfo
> +{
> +  if (returnCode == NSOKButton) {
> +    NSString *chosenBrowser = [[[sheet filenames] objectAtIndex:0]
stringByStandardizingPath];
> +    // add this browser to a list of apps we should always consider as browsers
> +    NSMutableArray *userChosenBrowsers = [NSMutableArray arrayWithCapacity:1];
> +    [userChosenBrowsers addObjectsFromArray:[[NSUserDefaults
standardUserDefaults] objectForKey:@"UserChosenBrowsers"]];

Please declare a constant NSString* for @"UserChosenBrowsers" at the top of the
file:

static NSString* const kUserChosenBrowserUserDefaultsKey = @"UserChosenBrowsers";

and use it in place of @"UserChosenBrowsers" everywhere.

Since you are storing full paths to apps in the prefs, what happens if the user
moves the apps around? Or if they point to an app on a removable volume, and
it's not available?

> +    BOOL alreadyExists = NO;
> +    for (unsigned int i = 0; i < [userChosenBrowsers count]; i++) {
> +      if ([chosenBrowser isEqualToString:[userChosenBrowsers objectAtIndex:i]]) {
> +        alreadyExists = YES;
> +        break;
> +      }
> +    }

Can't you just use [NSArray containsObject:] here? It calls isEqual: on the entries.


> +  // We don't want to put more than one version of each browser in the list
as launch
> +  // services picks whatever version it wants instead of listening to us anyway.
> +  // Do this by display name. This is not the most optimized loop scheme but
its simple
> +  // and it doesn't need to be very fast.
> +  for (unsigned int i = 0; i < [browsers count]; i++) {
> +    NSString *name1 = [OrgMozillaChimeraPreferenceNavigation
displayNameForApp:[browsers objectAtIndex:i]];
> +    for (unsigned int j = 0; j < [browsers count]; j++) {
> +      NSString *name2;
> +      if (j == i) continue; // don't compare name to itself
> +      name2 = [OrgMozillaChimeraPreferenceNavigation
displayNameForApp:[browsers objectAtIndex:j]];
> +      if ([name1 isEqualToString:name2])
> +        [browsers removeObjectAtIndex:j];
> +    }
> +  }

It would be almost as simple, and potentialy faster, to sort and then just
remove duplicates linearly.


> +  // set up new menu
> +  NSString *anApp;
> +  NSMenuItem *item;
> +  NSEnumerator *browserEnumerator = [browsers objectEnumerator];
> +  while (anApp = [browserEnumerator nextObject]) {
> +    NSImage *icon;
> +    NSString *name = [OrgMozillaChimeraPreferenceNavigation
displayNameForApp:anApp];
> +    
> +    item = [[NSMenuItem alloc] initWithTitle:[NSString
stringWithFormat:@"%@", name]
> +                                      action:@selector(defaultBrowserChange:)
keyEquivalent:@""];

Declare NSMenuItem* here, inside the loop, since that is its scope.

> +    icon = [[NSWorkspace sharedWorkspace] iconForFile:anApp];

Declare NSImage* here.

Comment 52

[Josh Aas]

Attachment: updated patch v1.3, not for review

This is v1.2 updated taking smfr's comments into account. This is just for my
own keeping, and is not for review. I'm working on a v2.0 that will store
bundle IDs instead of paths since that is the way Launch Services works anyway.
The downside is we won't be able to support single binary carbon apps (i.e.
apps that are not bundles with an Info.plist). I think that is fine though.

Comment 53

[Josh Aas]

Attachment: patch v1.4

This is bundle-ID oriented and thus safer. The code has been cleaned up and
optimized a bit too.

Comment 54

[Josh Aas]

Attachment: patch v1.4 text only

Comment 55

[Josh Aas]

Note that if you tested earlier versions of this patch, you may need to get rid
of any existing "UserChosenBrowsers" values in your Camino cocoa prefs plist. We
now store bundle IDs instead of app paths. 

Comment 56

[Mike Pinkerton (not reading bugmail)]

+  // this shouldn't ever happen
+  return NSOrderedSame;

is "same" the right response for an error condition? how would the calling code
handle this case?

+    _LSSetDefaultSchemeHandlerURL(@"http", browserURL);
+    _LSSetDefaultSchemeHandlerURL(@"https", browserURL);

don't we want to register ftp, gopher, and file as well? we do in the plist.

this is also what we reigster for filetypes in the plist:

				<string>html</string>
				<string>htm</string>
				<string>shtml</string>
				<string>xml</string>
				<string>xhtml</string>
				<string>jpeg</string>
				<string>jpg</string>
				<string>png</string>
				<string>gif</string>
				<string>webloc</string>
				<string>url</string>

rather than hardcoding the protocols and types, can we put the list into a plist
of our own and walk that?

i'd also like to see more function-level comments for the new routines you added.

Comment 57

[Wevah (Nate Weaver)]

(In reply to comment #56)
> +    _LSSetDefaultSchemeHandlerURL(@"http", browserURL);
> +    _LSSetDefaultSchemeHandlerURL(@"https", browserURL);
> 
> don't we want to register ftp, gopher, and file as well? we do in the plist.
> 

No; IIRC this sets the *default* handler, and I, for one, don't want Camino to
be my default FTP application.

Comment 58

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 175787 [details]
patch v1.4

sr=pink

Comment 59

[Josh Aas]

landed on trunk with fixes for pink's comments

Comment 60

[Simon Fraser [no longer active]]

This can cause a hang. See bug 184394#18