Web Inspector: all breakpoints are activated on page reload.
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>
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>