Bug 42886 - Web Inspector: implement DOM breakpoints
Summary: Web Inspector: implement DOM breakpoints
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 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-23 03:16 PDT by Pavel Podivilov
Modified: 2010-08-20 06:38 PDT (History)
3 users (show)

See Also:


Attachments
Stop on subtree modifications implementation. (18.90 KB, patch)
2010-07-23 03:45 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Rebase. (18.97 KB, patch)
2010-07-23 05:01 PDT, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Proposed patch. (20.40 KB, patch)
2010-08-17 06:49 PDT, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
All comments addressed. (18.14 KB, patch)
2010-08-18 05:09 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
After offline discussion. (18.49 KB, patch)
2010-08-18 07:27 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Fix nits and debug crash when closing inspector while paused on dom breakpoint. (19.05 KB, patch)
2010-08-19 01:14 PDT, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Proposed patch. (18.40 KB, patch)
2010-08-19 07:17 PDT, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
What's changed from previous patch. (8.71 KB, patch)
2010-08-19 07:18 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Proposed patch. (18.52 KB, patch)
2010-08-20 04:34 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Binary diff for localizedStrings.js (18.67 KB, patch)
2010-08-20 04:44 PDT, Pavel Podivilov
yurys: review-
Details | Formatted Diff | Diff
Proposed patch. (18.67 KB, patch)
2010-08-20 06:12 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Proposed patch. (18.79 KB, patch)
2010-08-20 06:29 PDT, Pavel Podivilov
yurys: review+
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-23 03:16:05 PDT
Implement breaking on DOM mutations feature.
Comment 1 Pavel Podivilov 2010-07-23 03:45:40 PDT
Created attachment 62407 [details]
Stop on subtree modifications implementation.
Comment 2 Pavel Podivilov 2010-07-23 05:01:03 PDT
Created attachment 62416 [details]
Rebase.
Comment 3 Patrick Mueller 2010-07-23 06:33:33 PDT
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.
Comment 4 Pavel Podivilov 2010-07-27 01:58:22 PDT
(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 5 Pavel Feldman 2010-07-27 11:27:05 PDT
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.
Comment 6 Pavel Podivilov 2010-08-17 06:49:25 PDT
Created attachment 64585 [details]
Proposed patch.
Comment 7 Pavel Podivilov 2010-08-17 06:55:36 PDT
(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 8 Pavel Feldman 2010-08-17 12:37:05 PDT
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?
Comment 9 Pavel Podivilov 2010-08-18 05:09:12 PDT
Created attachment 64688 [details]
All comments addressed.
Comment 10 Pavel Podivilov 2010-08-18 07:27:51 PDT
Created attachment 64707 [details]
After offline discussion.
Comment 11 Pavel Feldman 2010-08-18 07:48:37 PDT
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
Comment 12 Pavel Podivilov 2010-08-19 01:14:22 PDT
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 13 Pavel Feldman 2010-08-19 05:22:31 PDT
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 14 Yury Semikhatsky 2010-08-19 06:00:41 PDT
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 15 Yury Semikhatsky 2010-08-19 06:08:20 PDT
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);
Comment 16 Pavel Podivilov 2010-08-19 07:08:53 PDT
(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).
Comment 17 Pavel Podivilov 2010-08-19 07:10:24 PDT
(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.
Comment 18 Pavel Podivilov 2010-08-19 07:17:58 PDT
Created attachment 64846 [details]
Proposed patch.
Comment 19 Pavel Podivilov 2010-08-19 07:18:50 PDT
Created attachment 64847 [details]
What's changed from previous patch.
Comment 20 Yury Semikhatsky 2010-08-20 02:44:59 PDT
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 21 Pavel Feldman 2010-08-20 04:25:25 PDT
Comment on attachment 64846 [details]
Proposed patch.

Looks like Yury meant r-.
Comment 22 Pavel Podivilov 2010-08-20 04:34:09 PDT
Created attachment 64944 [details]
Proposed patch.
Comment 23 Pavel Podivilov 2010-08-20 04:35:15 PDT
(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.
Comment 24 Pavel Podivilov 2010-08-20 04:44:15 PDT
Created attachment 64945 [details]
Binary diff for localizedStrings.js
Comment 25 Yury Semikhatsky 2010-08-20 05:08:16 PDT
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.
Comment 26 Pavel Podivilov 2010-08-20 06:12:49 PDT
Created attachment 64950 [details]
Proposed patch.
Comment 27 Pavel Podivilov 2010-08-20 06:29:11 PDT
(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.
Comment 28 Pavel Podivilov 2010-08-20 06:29:45 PDT
Created attachment 64951 [details]
Proposed patch.
Comment 29 Yury Semikhatsky 2010-08-20 06:38:35 PDT
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