Summary: | Web Inspector: adding pause icon for JavaScript debugging | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sergey Rogulenko <rogulenko> | ||||||||||||||||||||||||||||||||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, caseq, dglazkov, gustavo, gyuyoung.kim, joepeck, keishi, loislo, pfeldman, philn, pmuellr, rakuco, rik, timothy, webkit.review.bot, xan.lopez, yurys | ||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 93124 | ||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Sergey Rogulenko
2012-07-10 06:34:45 PDT
Created attachment 151453 [details]
Patch
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. 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.
Created attachment 151484 [details]
Patch
It is not clear what this is about. Please add a description and/or mock up. 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.
Created attachment 151685 [details]
Patch
Created attachment 151706 [details]
Patch
Created attachment 151894 [details]
Patch
Created attachment 151920 [details]
Patch
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 Comment on attachment 151920 [details]
Patch
r- as per our discussion offline - lets reuse existing DOM highlight plumbing.
Created attachment 152280 [details]
Patch
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)?
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. Ugh, sorry for all the typos earlier. Comment on attachment 152280 [details]
Patch
r- as per Timothy's comments.
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? Created attachment 152504 [details]
Patch
Created attachment 152506 [details]
Patch
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? Created attachment 152523 [details]
Patch
Comment on attachment 152523 [details] Patch Attachment 152523 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13239949 Comment on attachment 152523 [details] Patch Attachment 152523 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13243967 Comment on attachment 152523 [details] Patch Attachment 152523 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13254249 Created attachment 152526 [details]
Patch
Comment on attachment 152526 [details] Patch Attachment 152526 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13239956 Comment on attachment 152526 [details] Patch Attachment 152526 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13254253 Comment on attachment 152526 [details] Patch Attachment 152526 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13248544 Comment on attachment 152526 [details] Patch Attachment 152526 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13239957 Comment on attachment 152526 [details] Patch Attachment 152526 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13245901 Comment on attachment 152526 [details] Patch Attachment 152526 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13238941 Created attachment 152534 [details]
Patch
Created attachment 152545 [details]
Patch
Comment on attachment 152545 [details] Patch Attachment 152545 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13233984 Created attachment 152553 [details]
Patch
Created attachment 152558 [details]
Patch
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? Created attachment 152567 [details]
Patch
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? Created attachment 152744 [details]
Patch
Comment on attachment 152744 [details]
Patch
LGTM, though I liked the previous approach where highlight was mutually exclusive with pause screen.
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. Created attachment 152783 [details]
Patch
Comment on attachment 152783 [details] Patch Attachment 152783 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13281215 Comment on attachment 152783 [details] Patch Attachment 152783 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13274185 Comment on attachment 152783 [details] Patch Attachment 152783 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13274186 Comment on attachment 152783 [details] Patch Attachment 152783 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13272192 Created attachment 152787 [details]
Patch
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 Created attachment 152976 [details]
Patch
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? Created attachment 152994 [details]
Patch
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 Created attachment 153003 [details]
Patch
Committed r122966: <http://trac.webkit.org/changeset/122966> Comment on attachment 153003 [details]
Patch
Clearing r?
|