[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 #2 from Alexander Pavlov (apavlov) <apavlov at chromium.org>  2012-07-10 08:37:56 PST ---
(From update of attachment 151453)
View in context: https://bugs.webkit.org/attachment.cgi?id=151453&action=review

> Source/WebKit/chromium/ChangeLog:16
> +        (DashBoard):

Is this going to be implemented for ports other than Chromium?

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:649
> +        pageAgent->showPauseWarning();

My guess is that exposing InspectorClient in InspectorPageAgent would be a more flexible approach than adding a one-off method to the cross-agents API (as opposed to the other similar methods).

Also, having such calls AFTER the internal state change seems a bit cleaner.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:674
> +        pageAgent->hidePauseWarning();

Ditto

> Source/WebCore/inspector/InspectorPageAgent.cpp:970
> +void InspectorPageAgent::showPauseWarning()

Order of methods in a CPP file must correspond to that in the respective header file.

> Source/WebCore/inspector/InspectorPageAgent.h:131
> +    void showPauseWarning();

These can be replaced by a single
InspectorClient* client();
as discussed above.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:81
> +static const int pauseWarning = 100;

The name is not intuitive enough. Something like "jsPausedSign" (or even "dashBoard" if it is going to be used for something else) would be a bit more clear.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:387
> +        FrameView* frameView = this->frameView();

Seems like no methods are called on the frameView?

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:394
> +        FloatSize fs = FloatSize(m_webView->size());

Acronyms are not used as names in WebKit. You might as well static_cast m_webView->size()'s components to float rather than create a new FloatSize instance.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:399
> +    WebCore::FrameView* frameView()

This can be removed with its only call site.

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