Summary: | QtTestBrowser should provide way to clear selected elements | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vivek Galatage <vivekg> | ||||||||
Component: | WebKit Qt | Assignee: | 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
Vivek Galatage
2013-01-21 03:38:14 PST
Created attachment 183754 [details]
Patch
Created attachment 183755 [details]
Patch
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. Created attachment 183758 [details]
Patch
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 on attachment 183758 [details] Patch Clearing flags on attachment: 183758 Committed r140323: <http://trac.webkit.org/changeset/140323> All reviewed patches have been landed. Closing bug. |