Web Inspector: adding pause icon for JavaScript debugging

[Sergey Rogulenko]

Adding pause icon for JavaScript debugging

[Sergey Rogulenko]

Created attachment 151453 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 151453 [details]
Patch

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.

[Andrey Kosyakov]

Comment on attachment 151453 [details]
Patch

As discussed, approach generally looks good, but it needs a bit more polish on both UI and implementation. Removing r? for the time being.

[Sergey Rogulenko]

Created attachment 151484 [details]
Patch

[Timothy Hatcher]

It is not clear what this is about. Please add a description and/or mock up.

[Sergey Rogulenko]

Created attachment 151645 [details]
The screen on the left is the original page, the screen on the right is how it looks, when you are standing paused on a break point.

[Sergey Rogulenko]

Created attachment 151685 [details]
Patch

[Sergey Rogulenko]

Created attachment 151706 [details]
Patch

[Sergey Rogulenko]

Created attachment 151894 [details]
Patch

[Sergey Rogulenko]

Created attachment 151920 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 151920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151920&action=review

Can we disable dashboard when "inspect element" mode is on?

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:372
> +class DevToolsDashBoard : public WebPageOverlay {

Can you please move it to a file of its own?

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:443
> +            DEFINE_STATIC_LOCAL(RefPtr<Image>, buttonImageDefault, (Image::loadPlatformResource("mediaplayerPlayDisabled")));
> +            DEFINE_STATIC_LOCAL(RefPtr<Image>, buttonImage, (Image::loadPlatformResource("mediaplayerPlay")));
> +            DEFINE_STATIC_LOCAL(RefPtr<Image>, buttonImageDown, (Image::loadPlatformResource("mediaplayerPlayDown")));
> +            DEFINE_STATIC_LOCAL(RefPtr<Image>, buttonImageHover, (Image::loadPlatformResource("mediaplayerPlayHover")));

I think we could pass resource ids from the outside, so we can reuse the class for other buttons. I think you can just pass the root name (e.g. "mediaplayerPlay") and derive the rest by adding specific states (Disabled/Play/Down/etc)

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:459
> +        void getState(bool& isFocused, bool& isPressed)

does this have to be public?

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:465
> +        void updateState(bool isFocused, bool isPressed)

ditto

[Pavel Feldman]

Comment on attachment 151920 [details]
Patch

r- as per our discussion offline - lets reuse existing DOM highlight plumbing.

[Sergey Rogulenko]

Created attachment 152280 [details]
Patch

[Timothy Hatcher]

Comment on attachment 152280 [details]
Patch

I preferred the approach where the client was repsosible for showing/drawing this. Can you make this be PLATFORM(CHROMUM) or !PLATFORM(MAC)?

[Timothy Hatcher]

Comment on attachment 152280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152280&action=review

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:490
> +InspectorDashBoard::InspectorDashBoard(InstrumentingAgents* instrumentingAgents, InspectorClient* client, InspectorPageAgent* pageAgent)

DashBoard is one word. So this should be InspectorDashboard. Though, I'm not sure this should even be a separate class. Also classes in WebCore get their own file.

[Timothy Hatcher]

Ugh, sorry for all the typos earlier.

[Pavel Feldman]

Comment on attachment 152280 [details]
Patch

r- as per Timothy's comments.

[Pavel Feldman]

Comment on attachment 152280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152280&action=review

As we agreed offline, lets do the following:
1) Convert DOMNodeHighlighter into the InspectorOverlay class
2) Put its instance into the InspectorPageAgent and configure it from within InspectorDOMAgent and PageDebuggerAgent
3) Make sure that getHighlight is still available on this new overlay so that Timothy could access it and paint highlight on the embedder level.

Timothy, this should not change the way things work for you.

>> Source/WebCore/inspector/DOMNodeHighlighter.cpp:490
>> +InspectorDashBoard::InspectorDashBoard(InstrumentingAgents* instrumentingAgents, InspectorClient* client, InspectorPageAgent* pageAgent)
> 
> DashBoard is one word. So this should be InspectorDashboard. Though, I'm not sure this should even be a separate class. Also classes in WebCore get their own file.

You don't need instrumentingAgents here.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:206
> +    , m_dashBoard(InspectorDashBoard::create(instrumentingAgents, client, pageAgent))

Dashboard should not belong to DOMAgent.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1091
> +    if (InspectorInstrumentation::isDebuggerPaused(m_instrumentingAgents))

DOM agent should not be debugger-aware.

> Source/WebCore/inspector/InspectorInstrumentation.h:101
> +    static bool isDebuggerPaused(InstrumentingAgents*);

Why do you need this?

[Sergey Rogulenko]

Created attachment 152504 [details]
Patch

[Sergey Rogulenko]

Created attachment 152506 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 152506 [details]
Patch

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?

[Sergey Rogulenko]

Created attachment 152523 [details]
Patch

[Early Warning System Bot]

Comment on attachment 152523 [details]
Patch

Attachment 152523 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13239949

[Build Bot]

Comment on attachment 152523 [details]
Patch

Attachment 152523 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13243967

[Early Warning System Bot]

Comment on attachment 152523 [details]
Patch

Attachment 152523 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13254249

[Sergey Rogulenko]

Created attachment 152526 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152526 [details]
Patch

Attachment 152526 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13239956

[Early Warning System Bot]

Comment on attachment 152526 [details]
Patch

Attachment 152526 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13254253

[Build Bot]

Comment on attachment 152526 [details]
Patch

Attachment 152526 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13248544

[Build Bot]

Comment on attachment 152526 [details]
Patch

Attachment 152526 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13239957

[WebKit Review Bot]

Comment on attachment 152526 [details]
Patch

Attachment 152526 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13245901

[Early Warning System Bot]

Comment on attachment 152526 [details]
Patch

Attachment 152526 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13238941

[Sergey Rogulenko]

Created attachment 152534 [details]
Patch

[Sergey Rogulenko]

Created attachment 152545 [details]
Patch

[Build Bot]

Comment on attachment 152545 [details]
Patch

Attachment 152545 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13233984

[Sergey Rogulenko]

Created attachment 152553 [details]
Patch

[Sergey Rogulenko]

Created attachment 152558 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 152558 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152558&action=review

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:547
> +void InspectorOverlay::update()

InspectorOverlay should be in a file of its own.

> Source/WebCore/inspector/InspectorPageAgent.cpp:851
> +void InspectorPageAgent::drawInspectorOverlay(GraphicsContext& context)
> +{
> +    m_overlay->draw(context);
> +}

Why do we need this method on InspectorPageAgent?

[Sergey Rogulenko]

Created attachment 152567 [details]
Patch

[Pavel Feldman]

Comment on attachment 152567 [details]
Patch

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?

[Sergey Rogulenko]

Created attachment 152744 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 152744 [details]
Patch

LGTM, though I liked the previous approach where highlight was mutually exclusive with pause screen.

[Pavel Feldman]

Comment on attachment 152744 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152744&action=review

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

Nit: we rarely use "is" prefix.

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:501
> +        drawHighlight(context);

Nit: You could simply call drawHighlight here and bail out from there in case of no highlightData.

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:541
> +void InspectorOverlay::setIsPausedInDebugger(bool flag)

setPausedInDebugger, sorry about that.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1038
> +    m_overlay->setHighlight(m_document.get(), m_highlightData.get());

m_document will expire upon reset.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1606
> +    m_overlay->getHighlight(m_highlightData->node ? m_highlightData->node->document() : m_document.get(), m_highlightData.get(), highlight);

I was hoping m_highlightData could go away from the DOMAgent.

[Sergey Rogulenko]

Created attachment 152783 [details]
Patch

[Early Warning System Bot]

Comment on attachment 152783 [details]
Patch

Attachment 152783 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13281215

[Early Warning System Bot]

Comment on attachment 152783 [details]
Patch

Attachment 152783 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13274185

[Gyuyoung Kim]

Comment on attachment 152783 [details]
Patch

Attachment 152783 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13274186

[Build Bot]

Comment on attachment 152783 [details]
Patch

Attachment 152783 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13272192

[Sergey Rogulenko]

Created attachment 152787 [details]
Patch

[Pavel Feldman]

Comment on attachment 152787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152787&action=review

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:73
> +static Color parseColor(const RefPtr<InspectorObject>* colorObject)

Overlay is a utility class, it should not depend on the inspector transport objects.

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:585
> +    m_highlightData = adoptPtr(new HighlightData());

I think this method should receive PassRefPtr<HighlightData> instead.

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:594
> +    m_highlightData = adoptPtr(new HighlightData());

ditto

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:611
> +    highlightConfig->getBoolean("showInfo", &showInfo);

ditto

[Sergey Rogulenko]

Created attachment 152976 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 152976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152976&action=review

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:567
> +    if (!m_client)
> +        return;

Do we need this check?

[Sergey Rogulenko]

Created attachment 152994 [details]
Patch

[Pavel Feldman]

Comment on attachment 152994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152994&action=review

Please fix OwnPtr nits prior to landing.

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:533
> +    if (m_highlightData) {

// FIXME: Clear entire highlight data here, store config upon setInspectModeEnabled

> Source/WebCore/inspector/DOMNodeHighlighter.h:96
> +    void setHighlightData(PassOwnPtr<HighlightData>);

I.e. highlightNode will merge with the setHighlightData.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:985
> +        m_overlay->highlightNode(node);

You should pass m_inspectModeHighlightData here (in the subsequent change).

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1009
> +    HighlightData* highlightData = new HighlightData();

You should always adoptPtr upon creation.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1029
> +    HighlightData* highlightData = new HighlightData();

ditto

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1054
> +        HighlightData* highlightData = new HighlightData();

ditto

[Sergey Rogulenko]

Created attachment 153003 [details]
Patch

[Andrey Kosyakov]

Committed r122966: <http://trac.webkit.org/changeset/122966>

[Vsevolod Vlasov]

Comment on attachment 153003 [details]
Patch

Clearing r?