Bug 41461 – Web Inspector: all breakpoints are activated on page reload

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Podivilov]

Web Inspector: all breakpoints are activated on page reload.

[Pavel Podivilov]

Created attachment 60237 [details]
Proposed patch.

[Yury Semikhatsky]

Comment on attachment 60237 [details]
Proposed patch.

Can we have a test for this change?

[Pavel Podivilov]

Created attachment 60511 [details]
Proposed patch.

[Yury Semikhatsky]

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.

[Pavel Podivilov]

Created attachment 60557 [details]
Proposed patch.

[Pavel Podivilov]

(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.

[Yury Semikhatsky]

Comment on attachment 60557 [details]
Proposed patch.

Clearing flags on attachment: 60557

Committed r62645: <http://trac.webkit.org/changeset/62645>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62645 might have broken GTK Linux 32-bit Release

[Eric Seidel (no email)]

Rolled out.

[Pavel Podivilov]

Created attachment 60900 [details]
Updated test.

[Pavel Feldman]

What was the problem? Can you attach diff wrt last reviewed state?

[Pavel Podivilov]

(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).

[Pavel Podivilov]

Created attachment 60907 [details]
Test diff

[Pavel Feldman]

Comment on attachment 60900 [details]
Updated test.

WebKit/chromium/src/js/DebuggerScript.js:98
 +      if (!args.enabled || !DebuggerScript._breakpointsActivated)
Why is this needed?

[Pavel Podivilov]

(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.

[Yury Semikhatsky]

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

[Pavel Podivilov]

Created attachment 61055 [details]
Proposed patch.

[Pavel Podivilov]

(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.

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Comment on attachment 61055 [details]
Proposed patch.

Clearing flags on attachment: 61055

Committed r63305: <http://trac.webkit.org/changeset/63305>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/63305 might have broken Qt Linux Release

[Yury Semikhatsky]

Rolled out in r63314 due to GTK test failure.

[Pavel Podivilov]

Created attachment 62664 [details]
Rebase and add test to gtk skip list.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.