Bug 41461 - Web Inspector: all breakpoints are activated on page reload
Summary: Web Inspector: all breakpoints are activated on page reload
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Pavel Podivilov
URL:
Keywords:
Depends on: 41757 42256
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-01 06:33 PDT by Pavel Podivilov
Modified: 2010-07-27 09:12 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch. (8.20 KB, patch)
2010-07-01 06:38 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Proposed patch. (14.60 KB, patch)
2010-07-05 04:27 PDT, Pavel Podivilov
yurys: review-
Details | Formatted Diff | Diff
Proposed patch. (14.02 KB, patch)
2010-07-05 11:22 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Updated test. (13.53 KB, patch)
2010-07-08 09:42 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Test diff (2.40 KB, patch)
2010-07-08 10:32 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Proposed patch. (14.18 KB, patch)
2010-07-09 10:20 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Rebase and add test to gtk skip list. (14.80 KB, patch)
2010-07-27 02:41 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2010-07-01 06:33:54 PDT
Web Inspector: all breakpoints are activated on page reload.
Comment 1 Pavel Podivilov 2010-07-01 06:38:47 PDT
Created attachment 60237 [details]
Proposed patch.
Comment 2 Yury Semikhatsky 2010-07-01 07:37:17 PDT
Comment on attachment 60237 [details]
Proposed patch.

Can we have a test for this change?
Comment 3 Pavel Podivilov 2010-07-05 04:27:57 PDT
Created attachment 60511 [details]
Proposed patch.
Comment 4 Yury Semikhatsky 2010-07-05 06:12:11 PDT
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.
Comment 5 Pavel Podivilov 2010-07-05 11:22:24 PDT
Created attachment 60557 [details]
Proposed patch.
Comment 6 Pavel Podivilov 2010-07-05 11:26:46 PDT
(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 7 Yury Semikhatsky 2010-07-07 02:31:19 PDT
Comment on attachment 60557 [details]
Proposed patch.

Clearing flags on attachment: 60557

Committed r62645: <http://trac.webkit.org/changeset/62645>
Comment 8 Yury Semikhatsky 2010-07-07 02:31:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2010-07-07 02:50:15 PDT
http://trac.webkit.org/changeset/62645 might have broken GTK Linux 32-bit Release
Comment 10 Eric Seidel (no email) 2010-07-07 04:28:15 PDT
Rolled out.
Comment 11 Pavel Podivilov 2010-07-08 09:42:31 PDT
Created attachment 60900 [details]
Updated test.
Comment 12 Pavel Feldman 2010-07-08 09:47:12 PDT
What was the problem? Can you attach diff wrt last reviewed state?
Comment 13 Pavel Podivilov 2010-07-08 10:31:35 PDT
(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).
Comment 14 Pavel Podivilov 2010-07-08 10:32:00 PDT
Created attachment 60907 [details]
Test diff
Comment 15 Pavel Feldman 2010-07-08 13:28:52 PDT
Comment on attachment 60900 [details]
Updated test.

WebKit/chromium/src/js/DebuggerScript.js:98
 +      if (!args.enabled || !DebuggerScript._breakpointsActivated)
Why is this needed?
Comment 16 Pavel Podivilov 2010-07-09 01:26:55 PDT
(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 17 Yury Semikhatsky 2010-07-09 01:52:09 PDT
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
Comment 18 Pavel Podivilov 2010-07-09 10:20:39 PDT
Created attachment 61055 [details]
Proposed patch.
Comment 19 Pavel Podivilov 2010-07-09 10:22:23 PDT
(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 20 Yury Semikhatsky 2010-07-14 01:59:19 PDT
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 21 Yury Semikhatsky 2010-07-14 03:18:28 PDT
Comment on attachment 61055 [details]
Proposed patch.

Clearing flags on attachment: 61055

Committed r63305: <http://trac.webkit.org/changeset/63305>
Comment 22 Yury Semikhatsky 2010-07-14 03:18:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 WebKit Review Bot 2010-07-14 03:44:32 PDT
http://trac.webkit.org/changeset/63305 might have broken Qt Linux Release
Comment 24 Yury Semikhatsky 2010-07-14 06:21:28 PDT
Rolled out in r63314 due to GTK test failure.
Comment 25 Pavel Podivilov 2010-07-27 02:41:47 PDT
Created attachment 62664 [details]
Rebase and add test to gtk skip list.
Comment 26 WebKit Commit Bot 2010-07-27 09:12:44 PDT
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>
Comment 27 WebKit Commit Bot 2010-07-27 09:12:51 PDT
All reviewed patches have been landed.  Closing bug.