DevTools: handle 'scripts' response only when context id is known

DevTools: scripts request is now sent only when context id is received(if we send scripts request right after context id request there is no guarantee that their responses will come in the same order).

BUG=19858
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25435

[apavlov, 2009-09-03 18:54:19]

LGTM

http://codereview.chromium.org/193006/diff/1/3
File webkit/glue/devtools/js/debugger_agent.js (right):

http://codereview.chromium.org/193006/diff/1/3#newcode724
Line 724: // If it's a first time context id is set request scripts list.
a first time -> the first time

http://codereview.chromium.org/193006/diff/1/3#newcode929
Line 929: var voidScript = devtools.DebuggerAgent.VOID_SCRIPT_EVAL_SOURCE;
interesting... did this change recently? How reliable is this check? I mean, a
single space can be added/deleted on the V8 side. Not that I'm against this
method...

[yurys, 2009-09-03 19:20:23]

http://codereview.chromium.org/193006/diff/1/3
File webkit/glue/devtools/js/debugger_agent.js (right):

http://codereview.chromium.org/193006/diff/1/3#newcode724
Line 724: // If it's a first time context id is set request scripts list.
On 2009/09/03 18:54:19, apavlov wrote:
> a first time -> the first time

Done. Thanks.

http://codereview.chromium.org/193006/diff/1/3#newcode929
Line 929: var voidScript = devtools.DebuggerAgent.VOID_SCRIPT_EVAL_SOURCE;
On 2009/09/03 18:54:19, apavlov wrote:
> interesting... did this change recently? How reliable is this check? I mean, a
> single space can be added/deleted on the V8 side. Not that I'm against this
> method...
It changed quite quite recently when JS eval behavior changed. It is
InjectedScript.evaluate that wraps javascript:void(0) in with statements to
extend its context with console API. And you're right we need to come up with a
more reliable method of detecting void scripts or even better get rid of those
scripts.

[pfeldman, 2009-09-03 20:50:26]

not LGTM?

http://codereview.chromium.org/193006/diff/6001/6002
File webkit/glue/devtools/debugger_agent_manager.cc (right):

http://codereview.chromium.org/193006/diff/6001/6002#newcode221
Line 221: // If the context is from one of the inpected tabs or injected
extension
Is this change intentional? Now we are getting here if we are _in_ the utility
context and we are hitting a non-break event such as after compile. In other
words evaluating things in console will be adding script into the combobox.

http://codereview.chromium.org/193006/diff/6001/6003
File webkit/glue/devtools/js/debugger_agent.js (right):

http://codereview.chromium.org/193006/diff/6001/6003#newcode169
Line 169: // There can be a number of scripts from after-compile events that are
Could you add a comment saying that this method is called _every time_ scripts
tab is selected?

http://codereview.chromium.org/193006/diff/6001/6003#newcode190
Line 190: this.requestScriptsWhenContextIdSet_ = true;
It seems like there is a strong correlation between
this.requestScriptsWhenContextIdSet_ and this.contextId. Do we clear latter?
Could we set it to -1 instead? In that case this.requestScriptsWhenContextIdSet_
== !this.contextId.

http://codereview.chromium.org/193006/diff/6001/6003#newcode773
Line 773: // At first try to invoke custom response handler and if it fails call
the
This looks (and looked) like a hack. Can we route 'scripts' response to the
call-back-for-response in all the cases and bind this.handleScriptsResponse to
the initial 'scripts' request instead? that would be much less hacky.

[yurys, 2009-09-04 07:15:14]

http://codereview.chromium.org/193006/diff/6001/6002
File webkit/glue/devtools/debugger_agent_manager.cc (right):

http://codereview.chromium.org/193006/diff/6001/6002#newcode221
Line 221: // If the context is from one of the inpected tabs or injected
extension
On 2009/09/03 20:50:26, pfeldman wrote:
> Is this change intentional? Now we are getting here if we are _in_ the utility
> context and we are hitting a non-break event such as after compile. In other
> words evaluating things in console will be adding script into the combobox.
Exactly. And this is intentional behavior. WebKit already behaves this way. If
after compile events are not propagated to the FE we may never see console API
set to the inspected window. Also console evaluations may create new functions
in the inspected window which we want to be able to debug and therefor we need
to send after compile event for the eval to notify FE about the new script.

http://codereview.chromium.org/193006/diff/6001/6003
File webkit/glue/devtools/js/debugger_agent.js (right):

http://codereview.chromium.org/193006/diff/6001/6003#newcode169
Line 169: // There can be a number of scripts from after-compile events that are
On 2009/09/03 20:50:26, pfeldman wrote:
> Could you add a comment saying that this method is called _every time_ scripts
> tab is selected?

Done.

http://codereview.chromium.org/193006/diff/6001/6003#newcode190
Line 190: this.requestScriptsWhenContextIdSet_ = true;
On 2009/09/03 20:50:26, pfeldman wrote:
> It seems like there is a strong correlation between
> this.requestScriptsWhenContextIdSet_ and this.contextId. Do we clear latter?
> Could we set it to -1 instead? In that case
this.requestScriptsWhenContextIdSet_
> == !this.contextId.

Do you want to introduce second "undefined" state for contextId_ so that it is
null when FE is started and set to -1 every time DebuggerAgent.reset is called?
In this case
this.requestScriptsWhenContextIdSet_ == (this.contextId_ == null) not
this.requestScriptsWhenContextIdSet_ == !this.contextId_.
It doesn't look more clear to me.

http://codereview.chromium.org/193006/diff/6001/6003#newcode773
Line 773: // At first try to invoke custom response handler and if it fails call
the
On 2009/09/03 20:50:26, pfeldman wrote:
> This looks (and looked) like a hack. Can we route 'scripts' response to the
> call-back-for-response in all the cases and bind this.handleScriptsResponse to
> the initial 'scripts' request instead? that would be much less hacky.

Done.

[pfeldman, 2009-09-04 07:45:15]

LGTM