Summary: | Web Inspector: implement DOM breakpoints | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Podivilov <podivilov> | ||||||||||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Enhancement | CC: | pfeldman, pmuellr, yurys | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Pavel Podivilov
2010-07-23 03:16:05 PDT
Created attachment 62407 [details]
Stop on subtree modifications implementation.
Created attachment 62416 [details]
Rebase.
neat. So how does this work, visually? Will the DOM node be highlighted in some way to indicate it has a breakpoint set on it? It didn't look like, from the code, that the breakpoint gets added to the breakpoints list, but maybe I missed that. Seems like it would be nice, so that you could remove the breakpoints from there. It doesn't appear that attribute changes will cause a breakpoint to be hit. I imagine that might be useful as well. (In reply to comment #3) > neat. > > So how does this work, visually? Will the DOM node be highlighted in some way to indicate it has a breakpoint set on it? It didn't look like, from the code, that the breakpoint gets added to the breakpoints list, but maybe I missed that. Seems like it would be nice, so that you could remove the breakpoints from there. > > It doesn't appear that attribute changes will cause a breakpoint to be hit. I imagine that might be useful as well. It's just a first bit of functionality, and UI part isn't there yet. Attribute changes will be covered by a separate breakpoint type (coming soon). Comment on attachment 62416 [details]
Rebase.
Few nits below. Otherwise, looks very good!
WebCore/bindings/js/ScriptDebugServer.h:82
+ void breakProgram() { }
Please file a bug against WebKit / JSC and put a FIXME referring the bug here.
WebCore/inspector/InspectorBackend.idl:109
+ void setDOMBreakpoint(in long nodeId, in long type);
Please rebase, this file no longer exists.
WebCore/inspector/InspectorDOMAgent.cpp:1027
+ #if ENABLE(JAVASCRIPT_DEBUGGER)
I think you need to optimize this:
1) do not enter this code when there are no dom breakpoints
2) when setting breakpoint on node, set it on all of its parents instead. that way you spend time on setting breakpoint, not on dom events
WebCore/inspector/InspectorDOMAgent.cpp:761
+ void InspectorDOMAgent::setDOMBreakpoint(long nodeId, long type)
type should be enum, synchronized with corresponding js enum.
Created attachment 64585 [details]
Proposed patch.
(In reply to comment #5) > (From update of attachment 62416 [details]) > Few nits below. Otherwise, looks very good! > > WebCore/bindings/js/ScriptDebugServer.h:82 > + void breakProgram() { } > Please file a bug against WebKit / JSC and put a FIXME referring the bug here. Done > WebCore/inspector/InspectorBackend.idl:109 > + void setDOMBreakpoint(in long nodeId, in long type); > Please rebase, this file no longer exists. Done > WebCore/inspector/InspectorDOMAgent.cpp:1027 > + #if ENABLE(JAVASCRIPT_DEBUGGER) > I think you need to optimize this: > 1) do not enter this code when there are no dom breakpoints > 2) when setting breakpoint on node, set it on all of its parents instead. that way you spend time on setting breakpoint, not on dom events Done > WebCore/inspector/InspectorDOMAgent.cpp:761 > + void InspectorDOMAgent::setDOMBreakpoint(long nodeId, long type) > type should be enum, synchronized with corresponding js enum. This method is called from generated code, so we can't have enum here. Comment on attachment 64585 [details]
Proposed patch.
WebCore/bindings/js/ScriptDebugServer.cpp:210
+ // FIXME(43332): implement this.
You can mention webkit bugzilla here so that we could distinguish this from radar / chromium entries.
WebCore/inspector/InspectorDOMAgent.cpp:742
+ Node* node = nodeForId(nodeId);
Move this check above? (This should not happen).
WebCore/inspector/front-end/ElementsTreeOutline.js:767
+ contextMenu.appendItem(WebInspector.UIString("Stop on subtree modifications"),
You should add these to the WebCore/English.lproj/localizedStrings.js
WebCore/inspector/front-end/BreakpointManager.js:93
+ setDOMBreakpoint: function(nodeId, type)
Nit: I think these should be declared on DOMNode's prototype.
WebCore/inspector/InspectorDOMAgent.cpp:1075
+ Vector<Node*> stack(1, innerFirstChild(node));
You have 4 places where you traverse the tree. Could it be extracted into a separate method? A part of r- is for this.
WebCore/inspector/InspectorDOMAgent.cpp:204
+ enum CachedDOMBreakpointType {
I am not sure why you need this separation? r- unless this is explained.
WebCore/inspector/InspectorDOMAgent.cpp:736
+ void InspectorDOMAgent::setDOMBreakpoint(long nodeId, long type)
So what happens when you add a breakpoint to the node, then add it to its parent and after that remove it from node?
Created attachment 64688 [details]
All comments addressed.
Created attachment 64707 [details]
After offline discussion.
Comment on attachment 64707 [details]
After offline discussion.
WebCore/inspector/InspectorDOMAgent.cpp:1039
+ void InspectorDOMAgent::updateSubtreeBreakpoints(Node* root, long mask, bool value, bool wholeSubtree)
Nit:
wholeSubtree -> recursive
value -> set
Created attachment 64818 [details]
Fix nits and debug crash when closing inspector while paused on dom breakpoint.
Before pausing on DOM breakpoints save current instance to a static variable which will be cleared in destructor to indicate that InspectorDOMAgent was deleted.
Comment on attachment 64818 [details]
Fix nits and debug crash when closing inspector while paused on dom breakpoint.
I don't think cq will land a binary git diff, so will need to land it by hand...
WebCore/inspector/InspectorDOMAgent.cpp:208
+ InspectorDOMAgent* InspectorDOMAgent::s_instance = 0;
You should pick a better name for this field.
WebCore/inspector/InspectorDOMAgent.cpp:1046
+ ScriptDebugServer::shared().breakProgram();
I think you should set s_instance (or ::s_domAgentOnBreakpoint) here and make this method return boolean.
Comment on attachment 64818 [details]
Fix nits and debug crash when closing inspector while paused on dom breakpoint.
WebCore/bindings/v8/ScriptDebugServer.cpp:368
+ ScriptState* currentCallFrameState = mainWorldScriptState(frame);
This should be ScriptState::forContext where context is the one that was current before call to the debugger. Otherwise, if it wasn't a javascript code that triggered the mutation the stack would be empty and you could use main world script state.
WebCore/bindings/v8/ScriptDebugServer.h:135
+ OwnHandle<v8::Context> m_pausedPageContext;
This one may be Local handle
WebCore/inspector/InspectorDOMAgent.cpp:744
+ m_breakpoints.set(node, m_breakpoints.get(node) | rootBit & ~derivedBit);
We discussed these bits with Pavel and it turns out that they don't need to be mutually exclusive. There should rather be two bits:
1. Indicates whether this node has a breakpoint set on it.
2. Whether we should pause on modifications of this node(currently it's deivedBit).
So I'd suggest we rename them and don't reset derivedBit when setting breakpoint.
Comment on attachment 64818 [details]
Fix nits and debug crash when closing inspector while paused on dom breakpoint.
WebCore/inspector/InspectorDOMAgent.cpp:764
+ m_breakpoints.set(node, mask & ~rootBit);
Map entry for the node should be removed when its values becomes 0.
WebCore/inspector/InspectorDOMAgent.cpp:1061
+ m_breakpoints.set(node, derivedMask);
This will override previous mask for given node. It should be m_breakpoints.set(node, m_breakpoints.get(node) | derivedMask);
(In reply to comment #14) > (From update of attachment 64818 [details]) > WebCore/bindings/v8/ScriptDebugServer.cpp:368 > + ScriptState* currentCallFrameState = mainWorldScriptState(frame); > This should be ScriptState::forContext where context is the one that was current before call to the debugger. Otherwise, if it wasn't a javascript code that triggered the mutation the stack would be empty and you could use main world script state. done > WebCore/bindings/v8/ScriptDebugServer.h:135 > + OwnHandle<v8::Context> m_pausedPageContext; > This one may be Local handle done > WebCore/inspector/InspectorDOMAgent.cpp:744 > + m_breakpoints.set(node, m_breakpoints.get(node) | rootBit & ~derivedBit); > We discussed these bits with Pavel and it turns out that they don't need to be mutually exclusive. There should rather be two bits: > 1. Indicates whether this node has a breakpoint set on it. > 2. Whether we should pause on modifications of this node(currently it's deivedBit). > So I'd suggest we rename them and don't reset derivedBit when setting breakpoint. Probably it's more logical to use second bit to indicate if we have a derived breakpoint (there is a breakpoint set on ancestor). This will simplify breakpoint deletion (no need to check parent). (In reply to comment #15) > (From update of attachment 64818 [details]) > WebCore/inspector/InspectorDOMAgent.cpp:764 > + m_breakpoints.set(node, mask & ~rootBit); > Map entry for the node should be removed when its values becomes 0. done > WebCore/inspector/InspectorDOMAgent.cpp:1061 > + m_breakpoints.set(node, derivedMask); > This will override previous mask for given node. It should be m_breakpoints.set(node, m_breakpoints.get(node) | derivedMask); This was done intentionally, because we use recursive mode only when adding subtrees, and new subtrees are always clean. Created attachment 64846 [details]
Proposed patch.
Created attachment 64847 [details]
What's changed from previous patch.
Comment on attachment 64846 [details]
Proposed patch.
WebCore/inspector/InspectorDOMAgent.cpp:1061
+ m_breakpoints.set(node, derivedMask);
I still don't like that we implicitly assume here that the node is one that has just been added and has no current flags. Let's make the code work for all nodes including those that already have non-0 mask.
WebCore/inspector/InspectorDOMAgent.cpp:1050
+ void InspectorDOMAgent::updateSubtreeBreakpoints(Node* root, long mask, bool set, bool recursive)
Please remove recursive flag as discussed offline and make the function always recursive.
Comment on attachment 64846 [details]
Proposed patch.
Looks like Yury meant r-.
Created attachment 64944 [details]
Proposed patch.
(In reply to comment #20) > (From update of attachment 64846 [details]) > WebCore/inspector/InspectorDOMAgent.cpp:1061 > + m_breakpoints.set(node, derivedMask); > I still don't like that we implicitly assume here that the node is one that has just been added and has no current flags. Let's make the code work for all nodes including those that already have non-0 mask. > > WebCore/inspector/InspectorDOMAgent.cpp:1050 > + void InspectorDOMAgent::updateSubtreeBreakpoints(Node* root, long mask, bool set, bool recursive) > Please remove recursive flag as discussed offline and make the function always recursive. updateSubtreeBreakpoints rewritten as discussed offline. Created attachment 64945 [details]
Binary diff for localizedStrings.js
Comment on attachment 64945 [details]
Binary diff for localizedStrings.js
WebCore/inspector/InspectorDOMAgent.cpp:1066
+ if (!node)
You don't seem to need this check.
WebCore/inspector/InspectorDOMAgent.cpp:1064
+ void InspectorDOMAgent::updateSubtreeBreakpoints(Node* node, long mask, bool set)
I'd rename mask to rootMask or something that would reflect the fact that it's not shifted.
WebCore/inspector/InspectorDOMAgent.h:218
+ HashMap<Node*, long> m_breakpoints;
Why not have int as value type? We use only 32 bits in the code anyway.
WebCore/inspector/InspectorDOMAgent.cpp:1060
+ return s_domAgentOnBreakpoint != 0;
Please reset s_domAgentOnBreakpoint to 0 before leaving this method.
WebCore/bindings/v8/ScriptDebugServer.cpp:243
+ v8::Handle<v8::Context> context = v8::Context::GetCurrent();
Isn't it Debugger context?
Pleas address the comments, otherwise looks good.
Created attachment 64950 [details]
Proposed patch.
(In reply to comment #25) > (From update of attachment 64945 [details]) > WebCore/inspector/InspectorDOMAgent.cpp:1066 > + if (!node) > You don't seem to need this check. done > WebCore/inspector/InspectorDOMAgent.cpp:1064 > + void InspectorDOMAgent::updateSubtreeBreakpoints(Node* node, long mask, bool set) > I'd rename mask to rootMask or something that would reflect the fact that it's not shifted. done > WebCore/inspector/InspectorDOMAgent.h:218 > + HashMap<Node*, long> m_breakpoints; > Why not have int as value type? We use only 32 bits in the code anyway. For both int and long types size is not guaranteed long -> uint32_t > WebCore/inspector/InspectorDOMAgent.cpp:1060 > + return s_domAgentOnBreakpoint != 0; > Please reset s_domAgentOnBreakpoint to 0 before leaving this method. done > WebCore/bindings/v8/ScriptDebugServer.cpp:243 > + v8::Handle<v8::Context> context = v8::Context::GetCurrent(); > Isn't it Debugger context? This method is called from javascript callbacks. Created attachment 64951 [details]
Proposed patch.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/js/ScriptDebugServer.cpp M WebCore/bindings/js/ScriptDebugServer.h M WebCore/bindings/v8/ScriptDebugServer.cpp M WebCore/bindings/v8/ScriptDebugServer.h M WebCore/inspector/Inspector.idl M WebCore/inspector/InspectorDOMAgent.cpp M WebCore/inspector/InspectorDOMAgent.h M WebCore/inspector/front-end/ElementsTreeOutline.js M WebCore/inspector/front-end/Settings.js M WebKit/chromium/ChangeLog M WebKit/chromium/src/js/DevTools.js Committed r65731 |