[Webkit-unassigned] [Bug 90880] Web Inspector: adding pause icon for JavaScript debugging

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90880





--- Comment #21 from Andrey Kosyakov <caseq at chromium.org>  2012-07-16 05:08:23 PST ---
(From update of attachment 152506)
View in context: https://bugs.webkit.org/attachment.cgi?id=152506&action=review

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:495
> +    , m_highlightData(0) { }

style: { } each on its own line.

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:502
> +    else
> +    if (m_shouldShowPauseWarning)

style: merge to "else if"

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:549
> +    if (m_client) {

nit:
if (!m_client)
    return;

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:576
> +    Frame* frame = m_pageAgent->mainFrame();

This seems to be the only place we use page agent. Please consider storing page instead, so we avoid circular dependencies.

> Source/WebCore/inspector/DOMNodeHighlighter.h:83
> +class InspectorOverlay {

You'll need to rename the file.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1601
>  void InspectorDOMAgent::getHighlight(Highlight* highlight) const

Does DOM agent need this still?

> Source/WebCore/inspector/InspectorDebuggerAgent.h:138
> +    virtual bool isPageDebuggerAgent() = 0;

Having BaseClass::isSomeDerivedClass() is very unfortunate. Making it pure virtual -- doubly so. Can we avoid this by just checking for a non-null InspectorOverlay?

> Source/WebCore/inspector/InspectorPageAgent.cpp:319
> +    , m_overlay(InspectorOverlay::create(this, client))

I'd put overlay ownership to InspectorController, so that it's life-time is explicit and we avoid another reasons for agents to depend on each other. Pavel, WDYT?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.