Bug 44679 – Web Inspector: provide more information to front-end when breaking on DOM event

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

[Pavel Podivilov]

Web Inspector: provide more information to front-end when breaking on DOM event

[Pavel Podivilov]

Created attachment 65564 [details]
Proposed patch.

[Pavel Feldman]

Comment on attachment 65564 [details]
Proposed patch.

Please run Dromaeo tests before and after the change to prove that it does not regress performance.

WebCore/dom/ContainerNode.cpp:137
 +  #if ENABLE(INSPECTOR)
These deserve inline function definitions. I'd do it on inspector controller in order to hide the complexity too:
InspectorController::willInsertDOMNode(this, child);
InspectorController::didInsertDOMNode(this, child);
InspectorController::willRemoveDOMNode(this, child);

[Pavel Podivilov]

Created attachment 65705 [details]
Proposed patch.

[Early Warning System Bot]

Attachment 65705 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3853016

[Pavel Podivilov]

Created attachment 65707 [details]
Proposed patch.

[Pavel Feldman]

Comment on attachment 65707 [details]
Proposed patch.

WebCore/dom/ContainerNode.cpp:137
 +          InspectorController::willInsertDOMNode(this);
Passing child would look more consistent.

WebCore/inspector/InspectorController.h:387
 +      if (Page* page = node->document()->page()) {
Please replace nested ifs with guard expressions when nesting is that big.

WebCore/inspector/InspectorController.h:416
 +      if (Page* page = node->document()->page()) {
ditto

[Pavel Podivilov]

Created attachment 65909 [details]
Proposed patch.

[Yury Semikhatsky]

Comment on attachment 65909 [details]
Proposed patch.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 908f761..925163a 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,59 @@
> [phone]  Pavel Podivilov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Web Inspector: provide more information to front-end when breaking on DOM event
> +        https://bugs.webkit.org/show_bug.cgi?id=44679
> +
> +        * dom/ContainerNode.cpp:
> +        (WebCore::ContainerNode::insertBefore):
> +        (WebCore::ContainerNode::parserInsertBefore):
> +        (WebCore::ContainerNode::replaceChild):
> +        (WebCore::ContainerNode::appendChild):
> +        (WebCore::ContainerNode::parserAddChild):
> +        (WebCore::notifyChildInserted):
> +        (WebCore::dispatchChildRemovalEvents):
> +        * dom/Element.cpp:
> +        (WebCore::Element::setAttribute):
> +        (WebCore::Element::removeAttribute):
> +        * inspector/Inspector.idl:
> +        * inspector/InspectorController.h:
> +        (WebCore::InspectorController::willInsertDOMNode):
> +        (WebCore::InspectorController::didInsertDOMNode):
> +        (WebCore::InspectorController::willRemoveDOMNode):
> +        (WebCore::InspectorController::willModifyDOMAttr):
> +        (WebCore::InspectorController::didModifyDOMAttr):
> +        (WebCore::InspectorController::inspectorControllerForNode):
> +        * inspector/InspectorDOMAgent.cpp:
> +        (WebCore::InspectorDOMAgent::~InspectorDOMAgent):
> +        (WebCore::InspectorDOMAgent::getBreakpointForNodeInsertion):
> +        (WebCore::InspectorDOMAgent::getBreakpointForNodeRemoval):
> +        (WebCore::InspectorDOMAgent::getBreakpointForAttributeModification):
> +        (WebCore::InspectorDOMAgent::didInsertDOMNode):
> +        (WebCore::InspectorDOMAgent::didRemoveDOMNode):
> +        (WebCore::InspectorDOMAgent::didModifyDOMAttr):
> +        (WebCore::InspectorDOMAgent::createBreakpoint):
> +        * inspector/InspectorDOMAgent.h:
> +        * inspector/InspectorDebuggerAgent.cpp:
> +        (WebCore::InspectorDebuggerAgent::InspectorDebuggerAgent):
> +        (WebCore::InspectorDebuggerAgent::~InspectorDebuggerAgent):
> +        (WebCore::InspectorDebuggerAgent::didPause):
> +        (WebCore::InspectorDebuggerAgent::breakProgram):
> +        * inspector/InspectorDebuggerAgent.h:
> +        * inspector/InspectorValues.h:
> +        (WebCore::InspectorValue::isNull):
> +        * inspector/front-end/BreakpointsSidebarPane.js:
> +        (WebInspector.BreakpointItem):
> +        (WebInspector.BreakpointItem.prototype._enableChanged):
> +        * inspector/front-end/DOMAgent.js:
> +        (WebInspector.DOMAgent.prototype._childNodeRemoved):
> +        (WebInspector.DOMBreakpointManager.prototype._breakpointRemoved):
> +        (WebInspector.DOMBreakpointManager.prototype.breakpointHit):
> +        * inspector/front-end/Script.js:
> +        (WebInspector.Script.prototype.get linesCount):
> +        * inspector/front-end/inspector.js:
> +        (WebInspector.pausedScript):
> +
>  2010-08-28  Jeremy Moskovich  <[e-mail]>
>  
>          Reviewed by Dimitri Glazkov.
> diff --git a/WebCore/dom/ContainerNode.cpp b/WebCore/dom/ContainerNode.cpp
> index ef62b38..7d88948 100644
> --- a/WebCore/dom/ContainerNode.cpp
> +++ b/WebCore/dom/ContainerNode.cpp
> @@ -134,6 +134,8 @@ bool ContainerNode::insertBefore(PassRefPtr<Node> newChild, Node* refChild, Exce
>          if (child->parentNode())
>              break;
>  
> +        InspectorController::willInsertDOMNode(child, this);
> +
>          insertBeforeCommon(next.get(), child);
>  
>          // Send notification about the children change.
> @@ -201,6 +203,8 @@ void ContainerNode::parserInsertBefore(PassRefPtr<Node> newChild, Node* nextChil
>      for (NodeVector::const_iterator it = targets.begin(); it != targets.end(); ++it) {
>          Node* child = it->get();
>  
> +        InspectorController::willInsertDOMNode(child, this);
> +
>          insertBeforeCommon(next.get(), child);
>  
>          childrenChanged(true, nextChildPreviousSibling.get(), nextChild, 1);
> @@ -274,6 +278,8 @@ bool ContainerNode::replaceChild(PassRefPtr<Node> newChild, Node* oldChild, Exce
>          ASSERT(!child->nextSibling());
>          ASSERT(!child->previousSibling());
>  
> +        InspectorController::willInsertDOMNode(child.get(), this);
> +
>          // Add child after "prev".
>          forbidEventDispatch();
>          Node* next;
> @@ -555,6 +561,8 @@ bool ContainerNode::appendChild(PassRefPtr<Node> newChild, ExceptionCode& ec, bo
>                  break;
>          }
>  
> +        InspectorController::willInsertDOMNode(child, this);
> +
>          // Append child to the end of the list
>          forbidEventDispatch();
>          child->setParent(this);
> @@ -593,6 +601,8 @@ void ContainerNode::parserAddChild(PassRefPtr<Node> newChild)
>      ASSERT(newChild);
>      ASSERT(!newChild->parent()); // Use appendChild if you need to handle reparenting (and want DOM mutation events).
>  
> +    InspectorController::willInsertDOMNode(newChild.get(), this);
> +
>      forbidEventDispatch();
>      Node* last = m_lastChild;
>      // FIXME: This method should take a PassRefPtr.
> @@ -964,12 +974,7 @@ static void notifyChildInserted(Node* child)
>  {
>      ASSERT(!eventDispatchForbidden());
>  
> -#if ENABLE(INSPECTOR)
> -    if (Page* page = child->document()->page()) {
> -        if (InspectorController* inspectorController = page->inspectorController())
> -            inspectorController->didInsertDOMNode(child);
> -    }
> -#endif
> +    InspectorController::didInsertDOMNode(child);
>  
>      RefPtr<Node> c = child;
>      RefPtr<Document> document = child->document();
> @@ -1003,12 +1008,7 @@ static void dispatchChildRemovalEvents(Node* child)
>  {
>      ASSERT(!eventDispatchForbidden());
>  
> -#if ENABLE(INSPECTOR)    
> -    if (Page* page = child->document()->page()) {
> -        if (InspectorController* inspectorController = page->inspectorController())
> -            inspectorController->didRemoveDOMNode(child);
> -    }
> -#endif
> +    InspectorController::willRemoveDOMNode(child);
>  
>      RefPtr<Node> c = child;
>      RefPtr<Document> document = child->document();
> diff --git a/WebCore/dom/Element.cpp b/WebCore/dom/Element.cpp
> index 39bf393..fb17565 100644
> --- a/WebCore/dom/Element.cpp
> +++ b/WebCore/dom/Element.cpp
> @@ -540,6 +540,11 @@ void Element::setAttribute(const AtomicString& name, const AtomicString& value,
>          return;
>      }
>  
> +#if ENABLE(INSPECTOR)
> +    if (!isSynchronizingStyleAttribute())
> +        InspectorController::willModifyDOMAttr(this);
> +#endif
> +
>      const AtomicString& localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
>  
>      // Allocate attribute map if necessary.
> @@ -563,17 +568,18 @@ void Element::setAttribute(const AtomicString& name, const AtomicString& value,
>      }
>  
>  #if ENABLE(INSPECTOR)
> -    if (Page* page = document()->page()) {
> -        if (InspectorController* inspectorController = page->inspectorController()) {
> -            if (!isSynchronizingStyleAttribute())
> -                inspectorController->didModifyDOMAttr(this);
> -        }
> -    }
> +    if (!isSynchronizingStyleAttribute())
> +        InspectorController::didModifyDOMAttr(this);
>  #endif
>  }
>  
>  void Element::setAttribute(const QualifiedName& name, const AtomicString& value, ExceptionCode&)
>  {
> +#if ENABLE(INSPECTOR)
> +    if (!isSynchronizingStyleAttribute())
> +        InspectorController::willModifyDOMAttr(this);
> +#endif
> +
>      document()->incDOMTreeVersion();
>  
>      // Allocate attribute map if necessary.
> @@ -592,12 +598,8 @@ void Element::setAttribute(const QualifiedName& name, const AtomicString& value,
>      }
>  
>  #if ENABLE(INSPECTOR)
> -    if (Page* page = document()->page()) {
> -        if (InspectorController* inspectorController = page->inspectorController()) {
> -            if (!isSynchronizingStyleAttribute())
> -                inspectorController->didModifyDOMAttr(this);
> -        }
> -    }
> +    if (!isSynchronizingStyleAttribute())
> +        InspectorController::didModifyDOMAttr(this);
>  #endif
>  }
>  
> @@ -1228,6 +1230,8 @@ void Element::setAttributeNS(const AtomicString& namespaceURI, const AtomicStrin
>  
>  void Element::removeAttribute(const String& name, ExceptionCode& ec)
>  {
> +    InspectorController::willModifyDOMAttr(this);
> +
>      String localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
>  
>      if (m_attributeMap) {
> @@ -1236,13 +1240,7 @@ void Element::removeAttribute(const String& name, ExceptionCode& ec)
>              ec = 0;
>      }
>      
> -#if ENABLE(INSPECTOR)
> -    if (Page* page = document()->page()) {
> -        if (InspectorController* inspectorController = page->inspectorController())
> -            inspectorController->didModifyDOMAttr(this);
> -    }
> -#endif
> -    
> +    InspectorController::didModifyDOMAttr(this);
>  }
>  
>  void Element::removeAttributeNS(const String& namespaceURI, const String& localName, ExceptionCode& ec)
> diff --git a/WebCore/inspector/Inspector.idl b/WebCore/inspector/Inspector.idl
> index 1f034b2..f58ae67 100644
> --- a/WebCore/inspector/Inspector.idl
> +++ b/WebCore/inspector/Inspector.idl
> @@ -72,7 +72,7 @@ module core {
>          [notify] void debuggerWasDisabled();
>          [notify] void failedToParseScriptSource(out String url, out String data, out int firstLine, out int errorLine, out String errorMessage);
>          [notify] void parsedScriptSource(out String sourceID, out String url, out String data, out int firstLine, out int scriptWorldType);
> -        [notify] void pausedScript(out Value callFrames);
> +        [notify] void pausedScript(out Value callFrames, out Value details);
Instead of adding additional parameter change the Value parameter to Object and put the callFrames along with the details into it.


>          [notify] void profilerWasEnabled();
>          [notify] void profilerWasDisabled();
>          [notify] void restoredBreakpoint(out String sourceID, out String url, out int line, out boolean enabled, out String condition);
> diff --git a/WebCore/inspector/InspectorController.h b/WebCore/inspector/InspectorController.h
> index 7ed2549..f9981a6 100644
> --- a/WebCore/inspector/InspectorController.h
> +++ b/WebCore/inspector/InspectorController.h
> @@ -31,7 +31,10 @@
>  
>  #include "Console.h"
>  #include "Cookie.h"
> +#include "Element.h"
>  #include "InspectorDOMAgent.h"
> +#include "InspectorDebuggerAgent.h"
> +#include "Page.h"
>  #include "PlatformString.h"
>  #include "ScriptState.h"
>  #include <wtf/HashMap.h>
> @@ -60,7 +63,6 @@ class InspectorClient;
>  class InspectorCSSStore;
>  class InspectorDOMStorageResource;
>  class InspectorDatabaseResource;
> -class InspectorDebuggerAgent;
>  class InspectorFrontend;
>  class InspectorFrontendClient;
>  class InspectorObject;
> @@ -183,9 +185,13 @@ public:
>      void mainResourceFiredLoadEvent(DocumentLoader*, const KURL&);
>      void mainResourceFiredDOMContentEvent(DocumentLoader*, const KURL&);
>  
> -    void didInsertDOMNode(Node*);
> -    void didRemoveDOMNode(Node*);
> -    void didModifyDOMAttr(Element*);
> +    static void willInsertDOMNode(Node* node, Node* parent);
> +    static void didInsertDOMNode(Node*);
> +    static void willRemoveDOMNode(Node*);
> +    static void willModifyDOMAttr(Element*);
> +    static void didModifyDOMAttr(Element*);
> +    static InspectorController* inspectorControllerForNode(Node*);
This method should be private.


> +
>  #if ENABLE(WORKERS)
>      enum WorkerAction { WorkerCreated, WorkerDestroyed };
>  
> @@ -376,30 +382,84 @@ private:
>  #endif
>  };
>  
> +inline void InspectorController::willInsertDOMNode(Node* node, Node* parent)
> +{
> +#if ENABLE(JAVASCRIPT_DEBUGGER)
> +    if (InspectorController* inspectorController = inspectorControllerForNode(node)) {
> +        InspectorDebuggerAgent* debuggerAgent = inspectorController->debuggerAgent();
> +        if (!debuggerAgent)
> +            return;
> +        InspectorDOMAgent* domAgent = inspectorController->domAgent();
> +        if (!domAgent)
> +            return;
> +        RefPtr<InspectorValue> breakpoint = domAgent->getBreakpointForNodeInsertion(node, parent);
> +        if (!breakpoint->isNull())
> +            debuggerAgent->breakProgram(breakpoint);
> +    }
> +#endif
> +}
> +
>  inline void InspectorController::didInsertDOMNode(Node* node)
>  {
>  #if ENABLE(INSPECTOR)
> -    if (m_domAgent)
> -        m_domAgent->didInsertDOMNode(node);
> +    if (InspectorController* inspectorController = inspectorControllerForNode(node)) {
> +        if (InspectorDOMAgent* domAgent = inspectorController->domAgent())
> +            domAgent->didInsertDOMNode(node);
> +    }
>  #endif
>  }
>  
> -inline void InspectorController::didRemoveDOMNode(Node* node)
> +inline void InspectorController::willRemoveDOMNode(Node* node)
>  {
> -#if ENABLE(INSPECTOR)
> -    if (m_domAgent)
> -        m_domAgent->didRemoveDOMNode(node);
> +#if ENABLE(JAVASCRIPT_DEBUGGER)
> +    if (InspectorController* inspectorController = inspectorControllerForNode(node)) {
> +        InspectorDOMAgent* domAgent = inspectorController->domAgent();
> +        if (!domAgent)
> +            return;
> +        if (InspectorDebuggerAgent* debuggerAgent = inspectorController->debuggerAgent()) {
> +            RefPtr<InspectorValue> breakpoint = domAgent->getBreakpointForNodeRemoval(node);
> +            if (!breakpoint->isNull())
> +                debuggerAgent->breakProgram(breakpoint);
> +        }
> +        domAgent->didRemoveDOMNode(node);
> +    }
> +#endif
> +}
> +
> +inline void InspectorController::willModifyDOMAttr(Element* element)
> +{
> +#if ENABLE(JAVASCRIPT_DEBUGGER)
> +    if (InspectorController* inspectorController = inspectorControllerForNode(element)) {
> +        InspectorDebuggerAgent* debuggerAgent = inspectorController->debuggerAgent();
> +        if (!debuggerAgent)
> +            return;
> +        InspectorDOMAgent* domAgent = inspectorController->domAgent();
> +        if (!domAgent)
> +            return;
> +        RefPtr<InspectorValue> breakpoint = domAgent->getBreakpointForAttributeModification(element);
> +        if (!breakpoint->isNull())
> +            debuggerAgent->breakProgram(breakpoint);
> +    }
>  #endif
>  }
>  
>  inline void InspectorController::didModifyDOMAttr(Element* element)
>  {
>  #if ENABLE(INSPECTOR)
> -    if (m_domAgent)
> -        m_domAgent->didModifyDOMAttr(element);
> +    if (InspectorController* inspectorController = inspectorControllerForNode(element)) {
> +        if (InspectorDOMAgent* domAgent = inspectorController->domAgent())
> +            domAgent->didModifyDOMAttr(element);
> +    }
>  #endif
>  }
>  
> +inline InspectorController* InspectorController::inspectorControllerForNode(Node* node)
> +{
> +    if (Page* page = node->document()->page())
> +        return page->inspectorController();
> +    return 0;
> +}
> +
>  } // namespace WebCore
>  
>  #endif // !defined(InspectorController_h)
> diff --git a/WebCore/inspector/InspectorDOMAgent.cpp b/WebCore/inspector/InspectorDOMAgent.cpp
> index 82827bd..17e957e 100644
> --- a/WebCore/inspector/InspectorDOMAgent.cpp
> +++ b/WebCore/inspector/InspectorDOMAgent.cpp
> @@ -64,7 +64,6 @@
>  #include "PlatformString.h"
>  #include "RenderStyle.h"
>  #include "RenderStyleConstants.h"
> -#include "ScriptDebugServer.h"
>  #include "ScriptEventListener.h"
>  #include "StyleSheetList.h"
>  #include "Text.h"
> @@ -210,8 +209,6 @@ const int domBreakpointDerivedTypeShift = 16;
>  
>  }
>  
> -InspectorDOMAgent* InspectorDOMAgent::s_domAgentOnBreakpoint = 0;
> -
>  InspectorDOMAgent::InspectorDOMAgent(InspectorCSSStore* cssStore, InspectorFrontend* frontend)
>      : EventListener(InspectorDOMAgentType)
>      , m_cssStore(cssStore)
> @@ -224,9 +221,6 @@ InspectorDOMAgent::InspectorDOMAgent(InspectorCSSStore* cssStore, InspectorFront
>  InspectorDOMAgent::~InspectorDOMAgent()
>  {
>      reset();
> -
> -    if (this == s_domAgentOnBreakpoint)
> -        s_domAgentOnBreakpoint = 0;
>  }
>  
>  void InspectorDOMAgent::reset()
> @@ -780,6 +774,29 @@ void InspectorDOMAgent::removeDOMBreakpoint(long nodeId, long type)
>      }
>  }
>  
> +PassRefPtr<InspectorValue> InspectorDOMAgent::getBreakpointForNodeInsertion(Node*, Node* parent)
> +{
> +    if (hasBreakpoint(parent, SubtreeModified))
> +        return createBreakpoint(parent, SubtreeModified);
> +    return InspectorObject::null();
> +}
> +
> +PassRefPtr<InspectorValue> InspectorDOMAgent::getBreakpointForNodeRemoval(Node* node)
> +{
> +    if (hasBreakpoint(node, NodeRemoved))
> +        return createBreakpoint(node, NodeRemoved);
> +    if (hasBreakpoint(innerParentNode(node), SubtreeModified))
> +        return createBreakpoint(innerParentNode(node), SubtreeModified);
> +    return InspectorObject::null();
> +}
> +
> +PassRefPtr<InspectorValue> InspectorDOMAgent::getBreakpointForAttributeModification(Element* element)
> +{
> +    if (hasBreakpoint(element, AttributeModified))
> +        return createBreakpoint(element, AttributeModified);
> +    return InspectorObject::null();
> +}
> +
>  String InspectorDOMAgent::documentURLString(Document* document) const
>  {
>      if (!document || document->url().isNull())
> @@ -985,12 +1002,7 @@ void InspectorDOMAgent::didInsertDOMNode(Node* node)
>          return;
>  
>      if (m_breakpoints.size()) {
> -        Node* parent = innerParentNode(node);
> -        if (hasBreakpoint(parent, SubtreeModified)) {
> -            if (!pauseOnBreakpoint())
> -                return;
> -        }
> -        uint32_t mask = m_breakpoints.get(parent);
> +        uint32_t mask = m_breakpoints.get(innerParentNode(node));
>          uint32_t inheritableTypesMask = (mask | (mask >> domBreakpointDerivedTypeShift)) & inheritableDOMBreakpointTypesMask;
>          if (inheritableTypesMask)
>              updateSubtreeBreakpoints(node, inheritableTypesMask, true);
> @@ -1023,10 +1035,6 @@ void InspectorDOMAgent::didRemoveDOMNode(Node* node)
>          return;
>  
>      if (m_breakpoints.size()) {
> -        if (hasBreakpoint(node, NodeRemoved) || hasBreakpoint(innerParentNode(node), SubtreeModified)) {
> -            if (!pauseOnBreakpoint())
> -                return;
> -        }
>          // Remove subtree breakpoints.
>          m_breakpoints.remove(node);
>          Vector<Node*> stack(1, innerFirstChild(node));
> @@ -1063,12 +1071,24 @@ void InspectorDOMAgent::didModifyDOMAttr(Element* element)
>      if (!id)
>          return;
>  
> -    if (hasBreakpoint(element, AttributeModified)) {
> -        if (!pauseOnBreakpoint())
> -            return;
> +    m_frontend->attributesUpdated(id, buildArrayForElementAttributes(element));
> +}
> +
> +PassRefPtr<InspectorObject> InspectorDOMAgent::createBreakpoint(Node* node, long type)
> +{
> +    RefPtr<InspectorObject> breakpoint = InspectorObject::create();
> +
> +    // Find breakpoint owner.
> +    while (!(m_breakpoints.get(node) & (1 << type))) {
> +        node = innerParentNode(node);
> +        ASSERT(node);
>      }
> +    long nodeId = m_documentNodeToIdMap.get(node);
> +    ASSERT(nodeId);
>  
> -    m_frontend->attributesUpdated(id, buildArrayForElementAttributes(element));
> +    breakpoint->setNumber("nodeId", nodeId);
> +    breakpoint->setNumber("type", type);
> +    return breakpoint.release();
>  }
>  
>  bool InspectorDOMAgent::hasBreakpoint(Node* node, long type)
> @@ -1078,19 +1098,6 @@ bool InspectorDOMAgent::hasBreakpoint(Node* node, long type)
>      return m_breakpoints.get(node) & (rootBit | derivedBit);
>  }
>  
> -bool InspectorDOMAgent::pauseOnBreakpoint()
> -{
> -#if ENABLE(JAVASCRIPT_DEBUGGER)
> -    s_domAgentOnBreakpoint = this;
> -    ScriptDebugServer::shared().breakProgram();
> -    bool deleted = !s_domAgentOnBreakpoint;
> -    s_domAgentOnBreakpoint = 0;
> -    return !deleted;
> -#else
> -    return true;
> -#endif
> -}
> -
>  void InspectorDOMAgent::updateSubtreeBreakpoints(Node* node, uint32_t rootMask, bool set)
>  {
>      uint32_t oldMask = m_breakpoints.get(node);
> diff --git a/WebCore/inspector/InspectorDOMAgent.h b/WebCore/inspector/InspectorDOMAgent.h
> index fd3c5b5..2787802 100644
> --- a/WebCore/inspector/InspectorDOMAgent.h
> +++ b/WebCore/inspector/InspectorDOMAgent.h
> @@ -36,7 +36,6 @@
>  #include "InspectorCSSStore.h"
>  #include "InspectorValues.h"
>  #include "NodeList.h"
> -#include "ScriptState.h"
>  #include "Timer.h"
>  
>  #include <wtf/Deque.h>
> @@ -114,6 +113,9 @@ namespace WebCore {
>          void searchCanceled();
>          void setDOMBreakpoint(long nodeId, long type);
>          void removeDOMBreakpoint(long nodeId, long type);
> +        PassRefPtr<InspectorValue> getBreakpointForNodeInsertion(Node* node, Node* parent);
> +        PassRefPtr<InspectorValue> getBreakpointForNodeRemoval(Node*);
> +        PassRefPtr<InspectorValue> getBreakpointForAttributeModification(Element*);
>  
>          // Methods called from the frontend for CSS styles inspection.
>          void getStyles(long nodeId, bool authorOnly, RefPtr<InspectorValue>* styles);
> @@ -160,7 +162,7 @@ namespace WebCore {
>          bool pushDocumentToFrontend();
>  
>          bool hasBreakpoint(Node* node, long type);
> -        bool pauseOnBreakpoint();
> +        PassRefPtr<InspectorObject> createBreakpoint(Node* node, long type);
>          void updateSubtreeBreakpoints(Node* root, uint32_t rootMask, bool value);
>  
>          PassRefPtr<InspectorObject> buildObjectForAttributeStyles(Element* element);
> @@ -217,8 +219,6 @@ namespace WebCore {
>          HashSet<RefPtr<Node> > m_searchResults;
>          Vector<long> m_inspectedNodes;
>          HashMap<Node*, uint32_t> m_breakpoints;
> -
> -        static InspectorDOMAgent* s_domAgentOnBreakpoint;
>      };
>  
>  #endif
> diff --git a/WebCore/inspector/InspectorDebuggerAgent.cpp b/WebCore/inspector/InspectorDebuggerAgent.cpp
> index 357a043..afba025 100644
> --- a/WebCore/inspector/InspectorDebuggerAgent.cpp
> +++ b/WebCore/inspector/InspectorDebuggerAgent.cpp
> @@ -56,11 +56,14 @@ PassOwnPtr<InspectorDebuggerAgent> InspectorDebuggerAgent::create(InspectorContr
>      return agent.release();
>  }
>  
> +InspectorDebuggerAgent* InspectorDebuggerAgent::s_debuggerAgentOnBreakpoint = 0;
> +
>  InspectorDebuggerAgent::InspectorDebuggerAgent(InspectorController* inspectorController, InspectorFrontend* frontend)
>      : m_inspectorController(inspectorController)
>      , m_frontend(frontend)
>      , m_pausedScriptState(0)
>      , m_breakpointsLoaded(false)
> +    , m_breakProgramDetails(InspectorValue::null())
>  {
>  }
>  
> @@ -68,6 +71,9 @@ InspectorDebuggerAgent::~InspectorDebuggerAgent()
>  {
>      ScriptDebugServer::shared().removeListener(this, m_inspectorController->inspectedPage());
>      m_pausedScriptState = 0;
> +
> +    if (this == s_debuggerAgentOnBreakpoint)
> +        s_debuggerAgentOnBreakpoint = 0;
>  }
>  
>  bool InspectorDebuggerAgent::isDebuggerAlwaysEnabled()
> @@ -283,8 +289,7 @@ void InspectorDebuggerAgent::didPause(ScriptState* scriptState)
>  {
>      ASSERT(scriptState && !m_pausedScriptState);
>      m_pausedScriptState = scriptState;
> -    RefPtr<InspectorValue> callFrames = currentCallFrames();
> -    m_frontend->pausedScript(callFrames.get());
> +    m_frontend->pausedScript(currentCallFrames(), m_breakProgramDetails);
>  }
>  
>  void InspectorDebuggerAgent::didContinue()
> @@ -293,6 +298,19 @@ void InspectorDebuggerAgent::didContinue()
>      m_frontend->resumedScript();
>  }
>  
> +void InspectorDebuggerAgent::breakProgram(PassRefPtr<InspectorValue> details)
> +{
> +    s_debuggerAgentOnBreakpoint = this;
> +    m_breakProgramDetails = details;
> +
> +    ScriptDebugServer::shared().breakProgram();
> +    if (!s_debuggerAgentOnBreakpoint)
> +        return;
> +
> +    s_debuggerAgentOnBreakpoint = 0;
> +    m_breakProgramDetails = InspectorValue::null();
> +}
> +
>  } // namespace WebCore
>  
>  #endif // ENABLE(JAVASCRIPT_DEBUGGER)
> diff --git a/WebCore/inspector/InspectorDebuggerAgent.h b/WebCore/inspector/InspectorDebuggerAgent.h
> index 91bcd49..6d6e8bc 100644
> --- a/WebCore/inspector/InspectorDebuggerAgent.h
> +++ b/WebCore/inspector/InspectorDebuggerAgent.h
> @@ -61,6 +61,7 @@ public:
>      void getScriptSource(const String& sourceID, String* scriptSource);
>  
>      void pause();
> +    void breakProgram(PassRefPtr<InspectorValue> details);
>      void resume();
>      void stepOverStatement();
>      void stepIntoStatement();
> @@ -93,6 +94,8 @@ private:
>      HashMap<String, SourceBreakpoints> m_stickyBreakpoints;
>      HashMap<String, unsigned> m_breakpointsMapping;
>      bool m_breakpointsLoaded;
> +    static InspectorDebuggerAgent* s_debuggerAgentOnBreakpoint;
> +    RefPtr<InspectorValue> m_breakProgramDetails;
>  };
>  
>  } // namespace WebCore
> diff --git a/WebCore/inspector/InspectorValues.h b/WebCore/inspector/InspectorValues.h
> index 3dd9594..4036f55 100644
> --- a/WebCore/inspector/InspectorValues.h
> +++ b/WebCore/inspector/InspectorValues.h
> @@ -67,6 +67,8 @@ public:
>  
>      Type type() const { return m_type; }
>  
> +    bool isNull() const { return m_type == TypeNull; }
> +
>      virtual bool asBoolean(bool* output) const;
>      virtual bool asNumber(double* output) const;
>      virtual bool asNumber(long* output) const;
> diff --git a/WebCore/inspector/front-end/BreakpointsSidebarPane.js b/WebCore/inspector/front-end/BreakpointsSidebarPane.js
> index 3a0860f..29be646 100644
> --- a/WebCore/inspector/front-end/BreakpointsSidebarPane.js
> +++ b/WebCore/inspector/front-end/BreakpointsSidebarPane.js
> @@ -106,7 +106,7 @@ WebInspector.BreakpointItem = function(breakpoint)
>      this._element.appendChild(checkboxElement);
>  
>      this._breakpoint.addEventListener("enable-changed", this._enableChanged, this);
> -    this._breakpoint.addEventListener("removed", this._removed, this);
> +    this._breakpoint.addEventListener("removed", this.dispatchEventToListeners.bind(this, "removed"));
>  }
>  
>  WebInspector.BreakpointItem.prototype = {
> @@ -128,15 +128,10 @@ WebInspector.BreakpointItem.prototype = {
>          event.stopPropagation();
>      },
>  
> -    _enableChanged: function()
> +    _enableChanged: function(event)
>      {
>          var checkbox = this._element.firstChild;
>          checkbox.checked = this._breakpoint.enabled;
> -    },
> -
> -    _removed: function()
> -    {
> -        this.dispatchEventToListeners("removed");
>      }
>  }
>  
> diff --git a/WebCore/inspector/front-end/DOMAgent.js b/WebCore/inspector/front-end/DOMAgent.js
> index 5aaa0d3..e2c880a 100644
> --- a/WebCore/inspector/front-end/DOMAgent.js
> +++ b/WebCore/inspector/front-end/DOMAgent.js
> @@ -417,7 +417,7 @@ WebInspector.DOMAgent.prototype = {
>          parent.removeChild_(node);
>          var event = { target : node, relatedNode : parent };
>          this.document._fireDomEvent("DOMNodeRemoved", event);
> -        delete this._idToDOMNode[nodeId];
> +        delete this._idToDOMNode[node.id];
Why is that change?


>      }
>  }
>  
> @@ -719,6 +719,13 @@ WebInspector.DOMBreakpointManager.prototype = {
>          for (var type in nodeBreakpoints)
>              return;
>          delete this._breakpoints[breakpoint.node.id];
> +    },
> +
> +    breakpointHit: function(nodeId, type)
> +    {
> +        var nodeBreakpoints = this._breakpoints[nodeId];
> +        if (type in nodeBreakpoints)
> +            nodeBreakpoints[type].dispatchEventToListeners("hit");
>      }
>  }
>  
> diff --git a/WebCore/inspector/front-end/Script.js b/WebCore/inspector/front-end/Script.js
> index 42d6850..2ef9328 100644
> --- a/WebCore/inspector/front-end/Script.js
> +++ b/WebCore/inspector/front-end/Script.js
> @@ -62,11 +62,12 @@ WebInspector.Script.prototype = {
>      {
>          if (!this.source)
>              return 0;
> -        this._linesCount = 0;
> +        var linesCount = 0;
I think the value was supposed to be cached. Instead of changing it into a local variable you should check whether it's been calculated.



>          var lastIndex = this.source.indexOf("\n");
>          while (lastIndex !== -1) {
>              lastIndex = this.source.indexOf("\n", lastIndex + 1)
> -            this._linesCount++;
> +            linesCount++;
>          }
> +        return linesCount;
>      }
>  }
> diff --git a/WebCore/inspector/front-end/inspector.js b/WebCore/inspector/front-end/inspector.js
> index 26dfa09..11cbe98 100644
> --- a/WebCore/inspector/front-end/inspector.js
> +++ b/WebCore/inspector/front-end/inspector.js
> @@ -1445,9 +1445,12 @@ WebInspector.failedToParseScriptSource = function(sourceURL, source, startingLin
>      this.panels.scripts.addScript(null, sourceURL, source, startingLine, errorLine, errorMessage);
>  }
>  
> -WebInspector.pausedScript = function(callFrames)
> +WebInspector.pausedScript = function(callFrames, details)
>  {
>      this.panels.scripts.debuggerPaused(callFrames);
> +    if (details)
> +        this.domBreakpointManager.breakpointHit(details.nodeId, details.type);
> +
>      InspectorFrontendHost.bringToFront();
>  }
>

[Yury Semikhatsky]

Also we need tests for DOM breakpoints.

[Pavel Podivilov]

Created attachment 66030 [details]
All comments addressed.

[Pavel Podivilov]

Created attachment 66210 [details]
Rebase.

[WebKit Commit Bot]

Comment on attachment 66210 [details]
Rebase.

Rejecting patch 66210 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
bKit.build/Debug/WebKit.build/Objects-normal/i386/WebInspectorClientCF.o /Users/eseidel/Projects/CommitQueue/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp normal i386 c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Objects-normal/i386/WebInspectorFrontend.o /Users/eseidel/Projects/CommitQueue/WebKit/mac/WebInspector/WebInspectorFrontend.mm normal i386 objective-c++ com.apple.compilers.gcc.4_2
(7 failures)


Full output: http://queues.webkit.org/results/3899017

[Pavel Podivilov]

Created attachment 66344 [details]
Fixed compilation problem.

[WebKit Commit Bot]

Comment on attachment 66344 [details]
Fixed compilation problem.

Rejecting patch 66344 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20898 test cases.
dom/xhtml/level3/core/documentadoptnode12.xhtml -> crashed

Exiting early after 1 failures. 3472 tests run.
753.46s total testing time

3471 test cases (99%) succeeded
1 test case (<1%) crashed

Full output: http://queues.webkit.org/results/3958048

[Pavel Podivilov]

Created attachment 66379 [details]
Fix crash

InspectorController::willInsertDOMNode should get page from parent node, because child doesn't have document yet.

[WebKit Commit Bot]

Comment on attachment 66379 [details]
Fix crash

Rejecting patch 66379 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20910 test cases.
http/tests/inspector/resource-har-conversion.html -> failed

Exiting early after 1 failures. 20042 tests run.
526.74s total testing time

20041 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
27 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3946085

[Pavel Podivilov]

Created attachment 66484 [details]
Proposed patch.

[WebKit Commit Bot]

Comment on attachment 66484 [details]
Proposed patch.

Clearing flags on attachment: 66484

Committed r66730: <http://trac.webkit.org/changeset/66730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66730 might have broken Qt Windows 32-bit Release