[webkit-reviews] review denied: [Bug 107437] QtTestBrowser should provide way to clear selected elements : [Attachment 183755] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Vivek Galatage
<vivek.vg at samsung.com>'s request for review:
Bug 107437: QtTestBrowser should provide way to clear selected elements
https://bugs.webkit.org/show_bug.cgi?id=107437

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

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


> Tools/QtTestBrowser/launcherwindow.cpp:76
> +struct ElementHighlight {

How about calling this HighlightedElement?

> Tools/QtTestBrowser/launcherwindow.cpp:78
> +    QString m_styleValue;

And this field m_oldStyle or m_previousStyle to emphasize the fact that this is
the style from _before_ the element was highlighted.

> Tools/QtTestBrowser/launcherwindow.cpp:825
> +	       ElementHighlight el = {e, e.styleProperty("background-color",
QWebElement::InlineStyle)};

Space needed after { and before }. Does this even compile? I thought semicolon
is needed after the styleProperty() call?

> Tools/QtTestBrowser/launcherwindow.cpp:838
> +}

You should clear the list here.

> Tools/QtTestBrowser/launcherwindow.h:231
> +    QList<ElementHighlight> m_previousHighlightElements;

I suggest to call this m_highlightedElements to stress that this holds the list
of currently highlighted elements, not the ones from the previous selection.