Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2672083004:
[DevTools] Search in Elements tab is not working for newly added elements (Closed)
|
Created:
3 years, 10 months ago by Oleksii Kadurin Modified:
3 years, 10 months ago Reviewers:
pfeldman 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, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Search in Elements tab is not working for newly added elements
BUG=679850
Review-Url: https://codereview.chromium.org/2672083004
Cr-Commit-Position: refs/heads/master@{#450236}
Committed: https://chromium.googlesource.com/chromium/src/+/8d7a71b75fff2d64a8cc192295fcdb4eb03987f4
Patch Set 1 #
Total comments: 7
Patch Set 2 #
Total comments: 9
Patch Set 3 #
Total comments: 1
Patch Set 4 #
Total comments: 3
Patch Set 5 #
Total comments: 1
Patch Set 6 #
Total comments: 1
Patch Set 7 #
Messages
Total messages: 47 (29 generated)
ovkadurin@gmail.com changed reviewers: + lushnikov@chromium.org, pfeldman@chromium.org
ovkadurin@gmail.com changed reviewers: + lushnikov@chromium.org, pfeldman@chromium.org
PTAL
PTAL
https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/.editorconfig (right): https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/.editorconfig:5: indent_size = 2 you can tell we don't use this one :) https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:580: if (!this._searchResults) Can we instead just call performSearch(sameConfig, true, false) here? And there you never reset this._currentSearchResultIndex or reset it only when config changes. https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:589: if (!this._searchResults) performSearch(sameConfig, true, true) https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js (right): https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js:543: performSearch(searchConfig, shouldJump, jumpBackwards, reset) {}, This makes our contract a bit more redundant - I would say that when you need to reset, you could just call searchCanceled.
* I've got rid of "reset" parameter. * Also I wrote some comments. https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/.editorconfig (right): https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/.editorconfig:5: indent_size = 2 On 2017/02/06 19:24:53, pfeldman wrote: > you can tell we don't use this one :) I understand that this fix isn't related to the current issue. However it will make the development much more easier on IDE :) So if you ask to remove it I will do it. https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:580: if (!this._searchResults) On 2017/02/06 19:24:53, pfeldman wrote: > Can we instead just call performSearch(sameConfig, true, false) here? And there > you never reset this._currentSearchResultIndex or reset it only when config > changes. If you are talking about resetting this._currentSearchResultIndex in _jumpToSearchResult then I do it for cases when the number of found items decreased dynamically. For instance, if the current index is 10, but this._searchResults.length is 5, then we need to reset the index. Hence I think tracking sameConfig will not help in catching such cases. https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js (right): https://codereview.chromium.org/2672083004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js:543: performSearch(searchConfig, shouldJump, jumpBackwards, reset) {}, On 2017/02/06 19:24:53, pfeldman wrote: > This makes our contract a bit more redundant - I would say that when you need to > reset, you could just call searchCanceled. I removed the "reset" parameter. And now I'm using this._currentSearchResultIndex as a reset marker. That value is "undefined" just after calling of .searchCanceled(). https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js (right): https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js:355: this._searchProvider.searchCanceled(); Reset search by explicit invocation of .searchCanceled() https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js:520: this._searchProvider.searchCanceled(); Reset search by explicit invocation of .searchCanceled()
https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:564: if (index > this._searchResults.length) This should never happen. Whenever we update searchResults, we should cap the index. https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:644: this._currentSearchResultIndex > this._searchResults.length - 1) ditto https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js (right): https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js:355: this._searchProvider.searchCanceled(); This might come unexpected to the provider, why do we need it here? https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js:520: this._searchProvider.searchCanceled(); On 2017/02/07 20:20:24, Oleksii Kadurin wrote: > Reset search by explicit invocation of .searchCanceled() That seems fine.
https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js (right): https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js:355: this._searchProvider.searchCanceled(); On 2017/02/07 23:19:36, pfeldman wrote: > This might come unexpected to the provider, why do we need it here? It was the part of performSearch function (in the ElementsPanel.js file 467). I moved that call outside the function. And call it every time the reset is needed. It is also initialization/reset for this._currentSearchResultIndex when a user hit Ctrl + F. https://codereview.chromium.org/2672083004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (left): https://codereview.chromium.org/2672083004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:556: this._hideSearchHighlights(); Removed this one because from now every request for the next/prev item should go through performSearch(). And hiding of the current item is a part of performSearch(). Also there are function like jumpToNextSearchResult() and umpToPreviousSearchResult(), that call _jumpToSearchResult directly. However those functions are not invoked at all. They were invoked before, when there were a find button in the search panel and some checkbox. But now those are hidden all the time.
https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js (right): https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js:355: this._searchProvider.searchCanceled(); Right, but you are changing this contract for all the SearchableViews now, which might be unexpected to respective providers: https://cs.chromium.org/search/?q=UI.SearchableView%5C(+package:%5Echromium$&... Your change should only touch ElementsPanel it seems.
https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js (right): https://codereview.chromium.org/2672083004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SearchableView.js:355: this._searchProvider.searchCanceled(); On 2017/02/08 01:52:41, pfeldman wrote: > Right, but you are changing this contract for all the SearchableViews now, which > might be unexpected to respective providers: > > https://cs.chromium.org/search/?q=UI.SearchableView%5C(+package:%5Echromium$&... > > Your change should only touch ElementsPanel it seems. I've got it now. Thanks!
PTAL
https://codereview.chromium.org/2672083004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2672083004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:504: if (this._currentSearchResultIndex === undefined || this._currentSearchResultIndex > this._searchResults.length) should this be this._currentSearchResultIndex >= this._searchResults.length? https://codereview.chromium.org/2672083004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:510: if (index === -1) It seems like if this._currentSearchResultIndex was set to 0 instead of -1 above, you would not need this branch, it would be: this._jumpToSearchResult(this._currentSearchResultIndex + (jumpBackwards ? - 1 : 1));
ovkadurin@gmail.com changed reviewers: - lushnikov@chromium.org
https://codereview.chromium.org/2672083004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2672083004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:510: if (index === -1) On 2017/02/11 01:46:24, pfeldman wrote: > It seems like if this._currentSearchResultIndex was set to 0 instead of -1 > above, you would not need this branch, it would be: > > this._jumpToSearchResult(this._currentSearchResultIndex + (jumpBackwards ? - 1 : > 1)); If it's a "reset" condition and this._currentSearchResultIndex = 0 (as you suggest) and jumpBackwards = false, then this._jumpToSearchResult will get "1" as an index. It means that in the search textbox the current value will be "2" (it adds +1 for index). And the label will be f.e. "2 of 6". That's no ok for "reset" case. Cause it should be "1 of 6".
lgtm % comment https://codereview.chromium.org/2672083004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2672083004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:510: if (index === -1) Let's replace this with this._currentSearchResultIndex === undefined for clarity then and reset it to undefined above. That way we encode missing selection as "this._currentSearchResultIndex === undefined". Today we encode it with "this._currentSearchResultIndex === undefined" (first time and in searchCanceled) and this._currentSearchResultIndex = -1 here.
PTAL
still lgtm https://codereview.chromium.org/2672083004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2672083004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:504: if (this._currentSearchResultIndex !== undefined && this._currentSearchResultIndex >= this._searchResults.length) nit: drop the this._currentSearchResultIndex !== undefined part.
The CQ bit was checked by ovkadurin@gmail.com
The CQ bit was unchecked by ovkadurin@gmail.com
The CQ bit was checked by ovkadurin@gmail.com
The CQ bit was unchecked by ovkadurin@gmail.com
The CQ bit was checked by ovkadurin@gmail.com to run a CQ dry run
The CQ bit was unchecked by ovkadurin@gmail.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ovkadurin@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ovkadurin@gmail.com
The CQ bit was checked by ovkadurin@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2672083004/#ps120001 (title: "")
The CQ bit was checked by ovkadurin@gmail.com
The CQ bit was unchecked by ovkadurin@gmail.com
The CQ bit was checked by ovkadurin@gmail.com to run a CQ dry run
The CQ bit was unchecked by ovkadurin@gmail.com
The CQ bit was checked by ovkadurin@gmail.com
The CQ bit was unchecked by ovkadurin@gmail.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ovkadurin@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pfeldman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487044772693410,
"parent_rev": "9775bfee2b12b717b138928078f95ff524af5564", "commit_rev":
"8d7a71b75fff2d64a8cc192295fcdb4eb03987f4"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Search in Elements tab is not working for newly added elements BUG=679850 ========== to ========== [DevTools] Search in Elements tab is not working for newly added elements BUG=679850 Review-Url: https://codereview.chromium.org/2672083004 Cr-Commit-Position: refs/heads/master@{#450236} Committed: https://chromium.googlesource.com/chromium/src/+/8d7a71b75fff2d64a8cc192295fc... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/8d7a71b75fff2d64a8cc192295fc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
