Last Comment Bug 483681 - Make SetInitAtStartup actually work
: Make SetInitAtStartup actually work
Status: RESOLVED FIXED
:
Product: Other Applications
Classification: Client Software
Component: Venkman JS Debugger (show other bugs)
: Trunk
: All All
: -- critical with 1 vote (vote)
: ---
Assigned To: Karsten Düsterloh
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
: 123958 (view as bug list)
Depends on: 364864
Blocks: 483282
  Show dependency treegraph
 
Reported: 2009-03-16 15:06 PDT by :Gijs Kruitbosch
Modified: 2009-12-17 02:04 PST (History)
7 users (show)
mbeltzner: blocking1.9.1-
mbeltzner: wanted1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
always on with reevaluation [Checkin: Comment 10] (4.88 KB, patch)
2009-03-18 14:26 PDT, Karsten Düsterloh
gijskruitbosch+bugs: review+
Details | Diff | Review

Description :Gijs Kruitbosch 2009-03-16 15:06:31 PDT
Spun off from bug 483282. It turns out SetInitAtStartup actually doesn't work because of bug 364864. We should fix that. What I'm thinking of is the following:

1. Always register for autoreg and app-startup.
2. Keep a pref for whether it should actually start work as soon as possible.
3. Check that pref when the autoreg and app-startup events actually come in.

In this case, we can map the Get/SetInitAtStartup code to the pref, I believe. Timeless, does that sound sane to you?
Comment 1 timeless 2009-03-17 07:35:45 PDT
i really really want bug 364864 to be fixed.

otherwise, changing the code to always have the category (since we don't have a choice) and use a pref is fine with me.

however, i'd appreciate it if we note somewhere to people that they must use the api and not the pref. This probably involves a comment before the pref in all.js and a pref name intentionally picked so that people do not use it directly.
Comment 2 Mike Beltzner [:beltzner, not reading bugmail] 2009-03-17 12:23:39 PDT
Doesn't block, would take fix with tests.
Comment 3 Karsten Düsterloh 2009-03-17 14:13:25 PDT
So I toyed around a bit in jsdASObserver::Observe (which is said app-startup observer), but I can't read user-set prefs there - it seems to be called too early in the startup process (maybe not even a profile yet?).
I can read app default pref values (including those from /bin/extensions/*), but that isn't particularly useful...
Comment 4 Karsten Düsterloh 2009-03-18 14:26:40 PDT
Created attachment 368110 [details] [diff] [review]
always on with reevaluation
[Checkin: Comment 10]

We can't read profile prefs on component registration.
We can't read profile prefs on app startup.
Thus we can't fix the original problem in JSD.

We can read profile prefs on commandline processing, though.
Thus we can avoid the perf hit for the host by investing a few cycles on startup:
- on component registration, always register the app-startup observer
- if the the app-startup observer is registered, let it always turn on JSD
- in the commandline handler (where we already may open Venkman's main window!), check extensions.venkman.jsd.initAtStartup and posiibly correct catman registration and/or JSD state.
Comment 5 Karsten Düsterloh 2009-03-18 14:29:34 PDT
As a sidenote: we still need to fix bug 364864.
Comment 6 :Gijs Kruitbosch 2009-03-26 08:52:34 PDT
I don't understand what this patch fixes. It doesn't change the actual /startup-init command, so it doesn't fix that. It doesn't fix the underlying issue, which you says is still required for this patch to do something. It doesn't fix the method mentioned in the bug summary... so what *does* it do?

Help?
Comment 7 Karsten Düsterloh 2009-05-02 05:56:17 PDT
> It doesn't fix the underlying issue, which you says is still required for
> this patch to do something.

No, it's not required for this, but it still needs fixing.

This patch enables host applications to override/control Venkman's startup registration by using a pref.
Comment 8 :Gijs Kruitbosch 2009-05-02 06:39:24 PDT
Comment on attachment 368110 [details] [diff] [review]
always on with reevaluation
[Checkin: Comment 10]

(In reply to comment #7)
> > It doesn't fix the underlying issue, which you says is still required for
> > this patch to do something.
> 
> No, it's not required for this, but it still needs fixing.
> 
> This patch enables host applications to override/control Venkman's startup
> registration by using a pref.


OK, this makes sense. r=me. Please remember to check in on HG, not on CVS (the patch is from CVS, so...)
Comment 9 :Gijs Kruitbosch 2009-05-02 07:33:50 PDT
*** Bug 123958 has been marked as a duplicate of this bug. ***
Comment 10 Karsten Düsterloh 2009-05-08 10:59:01 PDT
Comment on attachment 368110 [details] [diff] [review]
always on with reevaluation
[Checkin: Comment 10]

Landed as <http://hg.mozilla.org/venkman/rev/9fea2b31c556>.
Comment 11 Serge Gautherie (:sgautherie) 2009-10-05 09:26:46 PDT
What's left to be done here?
Comment 12 :Gijs Kruitbosch 2009-12-01 09:15:27 PST
I'm going to mark this fixed. JSD can't be fixed for the reasons outlined in comment #3 and comment #4, actually making the catmanager work is bug 364864, making Venkman work without JSD starting up on startup at all is bug 483685, making SeaMonkey use this pref to not have JSD startup if Venk is installed at least until it's started at least once is bug 483282, and so I don't see anything to be done in *this* bug. We've got enough other ones open to deal with all the other issues.
Comment 13 neil@parkwaycc.co.uk 2009-12-17 02:04:16 PST
One nit: all command line handlers get reinvoked for remoting (always on Windows; not sure how Mac and Linux work here.)

Note You need to log in before you can comment on or make changes to this bug.