Bug 31375 - Web Inspector: breakpoints in named evals are not restored after a reload
Summary: Web Inspector: breakpoints in named evals are not restored after a reload
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2009-11-11 14:13 PST by Patrick Mueller
Modified: 2009-11-12 09:00 PST (History)
8 users (show)

See Also:

preliminary patch (1.37 KB, patch)
2009-11-11 14:28 PST, Patrick Mueller
no flags Details | Formatted Diff | Diff
proposed patch 2009/11/12 - a (4.29 KB, patch)
2009-11-12 07:16 PST, Patrick Mueller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Mueller 2009-11-11 14:13:52 PST
If you have "named evals", by virtual of using the //@sourceURL= comment annotation, breakpoints set in these scripts are not restored when the original page is reloaded.  This is a problem with frameworks like Dojo that use evals() for module loading during development.

The breakpoint sidebar will (I believe) show previously set breakpoints after a reload, but they're zombies.  Clicking on them will not take you to the source.  The breakpoints are also not actually set in the source anyway.  So there's no way to get rid of these zombie breakpoints.
Comment 1 Timothy Hatcher 2009-11-11 14:15:36 PST
We could try to reinstall them by matching up the @sourceURL.
Comment 2 Patrick Mueller 2009-11-11 14:28:03 PST
Created attachment 43002 [details]
preliminary patch

The patches are actually simpler than that - the code was pretty much set up to handle this, except the calculated sourceURL wasn't used in ScriptsPanel.addScript(), and in the same method the _sourceIDMap[] map entry is simply set too late for the big if block to take advantage of it.

I'm reusing an existing manual test case to test this, but haven't changed the manual description; I'll do that tomorrow, but thought folks might want to see the current patch.  Looks reasonable.

Also note, I was planning on reorging the breakpoint stuff with this change, but now I'm thinking that I should just do the re-org in a separate patch.  I suspect it will be messy.
Comment 3 Timothy Hatcher 2009-11-11 14:56:05 PST
This patch looks good. Make a ChangeLog and I will r+ it.
Comment 4 Patrick Mueller 2009-11-12 07:16:32 PST
Created attachment 43063 [details]
proposed patch 2009/11/12 - a

per previous comment:

- in ScriptsPanel::addScript(), changed to use the calculated sourceURL rather than the original sourceURL.  The calculated one will have an actual name if the script was an eval that used the //@sourceURL= annotation comment

- in ScriptsPanel::addScript(), moved the cache-filling of _sourceIDmap[] to ABOVE the place it's used, later, in a called function, to paint the breakpoint marker in the source panel; without this fix, everything works, but the breakpoint markers are not repainted for these breakpoints

- added a new manual test case
Comment 5 WebKit Commit Bot 2009-11-12 09:00:31 PST
Comment on attachment 43063 [details]
proposed patch 2009/11/12 - a

Clearing flags on attachment: 43063

Committed r50880: <http://trac.webkit.org/changeset/50880>
Comment 6 WebKit Commit Bot 2009-11-12 09:00:37 PST
All reviewed patches have been landed.  Closing bug.