Bug 42886 – Web Inspector: implement DOM breakpoints

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

[Pavel Podivilov]

Implement breaking on DOM mutations feature.

[Pavel Podivilov]

Created attachment 62407 [details]
Stop on subtree modifications implementation.

[Pavel Podivilov]

Created attachment 62416 [details]
Rebase.

[Patrick Mueller]

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.

[Pavel Podivilov]

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

[Pavel Feldman]

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.

[Pavel Podivilov]

Created attachment 64585 [details]
Proposed patch.

[Pavel Podivilov]

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

[Pavel Feldman]

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?

[Pavel Podivilov]

Created attachment 64688 [details]
All comments addressed.

[Pavel Podivilov]

Created attachment 64707 [details]
After offline discussion.

[Pavel Feldman]

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

[Pavel Podivilov]

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.

[Pavel Feldman]

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.

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

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);

[Pavel Podivilov]

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

[Pavel Podivilov]

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

[Pavel Podivilov]

Created attachment 64846 [details]
Proposed patch.

[Pavel Podivilov]

Created attachment 64847 [details]
What's changed from previous patch.

[Yury Semikhatsky]

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.

[Pavel Feldman]

Comment on attachment 64846 [details]
Proposed patch.

Looks like Yury meant r-.

[Pavel Podivilov]

Created attachment 64944 [details]
Proposed patch.

[Pavel Podivilov]

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

[Pavel Podivilov]

Created attachment 64945 [details]
Binary diff for localizedStrings.js

[Yury Semikhatsky]

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.

[Pavel Podivilov]

Created attachment 64950 [details]
Proposed patch.

[Pavel Podivilov]

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

[Pavel Podivilov]

Created attachment 64951 [details]
Proposed patch.

[Yury Semikhatsky]

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