Bug 107437

Summary: QtTestBrowser should provide way to clear selected elements
Product: WebKit Reporter: Vivek Galatage <vivekg>
Component: WebKit QtAssignee: Vivek Galatage <vivek.vg>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, ossy, vivek.vg, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Vivek Galatage 2013-01-21 03:38:14 PST
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.
Comment 1 Vivek Galatage 2013-01-21 03:46:00 PST
Created attachment 183754 [details]
Patch
Comment 2 Vivek Galatage 2013-01-21 03:51:55 PST
Created attachment 183755 [details]
Patch
Comment 3 Simon Hausmann 2013-01-21 03:57:40 PST
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.
Comment 4 Vivek Galatage 2013-01-21 04:06:00 PST
Created attachment 183758 [details]
Patch
Comment 5 Vivek Galatage 2013-01-21 04:08:52 PST
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.
Comment 6 WebKit Review Bot 2013-01-21 04:46:51 PST
Comment on attachment 183758 [details]
Patch

Clearing flags on attachment: 183758

Committed r140323: <http://trac.webkit.org/changeset/140323>
Comment 7 WebKit Review Bot 2013-01-21 04:46:54 PST
All reviewed patches have been landed.  Closing bug.