Bug 91632 – Web Inspector: extracting HighlightInfo from HighlightData in DOMNodeHighlighter

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Sergey Rogulenko]

Extracting HighlightInfo from HighlightData in DOMNodeHighlighter.

[Sergey Rogulenko]

Created attachment 153043 [details]
Patch

[Andrey Kosyakov]

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.

[Sergey Rogulenko]

Created attachment 153267 [details]
Patch

[Sergey Rogulenko]

Created attachment 155302 [details]
Patch

[Alexander Pavlov (apavlov)]

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?

[Sergey Rogulenko]

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.

[Pavel Feldman]

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.

[Sergey Rogulenko]

Created attachment 155462 [details]
Patch

[Pavel Feldman]

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.

[Sergey Rogulenko]

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?

[Sergey Rogulenko]

Created attachment 155757 [details]
Patch

[Pavel Feldman]

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"

[Sergey Rogulenko]

Created attachment 156393 [details]
Patch

[Pavel Feldman]

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

[Sergey Rogulenko]

Created attachment 156627 [details]
Patch

[Pavel Feldman]

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, ...)

[Sergey Rogulenko]

Created attachment 156641 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156627 [details]
Patch

Clearing flags on attachment: 156627

Committed r124749: <http://trac.webkit.org/changeset/124749>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.