Closed Bug 542936 Opened 10 years ago Closed 10 years ago

improve nsCommandLineServiceMac, runs at startup

Categories

(Core :: General, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch fix v1.0 (obsolete) — Splinter Review
I was working on a different bug and I noticed that we might not need nsMacCommandLine. Getting rid of it would remove some work done during startup and make some code cleaner. I wrote a patch to remove it and it doesn't seem to change our behavior but I might be missing something.

We only seem to use it for two things - opening files on disk and printing. For opening files on disk we can just use nsICommandLineRunner directly, and the printing stuff wasn't implemented anyway.
Attachment #424174 - Flags: review?(benjamin)
Attached patch fix v1.1 (obsolete) — Splinter Review
Remove an old comment.
Attachment #424174 - Attachment is obsolete: true
Attachment #424175 - Flags: review?(benjamin)
Attachment #424174 - Flags: review?(benjamin)
Attached patch fix v1.2 (obsolete) — Splinter Review
Another small fix.
Attachment #424175 - Attachment is obsolete: true
Attachment #424325 - Flags: review?(benjamin)
Attachment #424175 - Flags: review?(benjamin)
Attachment #424325 - Flags: review?(benjamin)
Attachment #424325 - Attachment is obsolete: true
We can't simply remove this, though we can probably improve it and reduce its scope.
Summary: possibly remove nsCommandLineServiceMac/nsMacCommandLine → improve nsCommandLineServiceMac, runs at startup
Attached patch fix v2.0 (obsolete) — Splinter Review
We can greatly simplify this code, make it easier to understand and maybe faster.
Attachment #443807 - Flags: review?
Attachment #443807 - Flags: review? → review?(benjamin)
Attachment #443807 - Flags: review?(benjamin) → review?(mstange)
(In reply to comment #3)
> We can't simply remove this

Why not, after all?
It does some command line monkey business, which will be easier to understand and improve in the future if this patch lands. It's pretty clear what it does with this patch applied.
Comment on attachment 443807 [details] [diff] [review]
fix v2.0

>diff --git a/toolkit/xre/nsCommandLineServiceMac.cpp b/toolkit/xre/nsCommandLineServiceMac.cpp

>-nsresult nsMacCommandLine::HandleOpenOneDoc(const CFURLRef file, OSType inFileType)

>-  if (!mStartedUp) {
...
>-    return AddToCommandLine("-url", file);
>-  }

Why can we remove this block of code? I'm not really sure what it does...

r=me with that answered.
Attachment #443807 - Flags: review?(mstange) → review+
(In reply to comment #7)

That code was used on classic Mac OS where there was no terminal - people would put args and env variables in a file. I have never seen someone use command-line file. Also, that code already wouldn't run given that we only call HandleOpenOneDoc after startup.
Attached patch fix v2.1Splinter Review
Updated to current trunk.
Attachment #443807 - Attachment is obsolete: true
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/c448572f61cf
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.