Bug 69015 - Web Inspector: Add support for search in script content.
Summary: Web Inspector: Add support for search in script content.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on: 69265 69326
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-28 12:14 PDT by Vsevolod Vlasov
Modified: 2011-10-05 07:29 PDT (History)
16 users (show)

See Also:


Attachments
Patch (14.51 KB, patch)
2011-09-29 06:15 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (21.45 KB, patch)
2011-09-30 08:29 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (29.85 KB, patch)
2011-10-04 04:26 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (29.68 KB, patch)
2011-10-04 07:25 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (29.92 KB, patch)
2011-10-04 09:55 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (29.78 KB, patch)
2011-10-04 11:32 PDT, Vsevolod Vlasov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-09-28 12:14:42 PDT
Add support for backend search in script content.
Comment 1 Vsevolod Vlasov 2011-09-29 06:15:55 PDT
Created attachment 109156 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-29 06:19:38 PDT
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.
Comment 3 Pavel Feldman 2011-09-29 09:37:42 PDT
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.
Comment 4 Vsevolod Vlasov 2011-09-30 08:29:19 PDT
Created attachment 109301 [details]
Patch
Comment 5 WebKit Review Bot 2011-09-30 08:31:56 PDT
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.
Comment 6 Pavel Feldman 2011-10-02 10:27:16 PDT
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
Comment 7 Vsevolod Vlasov 2011-10-03 05:56:09 PDT
Committed r96491: <http://trac.webkit.org/changeset/96491>
Comment 8 Fehér Zsolt 2011-10-03 07:00:08 PDT
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
Comment 9 Csaba Osztrogonác 2011-10-03 09:36:34 PDT
New bug report filed for Qt fails: https://bugs.webkit.org/show_bug.cgi?id=69265
Comment 12 Vsevolod Vlasov 2011-10-04 04:26:39 PDT
Created attachment 109605 [details]
Patch
Comment 13 WebKit Review Bot 2011-10-04 04:28:57 PDT
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.
Comment 14 Csaba Osztrogonác 2011-10-04 04:30:32 PDT
Reopen the bug.
Comment 15 Pavel Feldman 2011-10-04 06:55:20 PDT
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;
Comment 16 Vsevolod Vlasov 2011-10-04 07:25:29 PDT
Created attachment 109620 [details]
Patch
Comment 17 WebKit Review Bot 2011-10-04 07:28:19 PDT
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.
Comment 18 Pavel Podivilov 2011-10-04 08:45:02 PDT
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.
Comment 19 Vsevolod Vlasov 2011-10-04 09:55:45 PDT
Created attachment 109633 [details]
Patch
Comment 20 WebKit Review Bot 2011-10-04 09:59:30 PDT
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.
Comment 21 WebKit Review Bot 2011-10-04 10:21:04 PDT
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
Comment 22 Pavel Podivilov 2011-10-04 10:41:48 PDT
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.
Comment 23 Vsevolod Vlasov 2011-10-04 11:32:17 PDT
Created attachment 109659 [details]
Patch
Comment 24 WebKit Review Bot 2011-10-04 11:35:48 PDT
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.
Comment 25 Ryosuke Niwa 2011-10-04 11:41:31 PDT
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.
Comment 26 Vsevolod Vlasov 2011-10-04 12:26:03 PDT
> 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.
Comment 27 Ryosuke Niwa 2011-10-04 13:09:00 PDT
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?
Comment 28 Vsevolod Vlasov 2011-10-04 13:27:23 PDT
(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.
Comment 29 Ryosuke Niwa 2011-10-04 13:29:08 PDT
(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.
Comment 30 Pavel Podivilov 2011-10-05 02:43:05 PDT
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.
Comment 31 Vsevolod Vlasov 2011-10-05 07:29:51 PDT
Committed r96703: <http://trac.webkit.org/changeset/96703>