Closed Bug 363747 Opened 18 years ago Closed 16 years ago

remove remaining Carbon apple event code, replace lost functionality

Categories

(Core :: Widget: Cocoa, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: stanshebs, Assigned: jaas)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files, 4 obsolete files)

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
Attached patch fix v0.5 (obsolete) (deleted) — 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.
Attached patch fix v0.6 (obsolete) (deleted) — 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
Attached patch fix v0.6.1 (obsolete) (deleted) — Splinter Review
updated to current mozilla-central
Attachment #360548 - Attachment is obsolete: true
Attached patch saved work 1 (deleted) — Splinter Review
some saved work, not useful for anything
Attached patch fix v1.0 (obsolete) (deleted) — 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
Attached patch fix v1.1 (deleted) — Splinter 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+
Status: NEW → RESOLVED
Closed: 16 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.

Attachment

General

Creator:
Created:
Updated:
Size: