Closed Bug 483685 Opened 13 years ago Closed 11 years ago

Make Venkman not rely on JSD starting early on trunk (and 1.9.1?)

Categories

(Other Applications Graveyard :: Venkman JS Debugger, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [fixed by bug 576869])

Attachments

(1 file)

Per bug 480765 comment #2, we should take advantage of the fix there so we no longer need to force JSD to start early and inflict perf issues on everyone.

Not sure how that's supposed to actually work, though. I'm assuming we need to somehow check for new scripts elsewhere as well. However, that doesn't fix obtaining the complete list of currently loaded scripts (for the script list view), afaict. What am I missing, Wladimir? (I should note I haven't had time to look into this deeply...)
My suggestion was to call jsdScriptCreated() from jsdCallHook(). Something like this:

  if (!(frame.script.tag in console.scriptWrappers))
    jsdScriptCreated(frame.script);

While that won't give you a completely populated list of scripts at startup, scripts will be added as they execute - and that should put the most relevant ones in the list pretty soon. Which should be good enough to disable debugger initialization on browser startup (at least by default).
(In reply to comment #1)
> My suggestion was to call jsdScriptCreated() from jsdCallHook(). Something like
> this:
> 
>   if (!(frame.script.tag in console.scriptWrappers))
>     jsdScriptCreated(frame.script);
> 
> While that won't give you a completely populated list of scripts at startup,
> scripts will be added as they execute - and that should put the most relevant
> ones in the list pretty soon. Which should be good enough to disable debugger
> initialization on browser startup (at least by default).

Hm. It'd be interesting to check if there was some way we could enumerate all the components at least... though I suppose that can be fixed in the existing bug we have on that matter (bug 374833). Anyway, that sounds like a decent plan, though obviously we'd need to have the appropriate null checks for frame.script.tag before using it like that :-)

I'll try to do a patch tonight.
So this is what I came up with based on comment #2. However, it doesn't actually fully work. For example, if I follow the following steps:

1. Start Venkman
2. From the Loaded Scripts view, open venkman-overlay.js
3. Set a breakpoint in the function handling the menuclick
4. In the browser window, open Venkman again.

In current trunk (with jsds.initAtStartup true as usual) this works and breaks. With this patch (and hence jsds.initAtStartup false), the script is not in the loaded scripts view. Even if we find it in the "Open Windows" view, and load it from there, we can only set future breakpoints because Venkman doesn't know about any JSDScript for it. Breakpoints then also don't get hit, of course, and even if we click the menu item several times, the script doesn't get picked up by Venkman for use in the loaded scripts window. AFAICT some other scripts do get picked up, so I'm not sure what this is about...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Unfortunately, I don't know enough about the way Venkman works internally to tell what the problem is here :-(
Blocks: 483282
OK, so after a little bit of debugging, I'm suspecting that this is because Venkman ordinarily doesn't set the functionHook (unless you're already stepping through code), which means it doesn't get to know about all these "new but not really new" scripts. Wladimir, would that make sense?
Yes, that should be it. functionHook/topLevelHook is where you would usually see these scripts - scriptHook isn't triggered, it is too late for that.
Depends on: 531837
Depends on: 519719
No longer depends on: 531837
Finally fixed by bug 576869. Thanks for the help, Wladimir! :-)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 576869
Whiteboard: [fixed by bug 576869]
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.