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)
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: stanshebs, Assigned: jaas)
References
(Depends on 2 open bugs)
Details
Attachments
(2 files, 4 obsolete files)
463.63 KB,
patch
|
Details | Diff | Splinter Review | |
466.69 KB,
patch
|
stuart.morgan+bugzilla
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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).
Component: OS Integration → Widget: Cocoa
Product: Firefox → Core
Updated•18 years ago
|
QA Contact: os.integration → cocoa
Reporter | ||
Comment 1•18 years ago
|
||
I can't help myself, I'm testing the whack-out now...
Reporter | ||
Comment 2•18 years ago
|
||
Josh, you might as well assign to me, I'm just about done with this one, only have startup sequence tinkering left to do.
Reporter | ||
Comment 4•18 years ago
|
||
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
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.
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
updated to current mozilla-central
Attachment #360548 -
Attachment is obsolete: true
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 10•16 years ago
|
||
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-
Comment 11•16 years ago
|
||
> But it breaks the 'open' command.
Oddly, it doesn't break double-clicking on local files, though.
Comment 12•16 years ago
|
||
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
Assignee | ||
Comment 13•16 years ago
|
||
Adds basic support for gurl events.
Attachment #363400 -
Attachment is obsolete: true
Attachment #364528 -
Flags: review?(smichaud)
Comment 14•16 years ago
|
||
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)
Assignee | ||
Comment 15•16 years ago
|
||
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+
Assignee | ||
Comment 16•16 years ago
|
||
pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/3304d08b9dc3
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).
Assignee | ||
Comment 18•16 years ago
|
||
Thanks for the info about DW. Someone should test and file a bug if something doesn't work.
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
Assignee: nobody → joshmoz
Attachment #364528 -
Flags: review?(benjamin)
Attachment #364528 -
Flags: review?(benjamin)
Updated•15 years ago
|
Target Milestone: --- → mozilla1.9.2a1
Updated•15 years ago
|
Depends on: 549680
No longer depends on: 393645
Depends on: 581763
You need to log in
before you can comment on or make changes to this bug.
Description
•