Extracting HighlightInfo from HighlightData in DOMNodeHighlighter.
Created attachment 153043 [details] Patch
Comment on attachment 153043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153043&action=review > Source/WebCore/inspector/DOMNodeHighlighter.cpp:538 > + if (highlightInfo) { So we're clearing highlight when passed a null highlightInfo. Can we instead have a separate method for that? > Source/WebCore/inspector/DOMNodeHighlighter.cpp:551 > + m_highlightData->highlightInfo = adoptPtr(new HighlightInfo(*highlightInfo)); Does highlightInfo member have to be a pointer? > Source/WebCore/inspector/DOMNodeHighlighter.h:51 > +struct HighlightInfo { HighlightInfo is too generic -- can we find a better name? HighlightConfig sounds like a slightly better match.
Created attachment 153267 [details] Patch
Created attachment 155302 [details] Patch
Comment on attachment 155302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155302&action=review > Source/WebCore/inspector/DOMNodeHighlighter.cpp:497 > + if (m_needsUpdateBeforePainting) Why did this appear? What issue does it solve? > Source/WebCore/inspector/DOMNodeHighlighter.cpp:593 > + IntRect visibleRect = IntRect(IntPoint(), frame->view()->visibleContentRect().size()); What was wrong with the old visibleContentRect()? You can use frame->view()->visibleSize() for brevity. > Source/WebCore/inspector/InspectorDOMAgent.cpp:1017 > + return PassOwnPtr<HighlightConfig> (); Will "return 0;" work in this case, like it does for PassRefPtr's?
Comment on attachment 155302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155302&action=review >> Source/WebCore/inspector/DOMNodeHighlighter.cpp:497 >> + if (m_needsUpdateBeforePainting) > > Why did this appear? What issue does it solve? When we scroll the page we do not repaint the whole view. So, if we do not do full repaint, there may appear multiple elementTitles on the screen. So I added this to force repainting of the whole overlay. >> Source/WebCore/inspector/DOMNodeHighlighter.cpp:593 >> + IntRect visibleRect = IntRect(IntPoint(), frame->view()->visibleContentRect().size()); > > What was wrong with the old visibleContentRect()? > You can use frame->view()->visibleSize() for brevity. The problem here could appear when you open a page, scroll down and press pause button. The position of visibleContentRect is (0, y), where y > 0, but the context is configured so that the upper-left corner is (0, 0). So the drawing rect would be painted below its proper place. Maybe it is better to do context.translate? >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1017 >> + return PassOwnPtr<HighlightConfig> (); > > Will "return 0;" work in this case, like it does for PassRefPtr's? I tried return 0, but it didn't compile.
Comment on attachment 155302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155302&action=review > Source/WebCore/inspector/DOMNodeHighlighter.cpp:498 > + update(); re-validating screen from paint is a no-go.
Created attachment 155462 [details] Patch
Comment on attachment 155462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155462&action=review Looks mostly good, few nits inline. > Source/WebCore/inspector/DOMNodeHighlighter.h:60 > +struct HighlightData { 1) I don't think you need highlight data type. 2) I think it is fine to have rect and node assigned at the same time > Source/WebCore/inspector/InspectorDOMAgent.cpp:1016 > + if (!highlightInspectorObject) This should never happen, you should return ErrorString for such cases, > Source/WebCore/inspector/InspectorDOMAgent.cpp:1021 > + highlightInspectorObject->getBoolean("showInfo", &showInfo); I believe we are generating constants for these.
Comment on attachment 155462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155462&action=review >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1021 >> + highlightInspectorObject->getBoolean("showInfo", &showInfo); > > I believe we are generating constants for these. I did not find constants for these (only setShowInfo, setContentColor and so on). Is it OK that InspectorTypeBuilder already has class HighlightConfig?
Created attachment 155757 [details] Patch
Comment on attachment 155757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155757&action=review Looks good. A couple of nits inline. > Source/WebCore/inspector/DOMNodeHighlighter.cpp:565 > + if (!m_highlightNode && !m_highlightRect) I think now node and rect highlighting are independent first class citizens. You should treat them separately (as you treat drawPausedInDebugger) > Source/WebCore/inspector/InspectorDOMAgent.cpp:1009 > + *errorString = "Null pointer for highlightInspectorObject in InspectorDOMAgent::setSearchingForNode"; "Internal error: highlight configuration parameter is missing"
Created attachment 156393 [details] Patch
Comment on attachment 156393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156393&action=review > Source/WebCore/inspector/InspectorDOMAgent.cpp:1009 > + *errorString = "Null pointer for highlightInspectorObject in InspectorDOMAgent::setSearchingForNode"; I think I was commenting on this earlier. > Source/WebCore/inspector/InspectorDOMAgent.cpp:1054 > + *errorString = "Null pointer for highlightInspectorObject in InspectorDOMAgent::highlightNode"; ditto
Created attachment 156627 [details] Patch
Comment on attachment 156627 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156627&action=review > Source/WebCore/inspector/InspectorDOMAgent.cpp:1053 > + if (!highlightInspectorObject) { I think you could move this code into highlightConfigFromInspectorObject(errorString, ...)
Created attachment 156641 [details] Patch
Comment on attachment 156627 [details] Patch Clearing flags on attachment: 156627 Committed r124749: <http://trac.webkit.org/changeset/124749>
All reviewed patches have been landed. Closing bug.