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

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied 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 135274: Patch
https://bugs.webkit.org/attachment.cgi?id=135274&action=review

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


My general concern is Gmail-size apps. Gmail ends up having 5Mb of script tags
in its DOM. Searching for "for" or "a" will be creating millions of node ids as
results. We are currently bound with the number of nodes as search results, but
after your change we become bound to the number of characters in DOM. Please
keep it in mind when applying the front-end changes.

> Source/WebCore/ChangeLog:3
> +	   Search should jump in all matches in Elements panel

Please prefix title with "Web Inspector:"

> Source/WebCore/ChangeLog:6
> +	   This patch is to get the correct count of matched values

You could be more specific here.

> Source/WebCore/ChangeLog:11
> +	   No new tests required as this is the backend part of the fix.

You should be able to test this: see
LayoutTests/inspector/elements/elements-panel-search.html

> Source/WebCore/inspector/InspectorDOMAgent.cpp:830
> +		   while ((queryIndex =
text.findIgnoringCase(whitespaceTrimmedQuery, queryIndex + 1)) !=
static_cast<int>(notFound))

I would introduce a static function above this method to re-use the code more
aggressively, something like

populateNodeResults(Vector<Node*>& resultCollector, const String& str)

> Source/WebCore/inspector/InspectorDOMAgent.cpp:890
> +	 resultsIt->second.append(*it);

So you need to have a front-end code that would iterate through individual
matches too. You should make it a part of the same change.