Bug 69015 – Web Inspector: Add support for search in script content.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Vsevolod Vlasov]

Add support for backend search in script content.

[Vsevolod Vlasov]

Created attachment 109156 [details]
Patch

[WebKit Review Bot]

Attachment 109156 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorDebuggerAgent.h:84:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 109156 [details]
Patch

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

> LayoutTests/http/tests/inspector/search/search-in-concatenated-script.html:10
> +    InspectorTest._runAfterResourcesAreCreated(["search-concatenated.html"], step2);

You should wait for resource loading to finish.

> LayoutTests/http/tests/inspector/search/search-in-script.html:10
> +    InspectorTest._runAfterResourcesAreCreated(["search.js"], step2);

ditto

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:326
> +    String content = m_scripts.get(scriptId).source;

You should look it up first.

[Vsevolod Vlasov]

Created attachment 109301 [details]
Patch

[WebKit Review Bot]

Attachment 109301 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorDebuggerAgent.h:84:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 109301 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Web Inspector: Add support for backend search in script content.

The title is a bit misleading since you have front-end pieces in this change as well.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:330
> +    *results = ContentSearchUtils::searchInTextByLines(query, source);

Searching in missing script should be an error.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:351
> +    *scriptSource = source;

ditto

> Source/WebCore/inspector/front-end/Script.js:62
> +            callback(searchMatches);

Please log potential error using console.error

[Vsevolod Vlasov]

Committed r96491: <http://trac.webkit.org/changeset/96491>

[Fehér Zsolt]

Two test failed after r96491
The tests:
http/tests/inspector/search/search-in-concatenated-script.html
Diff:
--- /ramdisk/qt-linux-32-release-qt470/build/layout-test-results/http/tests/inspector/search/search-in-concatenated-script-expected.txt	2011-10-03 06:25:47.786125385 -0700
+++ /ramdisk/qt-linux-32-release-qt470/build/layout-test-results/http/tests/inspector/search/search-in-concatenated-script-actual.txt	2011-10-03 06:25:47.786125385 -0700
@@ -2,14 +2,5 @@
 
 Bug 69015  
 http://127.0.0.1:8000/inspector/search/resources/search-concatenated.html
-Search matches: 
-lineNumber: 3, line: 'function searchTestUniqueString()'
-lineNumber: 6, line: '    // searchTestUniqueString two occurences on the same line searchTestUniqueString'
-lineNumber: 12, line: '    searchTestUniqueString();'
-lineNumber: 13, line: '    // SEARCHTestUniqueString();'
-lineNumber: 18, line: 'function searchTestUniqueString2()'
-lineNumber: 21, line: '    /* searchTestUniqueString two occurences on the same line searchTestUniqueString */ } '
-lineNumber: 21, line: ' function doSomething2() { searchTestUniqueString();'
-lineNumber: 22, line: '    // SEARCHTestUniqueString();'
-
+error: Exception during test execution: TypeError: 'undefined' is not an object

http/tests/inspector/search/search-in-script.html
Diff:
--- /ramdisk/qt-linux-32-release-qt470/build/layout-test-results/http/tests/inspector/search/search-in-script-expected.txt	2011-10-03 06:25:48.538039183 -0700
+++ /ramdisk/qt-linux-32-release-qt470/build/layout-test-results/http/tests/inspector/search/search-in-script-actual.txt	2011-10-03 06:25:48.538039183 -0700
@@ -1,11 +1,5 @@
 Tests script search in inspector debugger agent.
 
 Bug 69015  
-http://127.0.0.1:8000/inspector/search/resources/search.js
-Search matches: 
-lineNumber: 0, line: 'function searchTestUniqueString()'
-lineNumber: 3, line: '    // searchTestUniqueString two occurences on the same line searchTestUniqueString'
-lineNumber: 9, line: '    searchTestUniqueString();'
-lineNumber: 10, line: '    // SEARCHTestUniqueString();'
-
+error: Exception during test execution: TypeError: 'undefined' is not an object

[Csaba Osztrogonác]

New bug report filed for Qt fails: https://bugs.webkit.org/show_bug.cgi?id=69265

[Ryosuke Niwa]

This may have broken script-formatter.html on Snow Leopard:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r96571%20(33608)/inspector/debugger/script-formatter-pretty-diff.html

[Ryosuke Niwa]

Two tests added by this patch are failing on Snow Leopard:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r96571%20(33608)/http/tests/inspector/search/search-in-script-pretty-diff.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r96571%20(33608)/http/tests/inspector/search/search-in-concatenated-script-pretty-diff.html

[Vsevolod Vlasov]

Created attachment 109605 [details]
Patch

[WebKit Review Bot]

Attachment 109605 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorDebuggerAgent.h:84:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Csaba Osztrogonác]

Reopen the bug.

[Pavel Feldman]

Comment on attachment 109605 [details]
Patch

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

> Source/WebCore/inspector/front-end/ContentProviders.js:72
> +        if (!this._sortedScriptsArray) {

You should not cache this result.

> Source/WebCore/inspector/front-end/Script.js:52
> +            this._source = source || "";

You should be explicit about the error and do this._source = error ? "" : source;

[Vsevolod Vlasov]

Created attachment 109620 [details]
Patch

[WebKit Review Bot]

Attachment 109620 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorDebuggerAgent.h:84:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Podivilov]

Comment on attachment 109620 [details]
Patch

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

> Source/WebCore/inspector/front-end/ContentProviders.js:71
> +    {

Please cache the result.

> Source/WebCore/inspector/front-end/ContentProviders.js:84
> +            if (lineNumber > scripts[i].lineOffset || (lineNumber === scripts[i].lineOffset && columnNumber > scripts[i].columnOffset))

I think this function could and should be 2 times shorter, just get rid of lineNumber and columnNumber variables and rewrite this check.

> Source/WebCore/inspector/front-end/ContentProviders.js:119
> +            if (!scriptsLeft) {

Prefer early return.

[Vsevolod Vlasov]

Created attachment 109633 [details]
Patch

[WebKit Review Bot]

Attachment 109633 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorDebuggerAgent.h:84:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 109633 [details]
Patch

Attachment 109633 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9940401

New failing tests:
inspector/debugger/debugger-activation-crash2.html

[Pavel Podivilov]

Comment on attachment 109633 [details]
Patch

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

Looks good.

> Source/WebCore/inspector/front-end/ContentProviders.js:72
> +        if (!this._sortedScriptsArray) {

Prefer early return.

> Source/WebCore/inspector/front-end/ContentProviders.js:81
> +            if (!scripts.length)

RawSourceCode always has at least one script, you don't need this check.

> Source/WebCore/inspector/front-end/ContentProviders.js:88
> +                if (previousScript.endLine > scripts[i].lineOffset || (scripts[i - 1].endLine === scripts[i].lineOffset && columnNumber > scripts[i].columnOffset))

Consider negating this check.

[Vsevolod Vlasov]

Created attachment 109659 [details]
Patch

[WebKit Review Bot]

Attachment 109659 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorDebuggerAgent.h:84:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 109659 [details]
Patch

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

>> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324
>> +void InspectorDebuggerAgent::searchInContent(ErrorString* error, const String& scriptId, const String& query, RefPtr<InspectorArray>* results)
> 
> The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]

We don't use pointers to get values out of a function. Please use a reference instead. r- because of this.

[Vsevolod Vlasov]

> We don't use pointers to get values out of a function. Please use a reference instead. r- because of this.

That's the way communication between frontend and backend is implemented in Inspector.

[Ryosuke Niwa]

Comment on attachment 109659 [details]
Patch

(In reply to comment #26)
> > We don't use pointers to get values out of a function. Please use a reference instead. r- because of this.
> 
> That's the way communication between frontend and backend is implemented in Inspector.

That seems wrong. Why can't we use refenrece?

[Vsevolod Vlasov]

(In reply to comment #27)
> (From update of attachment 109659 [details])
> (In reply to comment #26)
> > > We don't use pointers to get values out of a function. Please use a reference instead. r- because of this.
> > 
> > That's the way communication between frontend and backend is implemented in Inspector.
> 
> That seems wrong. Why can't we use refenrece?

Inspector*Agents are called by InspectorBackendDispatcher which is generated automatically.
This generator was written long ago and could be fixed but this is definitely out of scope of this change.
I filed https://bugs.webkit.org/show_bug.cgi?id=69366 for that.

[Ryosuke Niwa]

(In reply to comment #28)
> Inspector*Agents are called by InspectorBackendDispatcher which is generated automatically.
> This generator was written long ago and could be fixed but this is definitely out of scope of this change.
> I filed https://bugs.webkit.org/show_bug.cgi?id=69366 for that.

Great. Thanks.

[Pavel Podivilov]

Comment on attachment 109659 [details]
Patch

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

Looks good.

> Source/WebCore/inspector/front-end/ContentProviders.js:80
> +        var scriptOpenTagLength = "<script>".length;

Please add _scriptOpenTag static field to ConcatenatedScriptsContentProvider class and replace scriptOpenTagLength with scriptOpenTag.length.

[Vsevolod Vlasov]

Committed r96703: <http://trac.webkit.org/changeset/96703>