[webkit-reviews] review denied: [Bug 90880] Web Inspector: adding pause icon for JavaScript debugging : [Attachment 152567] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Sergey Rogulenko
<rogulenko at google.com>'s request for review:
Bug 90880: Web Inspector: adding pause icon for JavaScript debugging
https://bugs.webkit.org/show_bug.cgi?id=90880

Attachment 152567: Patch
https://bugs.webkit.org/attachment.cgi?id=152567&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152567&action=review


> Source/WebCore/inspector/DOMNodeHighlighter.cpp:492
> +    , m_shouldShowPauseWarning(false)

This is not a warning, simply "m_pausedInDebugger"

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

I would not make these mutually exclusive - nuke the "else"!

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:503
> +	   drawPauseWarning(context);

drawIsPausedInDebugger.

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:569
> +void InspectorOverlay::drawPauseWarning(GraphicsContext& context)

drawIsPausedInDebugger

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:589
> +    String text = "Paused in debugger.";

Ok, we need to figure out how we localize it now.

> Source/WebCore/inspector/DOMNodeHighlighter.h:95
> +    void setShouldShowPauseWarning(bool);

setIsPausedInDebugger

> Source/WebCore/inspector/InspectorController.cpp:299
> +    m_overlay->draw(context);

Nit: We need to rename "draw" to "paint" everywhere in overlay at some point.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1036
>      // This method requires m_highlightData to have been filled in by its
client.

Now that highlightData lives in the InspectorOverlay, there is no need to
mirror it here.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:109
> +	   m_overlay->setShouldShowPauseWarning(false);

You should sniff for these from within PageDebuggerAgent.

> Source/WebCore/inspector/InspectorDebuggerAgent.h:128
> +    InspectorDebuggerAgent(InstrumentingAgents*, InspectorState*,
InjectedScriptManager*, InspectorOverlay*);

InspectorDebuggerAgent should not be affected. Only PageDebuggerAgent should
be.

> Source/WebCore/inspector/InspectorPageAgent.h:51
> +class GraphicsContext;

Remove this?