[webkit-reviews] review canceled: [Bug 21095] Search should jump in all matches in Elements panel : [Attachment 136459] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has canceled Sammy
<83.samarth at gmail.com>'s request for review:
Bug 21095: Search should jump in all matches in Elements panel
https://bugs.webkit.org/show_bug.cgi?id=21095

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=136459&action=review


> Source/WebCore/ChangeLog:6
> +	   The fix calculates number of matches within the node as well while

Make sure you address the style bot comments on the tab characters here.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:801
> +void InspectorDOMAgent::populateNodeResults(Vector<Node*>& resultCollector,
Node* node, const String& searchString, const String& queryString)

I don't think it should be public, I would not even mention it in the header.
What I was suggesting was a static function within cpp:

static void populateNodeResults(...)


I don't really like the changes to the semantics of the protocol that happen
here: performSearch returns the number of matches, getSearchResults returns
ranges of matches. But for the node that contains multiple instances of "foo",
we will get a number of identical results in the range. Given that
getSearchResults is hidden in the protocol, we could tweak it to optionally
return ordinal of the text occurrence within the node. I.e. each search match
is not a nodeId, but the {nodeId: NodeId, ordinal: number} pair. Then
traversing the results in the front-end would be much more straightforward and
other clients would be able to leverage the strictness.

> Source/WebCore/inspector/front-end/ElementsPanel.js:475
> +	   this.searchResultJumpDirection = "next";    

We prefix private fields with "_".