remove remaining Carbon apple event code, replace lost functionality

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
P2
normal
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: stanshebs, Assigned: jaas)

Tracking

(Depends on 2 bugs)

Trunk
mozilla1.9.2a1
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

The old Carbon-based Apple Events code in xpfe/bootstrap/appleevents should not be built or linked into Cocoa configs; it is redundant with Cocoa delegate-based handling added in 355352.

There are a couple technical problems that will need to be solved:

1) In toolkit/xre/nsCommandLineServiceMac.cpp, ProcessAppleEvents() handles files passed by double-clicking on them. Not clear if removing the call will let the app delegate handle everything via odoc's.

2) The Carbon AE also purports to set up a bunch of scripting vocabulary that one might use to control browser from AppleScript. Not clear if any of this works, or is expected to work (my guess is no to both).
Depends on: 355352
Component: OS Integration → Widget: Cocoa
Product: Firefox → Core
Assignee: nobody → joshmoz
QA Contact: os.integration → cocoa
I can't help myself, I'm testing the whack-out now...
Josh, you might as well assign to me, I'm just about done with this one, only have startup sequence tinkering left to do.
Assignee: joshmoz → stanshebs
Any update on this?
Heh, well, that "startup sequence" is trickier than it seems. The AE bits are tied in with the fake command-line stuff, and the restart stuff, and so on, so without AE, there are several behavioral regressions, like double-clicking on a file no longer works. I've backburnered this one for now, if you want the current state of the patch I can attach it.
Summary: Don't include old Carbon Apple Events code if Cocoa → Don't include old Carbon Apple Events code in Cocoa Mozilla
Posted patch fix v0.5 (obsolete) — Splinter Review
This has the same behavioral regressions that Stan mentioned, but it compiles without all the carbon AppleScript code. Just wanted to leave it here as a place to start later on.
Posted patch fix v0.6 (obsolete) — Splinter Review
Updated patch to remove carbon apple event handling. This probably has some functional regressions which we'll need to fix by adding Cocoa handlers for some apple events.
Attachment #306399 - Attachment is obsolete: true
Assignee: stanshebs → nobody
Severity: enhancement → normal
Component: Widget: Cocoa → Cmd-line Features
Flags: wanted1.9.2+
Priority: -- → P2
QA Contact: cocoa → cmd-line
Summary: Don't include old Carbon Apple Events code in Cocoa Mozilla → remove remaining Carbon apple event code, replace lost functionality
Assignee: nobody → smichaud
Assignee: smichaud → joshmoz
Blocks: 468509
Posted patch fix v0.6.1 (obsolete) — Splinter Review
updated to current mozilla-central
Attachment #360548 - Attachment is obsolete: true
Posted patch saved work 1Splinter Review
some saved work, not useful for anything
Posted patch fix v1.0 (obsolete) — Splinter Review
This removes the Carbon apple event code. Almost all of the functionality and more has been replaced by the default Cocoa script suites. The only thing we're losing is get/set URL in the front-most window, that scripting API was badly designed and doesn't belong in generic Gecko anyway. Script can still open URLs in Firefox other ways, by telling the app to open a simple URL file for example. We should file a bug on exposing a script suite for Firefox and whatever other apps wants suites specific to them.

The following example scripts work with this patch:

tell application "Firefox"
	return name of first window as text
end tell

tell application "Firefox"
	activate
	open "/Users/josh/Desktop/foo.url"
end tell
Attachment #363121 - Attachment is obsolete: true
Attachment #363400 - Flags: review?(smichaud)
Comment on attachment 363400 [details] [diff] [review]
fix v1.0

This (basically) looks fine, and the tests you describe in comment #9
both work fine.

But it breaks the 'open' command.

When a build made with this patch is running, and Firefox is the
default browser, and you enter 'open filename.html' at a Terminal
prompt, the browser is made active, but nothing else happens.  What
should happen (and what does happen even with today's Minefield
nightly) is that the browser becomes active and 'filename.html' loads.

For that reason I'm going to r- this patch.

I suspect it's ProcessAppleEvents() in nsCommandLineServiceMac.cpp
that makes the 'open' command work properly ... but I didn't have time
to test this.
Attachment #363400 - Flags: review?(smichaud) → review-
> But it breaks the 'open' command.

Oddly, it doesn't break double-clicking on local files, though.
Josh, I've figured out why the 'open' command doesn't work with	your
patch, and how to fix the problem:  The 'open' command sends a GURL
AppleEvent (http://www.scripting.com/midas/geturl.html).  So all you need
to do is install a handler for this event (using [NSAppleEventManager
setEventHandler:andSelector:forEventClass:andEventID:]).

This is what Safari does.  I found all this out using a little reverse
engineering :-)

For the record, Safari uses [NSAppleEventManager setEventHandler:...] to
install handlers for the following Apple events:

GURL/GURL
WWW!/OURL
aevt/oapp
aevt/rapp
aevt/odoc
aevt/pdoc
aevt/ocon
aevt/quit
aevt/actq
SECF/test
Posted patch fix v1.1Splinter Review
Adds basic support for gurl events.
Attachment #363400 - Attachment is obsolete: true
Attachment #364528 - Flags: review?(smichaud)
Comment on attachment 364528 [details] [diff] [review]
fix v1.1

r=me in that the new code looks fine, but I'm not familiar with the old AE code at all (or what you're doing it terms of other Cocoa-level AE handling) so I have no idea what other regressions this could potentially introduce.

>+    NSString* schemeString = [[NSURL URLWithString:urlString] scheme];
>+    if (!schemeString ||
>+        [schemeString compare:@"chrome"
>+                      options:NSCaseInsensitiveSearch
>+                        range:NSMakeRange(0, [schemeString length])] == NSOrderedSame) {
>+      return;

Do you want javascript: URLs to work? If so, you won't want to use NSURL. It will choke on the unescaped JS that those URLs often contain and return nil, so you'll return early.

Have you tested handling of open when Firefox *isn't* already running with this change?
Attachment #364528 - Flags: review?(smichaud) → review+
Attachment #364528 - Flags: superreview?(roc)
Comment on attachment 364528 [details] [diff] [review]
fix v1.1

This does work when the app isn't already running and js urls don't matter. That AE handler is only there to cover the common case of using "open" on a local document, the fact that it works with standard URLs like http/https URLs is a bonus at this point. We'll be re-examining extended apple event support in the future.
Attachment #364528 - Flags: superreview?(roc) → superreview+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/3304d08b9dc3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #12)
> WWW!/OURL

As long as you're planning on matching Safari's AEs before this ships, you're good, but just FYI, you need to make sure you implement support for WWW!OURL (even in one of the "hidden" ways that Safari and Camino use) or you'll break Dreamweaver and a large number of other apps.  

That (bug 403346 et seq) was one of the two most difficult regressions to track down and fix after peeja rewrote Camino's AppleScript support (the other being bug 415510 et seq).
Thanks for the info about DW. Someone should test and file a bug if something doesn't work.
Product: Core → Core Graveyard
Moving this back to a live component where it can be found in searches, since it's a change made 3 months ago that will be new in 1.9.2, not some change from 2001 :P
Assignee: joshmoz → nobody
Component: Cmd-line Features → Widget: Cocoa
Product: Core Graveyard → Core
QA Contact: cmd-line → cocoa
Blocks: 502193
Depends on: 514108
Attachment #364528 - Flags: review?(benjamin)
Attachment #364528 - Flags: review?(benjamin)
Blocks: 531552
Target Milestone: --- → mozilla1.9.2a1
No longer blocks: 531552
Depends on: 531552
Depends on: 393645
You need to log in before you can comment on or make changes to this bug.