DevTools: injected script per context(WebCore part)

DevTools: injected script per context(WebCore part)

[pfeldman, 2010-01-13 13:35:12]

http://codereview.chromium.org/542055/diff/1/3
File bindings/js/JSInjectedScriptHostCustom.cpp (right):

http://codereview.chromium.org/542055/diff/1/3#newcode91
bindings/js/JSInjectedScriptHostCustom.cpp:91: return
JSInspectedObjectWrapper::wrap(inspectedWindow->globalExec(), toJS(exec,
database));
what happens to this?

http://codereview.chromium.org/542055/diff/1/3#newcode107
bindings/js/JSInjectedScriptHostCustom.cpp:107: return
JSInspectedObjectWrapper::wrap(globalExec, toJS(exec, callFrame));
and this?

http://codereview.chromium.org/542055/diff/1/3#newcode134
bindings/js/JSInjectedScriptHostCustom.cpp:134: JSQuarantinedObjectWrapper*
wrapper = JSQuarantinedObjectWrapper::asWrapper(args.at(0));
and this?

http://codereview.chromium.org/542055/diff/1/3#newcode153
bindings/js/JSInjectedScriptHostCustom.cpp:153: JSQuarantinedObjectWrapper*
wrapper = JSQuarantinedObjectWrapper::asWrapper(args.at(0));
and this?

http://codereview.chromium.org/542055/diff/1/3#newcode173
bindings/js/JSInjectedScriptHostCustom.cpp:173: JSQuarantinedObjectWrapper*
wrapper = JSQuarantinedObjectWrapper::asWrapper(args.at(0));
and this?

http://codereview.chromium.org/542055/diff/1/24
File inspector/ConsoleMessage.h (right):

http://codereview.chromium.org/542055/diff/1/24#newcode67
inspector/ConsoleMessage.h:67: ScriptState* m_scriptState;
why state for the message if all the arguments have it?

http://codereview.chromium.org/542055/diff/1/14
File inspector/InjectedScriptHost.cpp (right):

http://codereview.chromium.org/542055/diff/1/14#newcode169
inspector/InjectedScriptHost.cpp:169: void
InjectedScriptHost::releaseWrapperObjectGroup(long injectedScriptId, const
String& objectGroup)
Did you forget to delete this?

http://codereview.chromium.org/542055/diff/1/14#newcode195
inspector/InjectedScriptHost.cpp:195: void
InjectedScriptHost::releaseWrapperObjectGroup(const ScriptObject&
injectedScript, const String& objectGroup)
Did you forget to delete this?

http://codereview.chromium.org/542055/diff/1/26
File inspector/InspectorBackend.cpp (right):

http://codereview.chromium.org/542055/diff/1/26#newcode263
inspector/InspectorBackend.cpp:263:
m_inspectorController->injectedScriptHost()->setInjectedScriptSource(source);
Does this compile?

http://codereview.chromium.org/542055/diff/1/26#newcode272
inspector/InspectorBackend.cpp:272: bool injectedScriptIdIsNodeId =
injectedScriptId >= 0;
Hm... Lets think of a more elegant solution first.

http://codereview.chromium.org/542055/diff/1/16
File inspector/InspectorBackend.h (right):

http://codereview.chromium.org/542055/diff/1/16#newcode107
inspector/InspectorBackend.h:107: void setInjectedScriptSource(const String&
source);
Nice

http://codereview.chromium.org/542055/diff/1/15
File inspector/InspectorController.cpp (right):

http://codereview.chromium.org/542055/diff/1/15#newcode544
inspector/InspectorController.cpp:544: void
InspectorController::setFrontendProxyObject(ScriptState* scriptState,
ScriptObject webInspectorObj, ScriptObject)
is this for the compatibility or did you forget to remove the argument type?

http://codereview.chromium.org/542055/diff/1/12
File inspector/InspectorFrontend.cpp (right):

http://codereview.chromium.org/542055/diff/1/12#newcode106
inspector/InspectorFrontend.cpp:106: ScriptObject injectedScript =
m_inspectorController->injectedScriptHost()->injectedScriptFor(scriptState);
I'll look at this more carefully. Did you try running the tests?

http://codereview.chromium.org/542055/diff/1/21
File inspector/front-end/InjectedScript.js (right):

http://codereview.chromium.org/542055/diff/1/21#newcode36
inspector/front-end/InjectedScript.js:36: InjectedScript.wrapObject =
function(quarantinedObject, objectGroup) {
{ on next line

http://codereview.chromium.org/542055/diff/1/21#newcode36
inspector/front-end/InjectedScript.js:36: InjectedScript.wrapObject =
function(quarantinedObject, objectGroup) {
why "quarantined"? 
objectGroup -> objectGroupName.

http://codereview.chromium.org/542055/diff/1/21#newcode38
inspector/front-end/InjectedScript.js:38: if (typeof quarantinedObject ==
"object") {
===

http://codereview.chromium.org/542055/diff/1/21#newcode54
inspector/front-end/InjectedScript.js:54: var r =
InjectedScript.wrapObject(quarantinedObject, objectGroup);
wrong indent

http://codereview.chromium.org/542055/diff/1/21#newcode207
inspector/front-end/InjectedScript.js:207: style.__disabledProperties = new
inspectedWindow.Object;
No need to use this constructor anymore. just use = {}.
btw, referencing inspectedWindow by name looks strange. Should you convert all
injected script methods into prototype ones and store inspectedWindow, scriptId
and host instance in "this"?

http://codereview.chromium.org/542055/diff/1/21#newcode237
inspector/front-end/InjectedScript.js:237: InjectedScript.applyStyleRuleText =
function(ruleId, newContent, selectedNodeId)
So why exactly does this call hit the right injected script instance?

http://codereview.chromium.org/542055/diff/1/22
File inspector/front-end/InjectedScriptAccess.js (right):

http://codereview.chromium.org/542055/diff/1/22#newcode52
inspector/front-end/InjectedScriptAccess.js:52: if ("object" == typeof
injectedScriptId) {
===

http://codereview.chromium.org/542055/diff/1/22#newcode53
inspector/front-end/InjectedScriptAccess.js:53: if
(injectedScriptId.injectedScriptId)
Lets talk about this one.

http://codereview.chromium.org/542055/diff/1/18
File inspector/front-end/StylesSidebarPane.js (right):

http://codereview.chromium.org/542055/diff/1/18#newcode695
inspector/front-end/StylesSidebarPane.js:695:
InjectedScriptAccess.applyStyleRuleText(this.rule.id, newContent,
this.pane.node.id, callback);
This won's work, right?

http://codereview.chromium.org/542055/diff/1/18#newcode749
inspector/front-end/StylesSidebarPane.js:749:
InjectedScriptAccess.addStyleSelector(newContent, this.pane.node.id, callback);
ditto

http://codereview.chromium.org/542055/diff/1/18#newcode1056
inspector/front-end/StylesSidebarPane.js:1056:
InjectedScriptAccess.toggleStyleEnabled(this.style.id, this.name, disabled,
callback);
ditto

http://codereview.chromium.org/542055/diff/1/18#newcode1219
inspector/front-end/StylesSidebarPane.js:1219:
InjectedScriptAccess.setStyleText(this.style.id, this.originalCSSText);
ditto

http://codereview.chromium.org/542055/diff/1/18#newcode1239
inspector/front-end/StylesSidebarPane.js:1239:
InjectedScriptAccess.setStyleText(this.style.id, this.originalCSSText);
ditto

http://codereview.chromium.org/542055/diff/1/18#newcode1364
inspector/front-end/StylesSidebarPane.js:1364: var injectedScriptId =
(section.pane.node ? section.pane.node.id : 0);
0 would qualify as the document node of the main document. What if we are
editing iframe's style with no associated node?

http://codereview.chromium.org/542055/diff/1/20
File inspector/front-end/inspector.js (right):

http://codereview.chromium.org/542055/diff/1/20#newcode36
inspector/front-end/inspector.js:36: function debugPrint(text) {
WebInspector.log will do it for you.

[yurys, 2010-01-14 17:35:37]

http://codereview.chromium.org/542055/diff/1/3
File bindings/js/JSInjectedScriptHostCustom.cpp (right):

http://codereview.chromium.org/542055/diff/1/3#newcode91
bindings/js/JSInjectedScriptHostCustom.cpp:91: return
JSInspectedObjectWrapper::wrap(inspectedWindow->globalExec(), toJS(exec,
database));
On 2010/01/13 13:35:12, pfeldman wrote:
> what happens to this?

Removed.

http://codereview.chromium.org/542055/diff/1/3#newcode107
bindings/js/JSInjectedScriptHostCustom.cpp:107: return
JSInspectedObjectWrapper::wrap(globalExec, toJS(exec, callFrame));
On 2010/01/13 13:35:12, pfeldman wrote:
> and this?

Removed.

http://codereview.chromium.org/542055/diff/1/3#newcode134
bindings/js/JSInjectedScriptHostCustom.cpp:134: JSQuarantinedObjectWrapper*
wrapper = JSQuarantinedObjectWrapper::asWrapper(args.at(0));
On 2010/01/13 13:35:12, pfeldman wrote:
> and this?

Removed.

http://codereview.chromium.org/542055/diff/1/3#newcode153
bindings/js/JSInjectedScriptHostCustom.cpp:153: JSQuarantinedObjectWrapper*
wrapper = JSQuarantinedObjectWrapper::asWrapper(args.at(0));
On 2010/01/13 13:35:12, pfeldman wrote:
> and this?

Removed.

http://codereview.chromium.org/542055/diff/1/3#newcode173
bindings/js/JSInjectedScriptHostCustom.cpp:173: JSQuarantinedObjectWrapper*
wrapper = JSQuarantinedObjectWrapper::asWrapper(args.at(0));
On 2010/01/13 13:35:12, pfeldman wrote:
> and this?

Removed.

http://codereview.chromium.org/542055/diff/1/24
File inspector/ConsoleMessage.h (right):

http://codereview.chromium.org/542055/diff/1/24#newcode67
inspector/ConsoleMessage.h:67: ScriptState* m_scriptState;
On 2010/01/13 13:35:12, pfeldman wrote:
> why state for the message if all the arguments have it?

The arguments are SriptValue(not ScriptObjects), none of them have ScriptState.

http://codereview.chromium.org/542055/diff/1/14
File inspector/InjectedScriptHost.cpp (right):

http://codereview.chromium.org/542055/diff/1/14#newcode169
inspector/InjectedScriptHost.cpp:169: void
InjectedScriptHost::releaseWrapperObjectGroup(long injectedScriptId, const
String& objectGroup)
On 2010/01/13 13:35:12, pfeldman wrote:
> Did you forget to delete this?

No. I've added it intentionally. It's called from InspectorBackend.

http://codereview.chromium.org/542055/diff/1/14#newcode195
inspector/InjectedScriptHost.cpp:195: void
InjectedScriptHost::releaseWrapperObjectGroup(const ScriptObject&
injectedScript, const String& objectGroup)
On 2010/01/13 13:35:12, pfeldman wrote:
> Did you forget to delete this?

No.See above.

http://codereview.chromium.org/542055/diff/1/26
File inspector/InspectorBackend.cpp (right):

http://codereview.chromium.org/542055/diff/1/26#newcode263
inspector/InspectorBackend.cpp:263:
m_inspectorController->injectedScriptHost()->setInjectedScriptSource(source);
On 2010/01/13 13:35:12, pfeldman wrote:
> Does this compile?

Of cause it does!

http://codereview.chromium.org/542055/diff/1/26#newcode272
inspector/InspectorBackend.cpp:272: bool injectedScriptIdIsNodeId =
injectedScriptId >= 0;
On 2010/01/13 13:35:12, pfeldman wrote:
> Hm... Lets think of a more elegant solution first.

Yep, front-end code needs more refactoring related to injected script ids.

http://codereview.chromium.org/542055/diff/1/15
File inspector/InspectorController.cpp (right):

http://codereview.chromium.org/542055/diff/1/15#newcode544
inspector/InspectorController.cpp:544: void
InspectorController::setFrontendProxyObject(ScriptState* scriptState,
ScriptObject webInspectorObj, ScriptObject)
On 2010/01/13 13:35:12, pfeldman wrote:
> is this for the compatibility or did you forget to remove the argument type?

It is for compatibility, we need to fix Chromium code first. I've already added
a default value for this parameter for now.

http://codereview.chromium.org/542055/diff/1/12
File inspector/InspectorFrontend.cpp (right):

http://codereview.chromium.org/542055/diff/1/12#newcode106
inspector/InspectorFrontend.cpp:106: ScriptObject injectedScript =
m_inspectorController->injectedScriptHost()->injectedScriptFor(scriptState);
On 2010/01/13 13:35:12, pfeldman wrote:
> I'll look at this more carefully. Did you try running the tests?

Only manual testing so far.

http://codereview.chromium.org/542055/diff/1/21
File inspector/front-end/InjectedScript.js (right):

http://codereview.chromium.org/542055/diff/1/21#newcode36
inspector/front-end/InjectedScript.js:36: InjectedScript.wrapObject =
function(quarantinedObject, objectGroup) {
On 2010/01/13 13:35:12, pfeldman wrote:
> { on next line

Done.

http://codereview.chromium.org/542055/diff/1/21#newcode36
inspector/front-end/InjectedScript.js:36: InjectedScript.wrapObject =
function(quarantinedObject, objectGroup) {
On 2010/01/13 13:35:12, pfeldman wrote:
> why "quarantined"? 
> objectGroup -> objectGroupName.

Done.

http://codereview.chromium.org/542055/diff/1/21#newcode38
inspector/front-end/InjectedScript.js:38: if (typeof quarantinedObject ==
"object") {
On 2010/01/13 13:35:12, pfeldman wrote:
> ===

Done.

http://codereview.chromium.org/542055/diff/1/21#newcode54
inspector/front-end/InjectedScript.js:54: var r =
InjectedScript.wrapObject(quarantinedObject, objectGroup);
On 2010/01/13 13:35:12, pfeldman wrote:
> wrong indent

Done.

http://codereview.chromium.org/542055/diff/1/21#newcode207
inspector/front-end/InjectedScript.js:207: style.__disabledProperties = new
inspectedWindow.Object;
On 2010/01/13 13:35:12, pfeldman wrote:
> No need to use this constructor anymore. just use = {}.
Done.

> btw, referencing inspectedWindow by name looks strange. Should you convert all
> injected script methods into prototype ones and store inspectedWindow,
scriptId
> and host instance in "this"?

I would make this later. The change is already very big.

http://codereview.chromium.org/542055/diff/1/21#newcode237
inspector/front-end/InjectedScript.js:237: InjectedScript.applyStyleRuleText =
function(ruleId, newContent, selectedNodeId)
On 2010/01/13 13:35:12, pfeldman wrote:
> So why exactly does this call hit the right injected script instance?

It seems that I need to add nodeId or better injectedScriptId param like with
InjectedScript.applyStyleText.

http://codereview.chromium.org/542055/diff/1/22
File inspector/front-end/InjectedScriptAccess.js (right):

http://codereview.chromium.org/542055/diff/1/22#newcode52
inspector/front-end/InjectedScriptAccess.js:52: if ("object" == typeof
injectedScriptId) {
On 2010/01/13 13:35:12, pfeldman wrote:
> ===

Done.

http://codereview.chromium.org/542055/diff/1/18
File inspector/front-end/StylesSidebarPane.js (right):

http://codereview.chromium.org/542055/diff/1/18#newcode695
inspector/front-end/StylesSidebarPane.js:695:
InjectedScriptAccess.applyStyleRuleText(this.rule.id, newContent,
this.pane.node.id, callback);
On 2010/01/13 13:35:12, pfeldman wrote:
> This won's work, right?

Done.

http://codereview.chromium.org/542055/diff/1/18#newcode749
inspector/front-end/StylesSidebarPane.js:749:
InjectedScriptAccess.addStyleSelector(newContent, this.pane.node.id, callback);
On 2010/01/13 13:35:12, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/542055/diff/1/18#newcode1056
inspector/front-end/StylesSidebarPane.js:1056:
InjectedScriptAccess.toggleStyleEnabled(this.style.id, this.name, disabled,
callback);
On 2010/01/13 13:35:12, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/542055/diff/1/18#newcode1219
inspector/front-end/StylesSidebarPane.js:1219:
InjectedScriptAccess.setStyleText(this.style.id, this.originalCSSText);
On 2010/01/13 13:35:12, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/542055/diff/1/18#newcode1239
inspector/front-end/StylesSidebarPane.js:1239:
InjectedScriptAccess.setStyleText(this.style.id, this.originalCSSText);
On 2010/01/13 13:35:12, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/542055/diff/1/18#newcode1364
inspector/front-end/StylesSidebarPane.js:1364: var injectedScriptId =
(section.pane.node ? section.pane.node.id : 0);
On 2010/01/13 13:35:12, pfeldman wrote:
> 0 would qualify as the document node of the main document. What if we are
> editing iframe's style with no associated node?

Could you elaborate on this? We may want to address orphan iframes in a specific
way.

http://codereview.chromium.org/542055/diff/1/20
File inspector/front-end/inspector.js (right):

http://codereview.chromium.org/542055/diff/1/20#newcode36
inspector/front-end/inspector.js:36: function debugPrint(text) {
On 2010/01/13 13:35:12, pfeldman wrote:
> WebInspector.log will do it for you.

Done.

[pfeldman, 2010-01-14 18:59:02]

http://codereview.chromium.org/542055/diff/4001/5013
File inspector/InjectedScriptHost.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5013#newcode183
inspector/InjectedScriptHost.cpp:183: ScriptObject injectedScript =
m_idToInjectedScript.get(injectedScriptId);
check for 0 - it is asynchronous.

http://codereview.chromium.org/542055/diff/4001/5025
File inspector/InspectorBackend.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5025#newcode272
inspector/InspectorBackend.cpp:272: bool injectedScriptIdIsNodeId =
injectedScriptId <= 0;
FIXME?

http://codereview.chromium.org/542055/diff/4001/5014
File inspector/InspectorController.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5014#newcode1867
inspector/InspectorController.cpp:1867: ASSERT(false);
you hit this due to async nature of stuff too.

http://codereview.chromium.org/542055/diff/4001/5011
File inspector/InspectorFrontend.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5011#newcode108
inspector/InspectorFrontend.cpp:108: ScriptFunctionCall
wrapFunction(scriptState, injectedScript, "wrapAndStringifyObject");
I really don't like double-json.

http://codereview.chromium.org/542055/diff/4001/5022
File inspector/InspectorFrontend.h (right):

http://codereview.chromium.org/542055/diff/4001/5022#newcode62
inspector/InspectorFrontend.h:62: ScriptArray newScriptArray();
We should enforce typing here and introduce FrontendArray/Object so that
inspected objects don't get into malicious context.

http://codereview.chromium.org/542055/diff/4001/5032
File inspector/front-end/ConsoleView.js (right):

http://codereview.chromium.org/542055/diff/4001/5032#newcode540
inspector/front-end/ConsoleView.js:540:
InjectedScriptAccess.pushNodeToFrontend(object.injectedScriptId, object,
printNode);
InjectedScriptAccess has too many parameters in calls already. I think we should
use following notation instead:

InjectedScriptAccess.get(object.injectedScriptId).pushNodeToFrontend(object,
printNode); That way you don't force clients to have injected script id as the
first argument of the method.

http://codereview.chromium.org/542055/diff/4001/5039
File inspector/front-end/Database.js (right):

http://codereview.chromium.org/542055/diff/4001/5039#newcode98
inspector/front-end/Database.js:98: InjectedScriptAccess.executeSql(0 /* For now
use main frame injected script. */, this._id, query, callback);
FIXME?

http://codereview.chromium.org/542055/diff/4001/5038
File inspector/front-end/ElementsPanel.js (right):

http://codereview.chromium.org/542055/diff/4001/5038#newcode239
inspector/front-end/ElementsPanel.js:239: InjectedScriptAccess.nodeByPath(0 /*
main frame injected script id */, this._selectedPathOnReset,
selectLastSelectedNode.bind(this));
InjectedScriptAccess.getDefault() here and below.

http://codereview.chromium.org/542055/diff/4001/5020
File inspector/front-end/InjectedScript.js (right):

http://codereview.chromium.org/542055/diff/4001/5020#newcode913
inspector/front-end/InjectedScript.js:913: return "";
it was a convention - false means "i failed".

http://codereview.chromium.org/542055/diff/4001/5021
File inspector/front-end/InjectedScriptAccess.js (right):

http://codereview.chromium.org/542055/diff/4001/5021#newcode52
inspector/front-end/InjectedScriptAccess.js:52: WebInspector.log(methodName + '
' + injectedScriptId + ' ' + (typeof injectedScriptId));
remove log

http://codereview.chromium.org/542055/diff/4001/5019
File inspector/front-end/inspector.js (right):

http://codereview.chromium.org/542055/diff/4001/5019#newcode396
inspector/front-end/inspector.js:396:
InspectorBackend.setInjectedScriptSource(injectedScriptSource.join(''));
do not use single quotes.
"(" + injectedScriptConstructor + ")"

http://codereview.chromium.org/542055/diff/4001/5019#newcode1156
inspector/front-end/inspector.js:1156: callFrames = JSON.parse(callFrames);
Don't like this either.

http://codereview.chromium.org/542055/diff/4001/5019#newcode1225
inspector/front-end/inspector.js:1225:
parsedArguments.push(JSON.parse(arguments[i]));
Should not need this - FrontendObject in WebKit, dispatch via parse in Chromium
should make us safe.

[yurys, 2010-01-18 09:35:13]

http://codereview.chromium.org/542055/diff/4001/5013
File inspector/InjectedScriptHost.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5013#newcode183
inspector/InjectedScriptHost.cpp:183: ScriptObject injectedScript =
m_idToInjectedScript.get(injectedScriptId);
On 2010/01/14 18:59:03, pfeldman wrote:
> check for 0 - it is asynchronous.

Done.

http://codereview.chromium.org/542055/diff/4001/5025
File inspector/InspectorBackend.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5025#newcode272
inspector/InspectorBackend.cpp:272: bool injectedScriptIdIsNodeId =
injectedScriptId <= 0;
On 2010/01/14 18:59:03, pfeldman wrote:
> FIXME?

Done.

http://codereview.chromium.org/542055/diff/4001/5014
File inspector/InspectorController.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5014#newcode1867
inspector/InspectorController.cpp:1867: ASSERT(false);
On 2010/01/14 18:59:03, pfeldman wrote:
> you hit this due to async nature of stuff too.

Done.

http://codereview.chromium.org/542055/diff/4001/5032
File inspector/front-end/ConsoleView.js (right):

http://codereview.chromium.org/542055/diff/4001/5032#newcode540
inspector/front-end/ConsoleView.js:540:
InjectedScriptAccess.pushNodeToFrontend(object.injectedScriptId, object,
printNode);
On 2010/01/14 18:59:03, pfeldman wrote:
> InjectedScriptAccess has too many parameters in calls already. I think we
should
> use following notation instead:
> 
> InjectedScriptAccess.get(object.injectedScriptId).pushNodeToFrontend(object,
> printNode); That way you don't force clients to have injected script id as the
> first argument of the method.

Done.

http://codereview.chromium.org/542055/diff/4001/5039
File inspector/front-end/Database.js (right):

http://codereview.chromium.org/542055/diff/4001/5039#newcode98
inspector/front-end/Database.js:98: InjectedScriptAccess.executeSql(0 /* For now
use main frame injected script. */, this._id, query, callback);
On 2010/01/14 18:59:03, pfeldman wrote:
> FIXME?

Done.

http://codereview.chromium.org/542055/diff/4001/5038
File inspector/front-end/ElementsPanel.js (right):

http://codereview.chromium.org/542055/diff/4001/5038#newcode239
inspector/front-end/ElementsPanel.js:239: InjectedScriptAccess.nodeByPath(0 /*
main frame injected script id */, this._selectedPathOnReset,
selectLastSelectedNode.bind(this));
On 2010/01/14 18:59:03, pfeldman wrote:
> InjectedScriptAccess.getDefault() here and below.

Done.

http://codereview.chromium.org/542055/diff/4001/5020
File inspector/front-end/InjectedScript.js (right):

http://codereview.chromium.org/542055/diff/4001/5020#newcode913
inspector/front-end/InjectedScript.js:913: return "";
On 2010/01/14 18:59:03, pfeldman wrote:
> it was a convention - false means "i failed".

Reverted. Would be nice to document this unobvious convention, otherwise it
looks like a bug.

http://codereview.chromium.org/542055/diff/4001/5021
File inspector/front-end/InjectedScriptAccess.js (right):

http://codereview.chromium.org/542055/diff/4001/5021#newcode52
inspector/front-end/InjectedScriptAccess.js:52: WebInspector.log(methodName + '
' + injectedScriptId + ' ' + (typeof injectedScriptId));
On 2010/01/14 18:59:03, pfeldman wrote:
> remove log

Done.

http://codereview.chromium.org/542055/diff/4001/5019
File inspector/front-end/inspector.js (right):

http://codereview.chromium.org/542055/diff/4001/5019#newcode396
inspector/front-end/inspector.js:396:
InspectorBackend.setInjectedScriptSource(injectedScriptSource.join(''));
On 2010/01/14 18:59:03, pfeldman wrote:
> do not use single quotes.
> "(" + injectedScriptConstructor + ")"

Done.

http://codereview.chromium.org/542055/diff/4001/5019#newcode1156
inspector/front-end/inspector.js:1156: callFrames = JSON.parse(callFrames);
On 2010/01/14 18:59:03, pfeldman wrote:
> Don't like this either.

This will disappear once we switch to SerializedScriptValue.