Bug 91592 – [EFL] Highlight the element under mouse on web inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Seokju Kwon]

Implement highlighting the nodes when using the inspector

[Seokju Kwon]

Created attachment 152947 [details]
Patch

[Chris Dumez]

Comment on attachment 152947 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:110
> +    hideHighlight();

This looks a bit odd this way. Maybe we should move the code in hideHighlight() to a new invalidateView(inspectedView) method and call this new function is both highlight() and hideHighlight()? This would be more understandable for me.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:118
> +        WebCore::IntRect rect(0, 0, width, height);

I would create this as late as possible, that is to say: after the NULL page check.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:122
> +            page->mainFrame()->view()->invalidateRect(rect);

page->mainFrame()->view()->invalidateRect(WebCore::IntRect(0, 0, width, height)); ?

> Source/WebKit/efl/ewk/ewk_paint_context.cpp:181
> +        if (controller && controller->highlightedNode())

page->inspectorController() cannot return NULL so the first check is not needed. Use an ASSERT() instead if you want to be safe.

[Seokju Kwon]

(In reply to comment #2)
> (From update of attachment 152947 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152947&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:110
> > +    hideHighlight();
> 
> This looks a bit odd this way. Maybe we should move the code in hideHighlight() to a new invalidateView(inspectedView) method and call this new function is both highlight() and hideHighlight()? This would be more understandable for me.
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:118
> > +        WebCore::IntRect rect(0, 0, width, height);
> 
> I would create this as late as possible, that is to say: after the NULL page check.
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:122
> > +            page->mainFrame()->view()->invalidateRect(rect);
> 
> page->mainFrame()->view()->invalidateRect(WebCore::IntRect(0, 0, width, height)); ?
> 
> > Source/WebKit/efl/ewk/ewk_paint_context.cpp:181
> > +        if (controller && controller->highlightedNode())
> 
> page->inspectorController() cannot return NULL so the first check is not needed. Use an ASSERT() instead if you want to be safe.

I will fix all things you mentioned. Thanks for reviewing.

[Seokju Kwon]

Created attachment 154224 [details]
Patch

[Chris Dumez]

Comment on attachment 154224 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.h:65
> +    void invalidateView(Evas_Object*);

I don't think invalidateView() needs to be public. I would have just used a static function in the cpp file.

[Gyuyoung Kim]

Comment on attachment 154224 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_paint_context.cpp:23
> +#if ENABLE(INSPECTOR)

EFL port has used ENABLE(...) below regular include list.

Please see ewk_view.cpp.
http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.cpp#L81

[Seokju Kwon]

(In reply to comment #6)
> (From update of attachment 154224 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154224&action=review
> 
> > Source/WebKit/efl/ewk/ewk_paint_context.cpp:23
> > +#if ENABLE(INSPECTOR)
> 
> EFL port has used ENABLE(...) below regular include list.
> 
> Please see ewk_view.cpp.
> http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.cpp#L81

you're right. I will move it.

[Seokju Kwon]

(In reply to comment #5)
> (From update of attachment 154224 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154224&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.h:65
> > +    void invalidateView(Evas_Object*);
> 
> I don't think invalidateView() needs to be public. I would have just used a static function in the cpp file.

It makes sense.

[Seokju Kwon]

Created attachment 154473 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 154473 [details]
Patch

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

The rest looks good.

> Source/WebKit/efl/ewk/ewk_paint_context.cpp:181
> +        WebCore::InspectorController* controller = page->inspectorController();

I wonder if page always has inspector controller. If not, I think below condition is good to avoid crash.

if (WebCore::InspectorController* controller = page->inspectorController())
    if (controller->highlightedNode())
        controller->drawHighlight(*context->graphicContext);

[Seokju Kwon]

(In reply to comment #10)
> (From update of attachment 154473 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154473&action=review
> 
> The rest looks good.
> 
> > Source/WebKit/efl/ewk/ewk_paint_context.cpp:181
> > +        WebCore::InspectorController* controller = page->inspectorController();
> 
> I wonder if page always has inspector controller. If not, I think below condition is good to avoid crash.
> 
> if (WebCore::InspectorController* controller = page->inspectorController())
>     if (controller->highlightedNode())
>         controller->drawHighlight(*context->graphicContext);

It seems to me that page always has inspectorController.

[Gyuyoung Kim]

Comment on attachment 154473 [details]
Patch

LGTM now.

[Chris Dumez]

Comment on attachment 154473 [details]
Patch

LGTM.

[Kentaro Hara]

Comment on attachment 154473 [details]
Patch

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

Looks OK. rs=me

>>> Source/WebKit/efl/ewk/ewk_paint_context.cpp:181
>>> +        WebCore::InspectorController* controller = page->inspectorController();
>> 
>> I wonder if page always has inspector controller. If not, I think below condition is good to avoid crash.
>> 
>> if (WebCore::InspectorController* controller = page->inspectorController())
>>     if (controller->highlightedNode())
>>         controller->drawHighlight(*context->graphicContext);
> 
> It seems to me that page always has inspectorController.

Looks so to me too.

[WebKit Review Bot]

Comment on attachment 154473 [details]
Patch

Clearing flags on attachment: 154473

Committed r123831: <http://trac.webkit.org/changeset/123831>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.