Summary: | Web Inspector: all breakpoints are activated on page reload | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Podivilov <podivilov> | ||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Pavel Podivilov <podivilov> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, pfeldman, webkit.review.bot, yurys | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Bug Depends on: | 41757, 42256 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Pavel Podivilov
2010-07-01 06:33:54 PDT
Created attachment 60237 [details]
Proposed patch.
Comment on attachment 60237 [details]
Proposed patch.
Can we have a test for this change?
Created attachment 60511 [details]
Proposed patch.
Comment on attachment 60511 [details]
Proposed patch.
LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:75
+ * Checks that all expected scripts are present in the scripts list
You don't need to port these comments.
WebCore/inspector/front-end/ScriptsPanel.js:614
+ if (!options)
I's rather change this to options = options || {};
LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:164
+ Tests that breakpoints are not activated on page reload.
We usually put link to a bug in test descriptions.
LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:125
+ function frontend_showMainPageScriptSource(scriptName, callback)
frontend_showScriptSource would better reflect what it does since it doesn't
use the fact that scriptName is the name of the main resource.
LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:145
+ WebInspector.currentPanel._showScriptOrResource(scriptResource);
Use scriptsPanel instead of WebInspector.currentPanel as you do below.
LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:27
+ var testName = "debugger-breakpoints-not-activated-on-reload.html";
You can get the test name from document.location
LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:58
+ testController.notifyDone();
You need to wait until all scripts are pushed to the front-end again, right?
r- for that.
Created attachment 60557 [details]
Proposed patch.
(In reply to comment #4) > (From update of attachment 60511 [details]) > LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:75 > + * Checks that all expected scripts are present in the scripts list > You don't need to port these comments. > > WebCore/inspector/front-end/ScriptsPanel.js:614 > + if (!options) > I's rather change this to options = options || {}; > > LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:164 > + Tests that breakpoints are not activated on page reload. > We usually put link to a bug in test descriptions. > > LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:125 > + function frontend_showMainPageScriptSource(scriptName, callback) > frontend_showScriptSource would better reflect what it does since it doesn't > use the fact that scriptName is the name of the main resource. > > LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:145 > + WebInspector.currentPanel._showScriptOrResource(scriptResource); > Use scriptsPanel instead of WebInspector.currentPanel as you do below. Done. > > LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:27 > + var testName = "debugger-breakpoints-not-activated-on-reload.html"; > You can get the test name from document.location I'm not sure how to grab document.location from inspected page, so I've used WebInspector.mainResource.url instead. > LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:58 > + testController.notifyDone(); > You need to wait until all scripts are pushed to the front-end again, right? > r- for that. This code is called only after main script with breakpoint is parsed, it should be enough. Comment on attachment 60557 [details] Proposed patch. Clearing flags on attachment: 60557 Committed r62645: <http://trac.webkit.org/changeset/62645> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/62645 might have broken GTK Linux 32-bit Release Rolled out. Created attachment 60900 [details]
Updated test.
What was the problem? Can you attach diff wrt last reviewed state? (In reply to comment #12) > What was the problem? Can you attach diff wrt last reviewed state? It was a problem with JSC (fixed in http://trac.webkit.org/changeset/62769). The patch is the same except for the test and expectation (see attached diff). Created attachment 60907 [details]
Test diff
Comment on attachment 60900 [details]
Updated test.
WebKit/chromium/src/js/DebuggerScript.js:98
+ if (!args.enabled || !DebuggerScript._breakpointsActivated)
Why is this needed?
(In reply to comment #15) > (From update of attachment 60900 [details]) > WebKit/chromium/src/js/DebuggerScript.js:98 > + if (!args.enabled || !DebuggerScript._breakpointsActivated) > Why is this needed? Without this flag, breakpoints will be restored in "enabled" state on reload, even if all breakpoints are deactivated in frontend. Please see ScriptDebugServer::setBreakpointsActivated implementation in js bindings. Comment on attachment 60900 [details]
Updated test.
LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:112
+ frontend_addSniffer(view, "_sourceFrameSetupFinished", function(event)
Please extract common code that waits for frame loading before executing callback from here and frontend_checkExecutionLineWhenSourceFrameIsLoaded in debugger-pause-in-eval-script.html test.
LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:56
+ function frontend_scriptsAreParsed(scripts)
you may want to move these methods to debugger-test.js
Created attachment 61055 [details]
Proposed patch.
(In reply to comment #17) > (From update of attachment 60900 [details]) > LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:112 > + frontend_addSniffer(view, "_sourceFrameSetupFinished", function(event) > Please extract common code that waits for frame loading before executing callback from here and frontend_checkExecutionLineWhenSourceFrameIsLoaded in debugger-pause-in-eval-script.html test. We can start using new function frontend_ensureSourceFrameLoaded in debugger-pause-in-eval-script.html since the test is fixed. > > LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:56 > + function frontend_scriptsAreParsed(scripts) > you may want to move these methods to debugger-test.js Done. Comment on attachment 61055 [details]
Proposed patch.
LayoutTests/inspector/debugger-breakpoints-not-activated-on-reload.html:41
+ if (!WebInspector.flag) {
I'd rather use more specific variable name or something like frontend_testBreakpointsNotActivatedOnReload.flag to avoid possible conflicts.
Comment on attachment 61055 [details] Proposed patch. Clearing flags on attachment: 61055 Committed r63305: <http://trac.webkit.org/changeset/63305> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/63305 might have broken Qt Linux Release Rolled out in r63314 due to GTK test failure. Created attachment 62664 [details]
Rebase and add test to gtk skip list.
Comment on attachment 62664 [details] Rebase and add test to gtk skip list. Clearing flags on attachment: 62664 Committed r64133: <http://trac.webkit.org/changeset/64133> All reviewed patches have been landed. Closing bug. |