Implement highlighting the nodes when using the inspector
Created attachment 152947 [details] Patch
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.
(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.
Created attachment 154224 [details] Patch
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.
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
(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.
(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.
Created attachment 154473 [details] Patch
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);
(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.
Comment on attachment 154473 [details] Patch LGTM now.
Comment on attachment 154473 [details] Patch LGTM.
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.
Comment on attachment 154473 [details] Patch Clearing flags on attachment: 154473 Committed r123831: <http://trac.webkit.org/changeset/123831>
All reviewed patches have been landed. Closing bug.