[webkit-reviews] review denied: [Bug 35125] [Qt] Web Inspector Does Not Highlight Elements : [Attachment 74311] Patch 01

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Ragner Magalhaes
<ragner.magalhaes at openbossa.org>'s request for review:
Bug 35125: [Qt] Web Inspector Does Not Highlight Elements
https://bugs.webkit.org/show_bug.cgi?id=35125

Attachment 74311: Patch 01
https://bugs.webkit.org/attachment.cgi?id=74311&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74311&action=review

r- because of the rect issue Antonio pointed out and because of the save() /
restore() that should be optimized to not be executed in the common case when
there is no highlighted node. Otherwise the patch is going in the right
direction I'd say :)

> WebKit/qt/Api/qwebframe.cpp:415
> +#if ENABLE(INSPECTOR)
> +    context->save();
> +    frame->page()->inspectorController()->drawNodeHighlight(*context);
> +    context->restore();
> +#endif

It appears that this code is in QWebFramePrivate::renderRelativeCoords. In the
_common_ case we would now end up calling save() and restore() on the graphics
context each time we render content, where _most_ of the time
drawNodeHighlight() returns immediately. save() and restore() are expensive
operations we should avoid if necessary.

I suggest to either add a function tin InspectorController that allows us to
perform a check here to see if we need to call drawNodeHighlight() and only
then do save() / restore() or alternatively change the implementation of
drawNodeHighligh() to do the save() / restore() pair.