Bug 107437 – QtTestBrowser should provide way to clear selected elements

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

[Vivek Galatage]

Steps:
1. Launch QtTestBrowser using ./Tools/Scripts/run-launcher --qt
2. Navigate to "Develop->Select Elements..." menu to enter the name of elements and press enter
3. The elements are highlighted on the page
4. Repeat step 2 with some other element name(valid/invalid element names) and press enter

Expected outcome:
Previously highlighted items should be restored while highlighting new elements if available.

Actual outcome:
Both the elements are hightlighted

Proposed Solution:
Provide a way to "Clear selection" in the "Develop" menu to clear the previous search highlight. 

Patch to follow.

[Vivek Galatage]

Created attachment 183754 [details]
Patch

[Vivek Galatage]

Created attachment 183755 [details]
Patch

[Simon Hausmann]

Comment on attachment 183755 [details]
Patch

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.

[Vivek Galatage]

Created attachment 183758 [details]
Patch

[Vivek Galatage]

Comment on attachment 183755 [details]
Patch

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

Thank you for your comments. Uploaded a new patch.

>> Tools/QtTestBrowser/launcherwindow.cpp:76
>> +struct ElementHighlight {
> 
> How about calling this HighlightedElement?

Done.

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

Done.

>> 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?

Done. Yes it compiles :)

>> Tools/QtTestBrowser/launcherwindow.cpp:838
>> +}
> 
> You should clear the list here.

Done.

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

Done.

[WebKit Review Bot]

Comment on attachment 183758 [details]
Patch

Clearing flags on attachment: 183758

Committed r140323: <http://trac.webkit.org/changeset/140323>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.