Issue 1899893003: [Devtools] JSONView implements Searchable interface

Issue 1899893003: [Devtools] JSONView implements Searchable interface (Closed)

Created:
4 years, 8 months ago by allada

Modified:
4 years, 7 months ago

Reviewers:
lushnikov

CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[Devtools] JSONView implements Searchable interface WebInspector.JSONView now implements WebInspector.Searchable interface and allows it to be searchable (with search bar) in network panel. BUG=604829 R=lushnikov Committed: https://crrev.com/9b759d6d6898f784a912b11278039425fbad8415 Cr-Commit-Position: refs/heads/master@{#392169}

Patch Set 1 : #

Total comments: 14

Patch Set 2 : #

Total comments: 23

Patch Set 3 : #

Total comments: 11

Patch Set 4 : #

Created: 4 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+445 lines, -24 lines)			Patch
M	third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview.html	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview-expected.txt	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/http/tests/inspector/network/network-choose-preview-view.html	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/http/tests/inspector/network/network-choose-preview-view-expected.txt	View	1 2	1 chunk	+12 lines, -3 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html	View	1 2 3	1 chunk	+122 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable-expected.txt	View	1 2	1 chunk	+67 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js	View	1 2 3	1 chunk	+46 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js	View	1 2	3 chunks	+3 lines, -5 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/network/JSONView.js	View	1 2 3	3 chunks	+168 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js	View	1	4 chunks	+9 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js	View	1 2	2 chunks	+11 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download

Dependent Patchsets:

Issue 1942683006 Patch 260001

Issue 1942683006 Patch 280001

Messages

Total messages: 36 (18 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

lushnikov

some intermediate comments https://codereview.chromium.org/1899893003/diff/140001/third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview.html File third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview.html (right): https://codereview.chromium.org/1899893003/diff/140001/third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview.html#newcode23 third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview.html:23: InspectorTest.addResult("Its previewer is searchable: " + ...

4 years, 8 months ago (2016-04-22 19:43:00 UTC) #9

allada

PTL https://codereview.chromium.org/1899893003/diff/140001/third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview.html File third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview.html (right): https://codereview.chromium.org/1899893003/diff/140001/third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview.html#newcode23 third_party/WebKit/LayoutTests/http/tests/inspector/network/json-preview.html:23: InspectorTest.addResult("Its previewer is searchable: " + (previewer && ...

4 years, 8 months ago (2016-04-25 23:48:49 UTC) #12

lushnikov

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js#newcode225 third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:225: setSearchRegex: function(regex, additionalCssClassName) { Just to make sure: some ...

4 years, 8 months ago (2016-04-26 19:22:03 UTC) #13

allada

PTL https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js#newcode225 third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:225: setSearchRegex: function(regex, additionalCssClassName) { On 2016/04/26 19:22:03, lushnikov ...

4 years, 7 months ago (2016-04-27 21:19:57 UTC) #15

PTL

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:225:
setSearchRegex: function(regex, additionalCssClassName) {
On 2016/04/26 19:22:03, lushnikov wrote:
> Just to make sure: some time ago you planned to introduce similar API to the
> TreeElement class. Whenever it will be done, these methods will override the
> ones from treelement?

The patch you are referring to was rolled back and we decided to go a different
route, so right now they do not interact with each other.

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/JSONView.js (right):

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:52:
this._searchRegEx = null;
On 2016/04/26 19:22:03, lushnikov wrote:
> search front_end/ for "RegEx\b": 4 occurrences
> search front_end/ for "Regex\b": 206 occurrences
> 
> Let's name this "_searchRegex" - to be consistent with the codebase.

Done.

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:181:
_jumpToMatch: function(index)
On 2016/04/26 19:22:03, lushnikov wrote:
> let's have a contract that _jumpToMatch always has a valid index

It needs to be able to be passed with -1 in order to know it needs to start from
beginning (not 0). At line 189 it sets it to -1 if called out of range.

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:185: var
previousIndex = this._currentSearchFocusIndex;
On 2016/04/26 19:22:03, lushnikov wrote:
> no need for the variable

Done.

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:258: var
focusNextSearchResult = newIndex === -1 ? true : false;
On 2016/04/26 19:22:03, lushnikov wrote:
> var focusNextSearchResult = newIndex === -1;

Done.

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:262: var
hasMatch = currentElement.setSearchRegex(this._searchRegEx);
On 2016/04/26 19:22:03, lushnikov wrote:
> how will it work if there are multiple matches in currentElement?

It considers multiple matches in the same element as a count of 1, but will
highlight each one separate (similar to how elements panel works).

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:267: if
(focusNextSearchResult && hasMatch) {
On 2016/04/26 19:22:03, lushnikov wrote:
> Why do we need this? I don't follow this logic

It keeps track of the current highlighted node with the index of it's position
in this._currentSearchTreeElements. Since this function is rebuilding
this._currentSearchTreeElements, this is how it finds where the last selected
node is and considers the index of the last found item the new anchor index to
perform jumpToNextSearchResult or jumpToPreviousSearchResult.

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:294: var
currentIndex = this._currentSearchFocusIndex;
On 2016/04/26 19:22:03, lushnikov wrote:
> var newIndex = mod(this._currentSearchFocusIndex + 1,
> this._currentSearchTreeElements.length)
> this._jumpToMatch(newIndex)

Done.

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:307: var
currentIndex = this._currentSearchFocusIndex;
On 2016/04/26 19:22:03, lushnikov wrote:
> var newIndex = mod(this._currentSearchFocusIndex - 1,
> this._currentSearchTreeElements.length)
> this._jumpToMatch(newIndex);

Done.

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js
(right):

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:152:
callback(xmlView);
On 2016/04/26 19:22:03, lushnikov wrote:
> looks like a fix for a regression! Let's land this fix separately.

Done.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1899893003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899893003/240001

4 years, 7 months ago (2016-04-27 21:20:32 UTC) #17

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 7 months ago (2016-04-27 23:04:45 UTC) #19

allada

https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Source/devtools/front_end/network/JSONView.js File third_party/WebKit/Source/devtools/front_end/network/JSONView.js (right): https://codereview.chromium.org/1899893003/diff/200001/third_party/WebKit/Source/devtools/front_end/network/JSONView.js#newcode267 third_party/WebKit/Source/devtools/front_end/network/JSONView.js:267: if (focusNextSearchResult && hasMatch) { On 2016/04/27 21:19:57, Blaise ...

4 years, 7 months ago (2016-05-05 23:01:35 UTC) #23

lushnikov

Looks good! https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html File third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html (right): https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html#newcode43 third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html:43: typeName = "XMLView"; these should be wrapped ...

4 years, 7 months ago (2016-05-06 18:46:26 UTC) #25

allada

PTL https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html File third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html (right): https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html#newcode43 third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html:43: typeName = "XMLView"; On 2016/05/06 18:46:25, lushnikov wrote: ...

4 years, 7 months ago (2016-05-06 19:31:51 UTC) #26

PTL

https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html
(right):

https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/network/preview-searchable.html:43:
typeName = "XMLView";
On 2016/05/06 18:46:25, lushnikov wrote:
> these should be wrapped in { } per style guide

Done.

https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:216:
this._highlightChanges = [];
On 2016/05/06 18:46:25, lushnikov wrote:
> let's annotate

Done.

https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/JSONView.js (right):

https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:145:
setSearchableView: function(view)
On 2016/05/06 18:46:25, lushnikov wrote:
> Could this be made private?

Done. But I did it because I was following how SourcesView handles it.

https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:189: }
On 2016/05/06 18:46:25, lushnikov wrote:
> I think _jumpToMatch function could be structured a little bit more
> straightforward:
> 
> if (!this._searchRegex)
>     return;
> 
> var previousFocusElement = ...;
> if (previousFocusElement)
>     previousFocusElement.setSearchRegex(..);
> 
> var newFocusElement = ...;
> if (newFocusElement) {
>     newFocusElement.setSearchRegex(..)
>     newFocusElement.reveal();
>     this._updateSearchIndex(index);
> } else {
>     this._updateSearchIndex(0);
> }

Done.

https://codereview.chromium.org/1899893003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/JSONView.js:244: if
(!(element instanceof WebInspector.ObjectPropertyTreeElement))
On 2016/05/06 18:46:25, lushnikov wrote:
> which other type could it be?

Root node has a special type. Also the
WebInspector.ObjectPropertyTreeElement.populateWithProperties (it's part of the
constructor/initializer stack) function may append:
WebInspector.FunctionScopeMainTreeElement and
WebInspector.CollectionEntriesMainTreeElement.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1899893003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899893003/300001

4 years, 7 months ago (2016-05-06 19:56:32 UTC) #29

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 7 months ago (2016-05-06 21:33:34 UTC) #31

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1899893003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899893003/300001

4 years, 7 months ago (2016-05-06 21:56:21 UTC) #33

commit-bot: I haz the power

4 years, 7 months ago (2016-05-06 22:03:28 UTC) #36

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/9b759d6d6898f784a912b11278039425fbad8415
Cr-Commit-Position: refs/heads/master@{#392169}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages