|
|
Created:
6 years, 1 month ago by aknudsen Modified:
5 years, 11 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionNavigate between individual search matches in DevTools console
Rather than the original DevTools console search behaviour, which is to navigate between lines containing search matches, highlight all matches and navigate between individual matches irrespective of containing lines.
Please note that the .current-search-result CSS class should be revised by a designer.
BUG=426366
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188234
Patch Set 1 #
Total comments: 22
Patch Set 2 : Revision #1 #Patch Set 3 : Add test; fix scrolling to current match #Patch Set 4 : Mark current search results by modifying CSS classes #Patch Set 5 : Scroll tree elements into view #Patch Set 6 : Add tests #
Total comments: 102
Patch Set 7 : Highlight all matches per message at once #Patch Set 8 : Rebase on origin/master #Patch Set 9 : Consolidate test suites #
Total comments: 8
Patch Set 10 : Improve test #
Total comments: 7
Patch Set 11 : Rebase on origin/master #Patch Set 12 : Call clearHighlights once per message #
Total comments: 2
Patch Set 13 : Remove comments #Patch Set 14 : Synchronize with origin #
Messages
Total messages: 66 (11 generated)
arve.knudsen@gmail.com changed reviewers: + eustas@chromium.org, rob@robwu.nl, vsevik@chromium.org
I've reviewed your code and glanced over the logic, but I haven't looked at the full picture yet, mainly due to its complexity and the fact that you said that it will be changed later. Reviewing would be easier if you didn't add the debugging code such as console.log. You could remove all debugging code, commit and upload the changes, then use Ctrl+Z (undo) to get your debugging code back so we don't have to look at it (make sure that you verify that the code works without debugging code before committing). And these features surely need some test coverage. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:579: if (this._searchRegex && this._searchMessage(this._visibleViewMessages.length-1)) { Spaces around minus. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:924: } Omit braces for single-line for/if/if-else blocks (this comment applies to all other occurrences of this pattern in your patch). https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:966: matchRange.message]; matchRange.message? As said in the comment below, this is confusing and needs to be change to something more sensible, e.g. matchRange.resultIndex. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:974: _jumpToMatch: function(index) "_jumpToSearchResult" was more descriptive than "_jumpToMatch", in my opinion. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:1011: while (match && match[0]) { The idiomatic way to use .exec in a JS loop is: var match; while ((match = this._searchRegex.exec(text)) !== null && match[0]) { .... } https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:1013: message: index, "message" is confusing. I thought that it was a string when I read it elsewhere. Choose a more appropriate property name, e.g. resultIndex. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:1015: end: match.index + match[0].length-1, Spaces around the minus sign. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:1024: return numMatches > 0; numMatches can be eliminated by using the fact that the matchRange variable is only truthy when the loop runs at least once. return !!matchRange; https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... File Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleViewMessage.js:1280: return this._messageElement.textContent; What's the difference between the text getter (a few lines back) and your new getText method? Why can't you use the previous one? https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/ui... File Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/ui... Source/devtools/front_end/ui/UIUtils.js:787: * @param {?Boolean=} isCurrent Booleans are primitive types, use a lowercase "boolean". ? denotes nullable and {...=} denotes an optional value, you can therefore remove the "=". (side note, the ! and = at the previous line doesn't make sense either, it says non-nullable AND optional. That doesn't fit together). https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/ui... Source/devtools/front_end/ui/UIUtils.js:792: var style = isCurrent ? "current-search-result" : "highlighted-search-result"; Rename "style" to className.
On 2014/10/25 23:13:00, robwu wrote: > I've reviewed your code and glanced over the logic, but I haven't looked at the > full picture yet, mainly due to its complexity and the fact that you said that > it will be changed later. > Reviewing would be easier if you didn't add the debugging code such as > console.log. You could remove all debugging code, commit and upload the changes, > then use Ctrl+Z (undo) to get your debugging code back so we don't have to look > at it (make sure that you verify that the code works without debugging code > before committing). OK, I'll keep it in mind for the future. Are there no facilities at all for inserting debug log calls in DevTools though? I find them extremely useful during development/fixing. I think you can now consider the code ready as far as I am concerned, I don't see anything that needs improvement myself. I guess that's up to you guys :) > And these features surely need some test coverage. Yes, planning to add tests.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:579: if (this._searchRegex && this._searchMessage(this._visibleViewMessages.length-1)) { On 2014/10/25 23:12:59, robwu wrote: > Spaces around minus. Done. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:924: } On 2014/10/25 23:12:59, robwu wrote: > Omit braces for single-line for/if/if-else blocks (this comment applies to all > other occurrences of this pattern in your patch). Done. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:966: matchRange.message]; On 2014/10/25 23:13:00, robwu wrote: > matchRange.message? As said in the comment below, this is confusing and needs to > be change to something more sensible, e.g. matchRange.resultIndex. I changed it to messageIndex, since it's actually an index into the list of messages. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:974: _jumpToMatch: function(index) On 2014/10/25 23:12:59, robwu wrote: > "_jumpToSearchResult" was more descriptive than "_jumpToMatch", in my opinion. The reason I renamed to jumpToMatch was to tie into the core concept of 'matches', since I identify individual search hits as matches. I think regardless of which term we choose to identify these occurrences, it makes sense to name these methods correspondingly. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:1011: while (match && match[0]) { On 2014/10/25 23:12:59, robwu wrote: > The idiomatic way to use .exec in a JS loop is: > > var match; > while ((match = this._searchRegex.exec(text)) !== null && match[0]) { > .... > } Done. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:1013: message: index, On 2014/10/25 23:12:59, robwu wrote: > "message" is confusing. I thought that it was a string when I read it elsewhere. > Choose a more appropriate property name, e.g. resultIndex. Done. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:1015: end: match.index + match[0].length-1, On 2014/10/25 23:12:59, robwu wrote: > Spaces around the minus sign. Done. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleView.js:1024: return numMatches > 0; On 2014/10/25 23:12:59, robwu wrote: > numMatches can be eliminated by using the fact that the matchRange variable is > only truthy when the loop runs at least once. > > return !!matchRange; Done. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... File Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/console/ConsoleViewMessage.js:1280: return this._messageElement.textContent; On 2014/10/25 23:13:00, robwu wrote: > What's the difference between the text getter (a few lines back) and your new > getText method? Why can't you use the previous one? The reason for the getText method is to implement a common interface of console messages, that is used when searching the console (ConsoleView._searchMessage). I'm not sure if the text getter could be used for the common interface instead; I noticed originally for instance that the text getter was implemented differently than ConsoleViewMessage.matchesRegex, which is used when searching. I'm still wondering if the text getter's implementation would actually differ from getText(). https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/ui... File Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/ui... Source/devtools/front_end/ui/UIUtils.js:787: * @param {?Boolean=} isCurrent On 2014/10/25 23:13:00, robwu wrote: > Booleans are primitive types, use a lowercase "boolean". ? denotes nullable and > {...=} denotes an optional value, you can therefore remove the "=". > > (side note, the ! and = at the previous line doesn't make sense either, it says > non-nullable AND optional. That doesn't fit together). I made it boolean= instead since the argument is in fact optional. Anyway, the Closure compiler won't accept ?boolean, since there's an optional argument before it. https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/ui... Source/devtools/front_end/ui/UIUtils.js:792: var style = isCurrent ? "current-search-result" : "highlighted-search-result"; On 2014/10/25 23:13:00, robwu wrote: > Rename "style" to className. Done.
On 2014/10/25 23:13:00, robwu wrote: > And these features surely need some test coverage. Would you mind providing me with some tips regarding how to build the layout test runner? I've read the documentation, but the exact steps are still unclear to me. Also, could you please explain the logic of the LayoutTests/inspector/console/console-search-reveals-messages.html test module? I thought I'd base my test(s) on this.
On 2014/10/26 18:43:21, aknudsen wrote: > On 2014/10/25 23:13:00, robwu wrote: > > And these features surely need some test coverage. > > Would you mind providing me with some tips regarding how to build the layout > test runner? I've read the documentation, but the exact steps are still unclear > to me. (assuming Linux, steps for Mac/Windows are similar) 1. Get Chromium, see http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/do... 2. Build the content_shell ninja -c out/Release content_shell 3. Run the tests (e.g. a subset of tests that is located at third_party/WebKit/LayoutTests/inspector/console/) ./third_party/WebKit/Tools/Scripts/run-webkit-tests inspector/console/* > Also, could you please explain the logic of the > LayoutTests/inspector/console/console-search-reveals-messages.html test module? > I thought I'd base my test(s) on this. I'm not a devtools developer, so I don't know the answer without looking at the code (and I don't have time to look at the code right now).
On 2014/10/26 19:03:02, robwu wrote: > 1. Get Chromium, see > http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/do... Can I execute 'fetch {chromium,blink}' within the blink repo which I've already cloned? > 2. Build the content_shell > ninja -c out/Release content_shell > 3. Run the tests (e.g. a subset of tests that is located at > third_party/WebKit/LayoutTests/inspector/console/) > ./third_party/WebKit/Tools/Scripts/run-webkit-tests inspector/console/* Thanks.
On 2014/10/26 19:36:34, aknudsen wrote: > On 2014/10/26 19:03:02, robwu wrote: > > 1. Get Chromium, see > > > http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/do... > > Can I execute 'fetch {chromium,blink}' within the blink repo which I've already > cloned? You can, but it will probably not have the desired result. Just go to the upper directory and use fetch blink. This will grab the source of Chromium with DEPS set to the tip of the source tree. Then, before you run gclient sync, replace third_party/WebKit with your existing Blink directory. After doing that, run gclient sync. gclient sync will then update all dependencies (in third_party/), including Blink. Make sure that you've committed your changes to a separate branch, or else gsync will fail (and you have to run gclient sync again). gclient sync will check out the revision as indicated in DEPS, if you want to return to your feature branch, use git checkout <name of your branch> to get back to work. You might want to rebase your branch to be in sync with the latest Blink code (within third_party/WebKit, use git rebase origin/master ). > > > 2. Build the content_shell > > ninja -c out/Release content_shell > > 3. Run the tests (e.g. a subset of tests that is located at > > third_party/WebKit/LayoutTests/inspector/console/) > > ./third_party/WebKit/Tools/Scripts/run-webkit-tests inspector/console/* > > Thanks.
On 2014/10/26 20:05:56, robwu wrote: > On 2014/10/26 19:36:34, aknudsen wrote: > > On 2014/10/26 19:03:02, robwu wrote: > > > 1. Get Chromium, see > > > > > > http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/do... > > > > Can I execute 'fetch {chromium,blink}' within the blink repo which I've > already > > cloned? > > You can, but it will probably not have the desired result. > Just go to the upper directory and use fetch blink. This will grab the source of > Chromium with DEPS set to the tip of the source tree. Thanks. What's the practical difference between 'fetch blink' and 'fetch {chromium,blink}'? What's DEPS mean?
On 2014/10/26 21:24:10, aknudsen wrote: > On 2014/10/26 20:05:56, robwu wrote: > > On 2014/10/26 19:36:34, aknudsen wrote: > > > On 2014/10/26 19:03:02, robwu wrote: > > > > 1. Get Chromium, see > > > > > > > > > > http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/do... > > > > > > Can I execute 'fetch {chromium,blink}' within the blink repo which I've > > already > > > cloned? > > > > You can, but it will probably not have the desired result. > > Just go to the upper directory and use fetch blink. This will grab the source > of > > Chromium with DEPS set to the tip of the source tree. > > Thanks. What's the practical difference between 'fetch blink' and 'fetch > {chromium,blink}'? What's DEPS mean? fetch, see http://www.chromium.org/developers/how-tos/get-the-code#Actual_Checkout src/DEPS defines all dependencies that are needed to build Chrome.
vsevik@chromium.org changed reviewers: + lushnikov@chromium.org
I've applied the patch and played with it. It's a great start! However, there are a couple of glitches that should be fixed. 1. As the user goes through the matched entries, the console viewport should be scrolled to present active match. 2. If you do a search which does at least two matches and then navigate between the first and second multiple times (via "enter" and "shift-enter"), you'll see how the layout gets gradually disrupted - text overlays each other.
Regarding the console-search-reveals-messages.html test, here's an explanation how things work https://gist.github.com/aslushnikov/f1bf449ebc955435c38a Hope this helps.
On 2014/10/26 19:03:02, robwu wrote: > On 2014/10/26 18:43:21, aknudsen wrote: > > Would you mind providing me with some tips regarding how to build the layout > > test runner? I've read the documentation, but the exact steps are still > unclear > > to me. > > (assuming Linux, steps for Mac/Windows are similar) > > 1. Get Chromium, see > http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/do... > 2. Build the content_shell > ninja -c out/Release content_shell The build fails like this (I'm on OS X): ➜ src git:(dcb2165) ninja -C out/Release ninja: Entering directory `out/Release' [68/25910] MACTOOL copy-bundle-resource ../../breakpad/src/client/mac/sender/English.lproj/Localizable.strings FAILED: ./gyp-mac-tool copy-bundle-resource ../../breakpad/src/client/mac/sender/English.lproj/Localizable.strings crash_report_sender.app/Contents/Resources/English.lproj/Localizable.strings Traceback (most recent call last): File "./gyp-mac-tool", line 588, in <module> sys.exit(main(sys.argv[1:])) File "./gyp-mac-tool", line 28, in main exit_code = executor.Dispatch(args) File "./gyp-mac-tool", line 43, in Dispatch return getattr(self, method)(*args[1:]) File "./gyp-mac-tool", line 66, in ExecCopyBundleResource self._CopyStringsFile(source, dest) File "./gyp-mac-tool", line 105, in _CopyStringsFile import CoreFoundation ImportError: No module named CoreFoundation
On 2014/10/27 08:16:37, lushnikov wrote: > I've applied the patch and played with it. It's a great start! However, there > are a couple of glitches that should be fixed. > > 1. As the user goes through the matched entries, the console viewport should be > scrolled to present active match. > 2. If you do a search which does at least two matches and then navigate between > the first and second multiple times (via "enter" and "shift-enter"), you'll > see how the layout gets gradually disrupted - text overlays each other. Thanks for catching those! I will try to rectify the issues.
On 2014/10/27 17:37:43, aknudsen wrote: > On 2014/10/26 19:03:02, robwu wrote: > > On 2014/10/26 18:43:21, aknudsen wrote: > > > Would you mind providing me with some tips regarding how to build the layout > > > test runner? I've read the documentation, but the exact steps are still > > unclear > > > to me. > > > > (assuming Linux, steps for Mac/Windows are similar) > > > > 1. Get Chromium, see > > > http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/do... > > 2. Build the content_shell > > ninja -c out/Release content_shell > The build fails like this (I'm on OS X): > ➜ src git:(dcb2165) ninja -C out/Release > ninja: Entering directory `out/Release' > [68/25910] MACTOOL copy-bundle-resource > ../../breakpad/src/client/mac/sender/English.lproj/Localizable.strings > FAILED: ./gyp-mac-tool copy-bundle-resource > ../../breakpad/src/client/mac/sender/English.lproj/Localizable.strings > crash_report_sender.app/Contents/Resources/English.lproj/Localizable.strings > Traceback (most recent call last): > File "./gyp-mac-tool", line 588, in <module> > sys.exit(main(sys.argv[1:])) > File "./gyp-mac-tool", line 28, in main > exit_code = executor.Dispatch(args) > File "./gyp-mac-tool", line 43, in Dispatch > return getattr(self, method)(*args[1:]) > File "./gyp-mac-tool", line 66, in ExecCopyBundleResource > self._CopyStringsFile(source, dest) > File "./gyp-mac-tool", line 105, in _CopyStringsFile > import CoreFoundation > ImportError: No module named CoreFoundation Do you have both xCode & xcode command line tools installed?
On 2014/10/28 04:43:55, lushnikov wrote: > On 2014/10/27 17:37:43, aknudsen wrote: > > On 2014/10/26 19:03:02, robwu wrote: > > > On 2014/10/26 18:43:21, aknudsen wrote: > > > > Would you mind providing me with some tips regarding how to build the > layout > > > > test runner? I've read the documentation, but the exact steps are still > > > unclear > > > > to me. > > > > > > (assuming Linux, steps for Mac/Windows are similar) > > > > > > 1. Get Chromium, see > > > > > > http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/do... > > > 2. Build the content_shell > > > ninja -c out/Release content_shell > > The build fails like this (I'm on OS X): > > ➜ src git:(dcb2165) ninja -C out/Release > > ninja: Entering directory `out/Release' > > [68/25910] MACTOOL copy-bundle-resource > > ../../breakpad/src/client/mac/sender/English.lproj/Localizable.strings > > FAILED: ./gyp-mac-tool copy-bundle-resource > > ../../breakpad/src/client/mac/sender/English.lproj/Localizable.strings > > crash_report_sender.app/Contents/Resources/English.lproj/Localizable.strings > > Traceback (most recent call last): > > File "./gyp-mac-tool", line 588, in <module> > > sys.exit(main(sys.argv[1:])) > > File "./gyp-mac-tool", line 28, in main > > exit_code = executor.Dispatch(args) > > File "./gyp-mac-tool", line 43, in Dispatch > > return getattr(self, method)(*args[1:]) > > File "./gyp-mac-tool", line 66, in ExecCopyBundleResource > > self._CopyStringsFile(source, dest) > > File "./gyp-mac-tool", line 105, in _CopyStringsFile > > import CoreFoundation > > ImportError: No module named CoreFoundation > > Do you have both xCode & xcode command line tools installed? Thanks, the issue was resolved in the meantime. Turned out I had to install PyObjC, since I'm not using the Apple-provided Python installation. I am ready to write tests now, just gotta find the time :)
On 2014/10/27 09:14:40, lushnikov wrote: > Regarding the console-search-reveals-messages.html test, here's an explanation > how things work > https://gist.github.com/aslushnikov/f1bf449ebc955435c38a > > Hope this helps. Thank you so much, it was a great help :)
On 2014/10/27 08:16:37, lushnikov wrote: > I've applied the patch and played with it. It's a great start! However, there > are a couple of glitches that should be fixed. > > 1. As the user goes through the matched entries, the console viewport should be > scrolled to present active match. Fixed. > 2. If you do a search which does at least two matches and then navigate between > the first and second multiple times (via "enter" and "shift-enter"), you'll > see how the layout gets gradually disrupted - text overlays each other. I can't reproduce this. Can you see if it's still an issue after patch set 3?
On 2014/10/29 22:09:41, aknudsen wrote: > On 2014/10/27 08:16:37, lushnikov wrote: > > I've applied the patch and played with it. It's a great start! However, there > > are a couple of glitches that should be fixed. > > > > 1. As the user goes through the matched entries, the console viewport should > be > > scrolled to present active match. > Fixed. > > > 2. If you do a search which does at least two matches and then navigate > between > > the first and second multiple times (via "enter" and "shift-enter"), you'll > > see how the layout gets gradually disrupted - text overlays each other. > I can't reproduce this. Can you see if it's still an issue after patch set 3? Still reproes; however it turned out devtools has to be zoomed in in order to reproduce this (I have a 125% zoom).
On 2014/10/30 06:09:35, lushnikov wrote: > On 2014/10/29 22:09:41, aknudsen wrote: > > On 2014/10/27 08:16:37, lushnikov wrote: > > > I've applied the patch and played with it. It's a great start! However, > there > > > are a couple of glitches that should be fixed. > > > > > > 1. As the user goes through the matched entries, the console viewport should > > be > > > scrolled to present active match. > > Fixed. > > > > > 2. If you do a search which does at least two matches and then navigate > > between > > > the first and second multiple times (via "enter" and "shift-enter"), you'll > > > see how the layout gets gradually disrupted - text overlays each other. > > I can't reproduce this. Can you see if it's still an issue after patch set 3? > > Still reproes; however it turned out devtools has to be zoomed in in order to > reproduce this (I have a 125% zoom). Are you able to tell what's causing the layout disruption, technically?
On 2014/10/30 09:03:54, aknudsen wrote: > On 2014/10/30 06:09:35, lushnikov wrote: > > On 2014/10/29 22:09:41, aknudsen wrote: > > > On 2014/10/27 08:16:37, lushnikov wrote: > > > > I've applied the patch and played with it. It's a great start! However, > > there > > > > are a couple of glitches that should be fixed. > > > > > > > > 1. As the user goes through the matched entries, the console viewport > should > > > be > > > > scrolled to present active match. > > > Fixed. > > > > > > > 2. If you do a search which does at least two matches and then navigate > > > between > > > > the first and second multiple times (via "enter" and "shift-enter"), > you'll > > > > see how the layout gets gradually disrupted - text overlays each other. > > > I can't reproduce this. Can you see if it's still an issue after patch set > 3? > > > > Still reproes; however it turned out devtools has to be zoomed in in order to > > reproduce this (I have a 125% zoom). > Are you able to tell what's causing the layout disruption, technically? Looks like a bug in cleanup when you move from highlighting one search result to another. As a result, a nested structure of spans with class "highlighted-search-result" is created
On 2014/10/30 09:20:53, lushnikov wrote: > On 2014/10/30 09:03:54, aknudsen wrote: > > On 2014/10/30 06:09:35, lushnikov wrote: > > > On 2014/10/29 22:09:41, aknudsen wrote: > > > > On 2014/10/27 08:16:37, lushnikov wrote: > > > > > I've applied the patch and played with it. It's a great start! However, > > > there > > > > > are a couple of glitches that should be fixed. > > > > > > > > > > 1. As the user goes through the matched entries, the console viewport > > should > > > > be > > > > > scrolled to present active match. > > > > Fixed. > > > > > > > > > 2. If you do a search which does at least two matches and then navigate > > > > between > > > > > the first and second multiple times (via "enter" and "shift-enter"), > > you'll > > > > > see how the layout gets gradually disrupted - text overlays each other. > > > > I can't reproduce this. Can you see if it's still an issue after patch set > > 3? > > > > > > Still reproes; however it turned out devtools has to be zoomed in in order > to > > > reproduce this (I have a 125% zoom). > > Are you able to tell what's causing the layout disruption, technically? > > Looks like a bug in cleanup when you move from highlighting one search result to > another. As a result, a nested structure of spans with class > "highlighted-search-result" is created Ah, sounds very plausible. I'll get on it. Thanks!
I've applied your patch one more time. There's still a lag when i search for "HTML" in the dumped tree element for window object. The profiler points to multiple highlightRangesWithStyleClass calls, which hopefully will be drastically reduced given the comments addressed. https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... File LayoutTests/inspector/console/console-search-can-highlight-current-match.html (right): https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search-can-highlight-current-match.html:22: setTimeout(InspectorTest.completeTest.bind(InspectorTest), 2000); We do not commit these lines - just use them to debug. Please, remove https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search-can-highlight-current-match.html:46: } you don't call "next()" in the end of the function. https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... File LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html (right): https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html:51: function testFindMatches(next) Could you please group all the tests into a single console-search-test.html file with a single runTestSuite() command, which will run all these functions? This will save us a lot of memory - you patch adds 800+ lines of code, most of which are test expectations. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:51: this._regexMatchRanges = []; please annotate this with the type of elemens in the array. You might want to typedef your classname for regexMatch object - please, take a look at the CSSParser.js file for an example. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:959: var message = this._visibleViewMessages[ please no wrapping in the code https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:967: _jumpToMatch: function(index) annotate https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:972: index = mod(index, this._regexMatchRanges.length); this line could be moved after the "if" https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:975: // Convert current match highlight to regular match highlight remove comment https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:978: matchRange.highlightNode.classList.add(WebInspector.highlightedSearchResultClassName); We can make this simpler. This line could be deleted together with line #984. After that, your rule for highlighting current-search-result should have a selector with bigger specificity, e.g. .highlighted-search-result.current-search-result { ... } https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:984: matchRange.highlightNode.classList.remove(WebInspector.highlightedSearchResultClassName); this is not needed as per previous comment https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:986: // Scroll the message itself into view lets remove these comments regarding scroll and add a test instead to verify that every match is actually shown to the user. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:993: _searchMessage: function(index) please annotate also, please move it closer to the performSearch method to improve ease of read https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:995: // Reset regex wrt. global search please finish the comment with a period https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1006: end: match.index + match[0].length - 1, you compute "end" here, and afterward couple of layers below (in highlightMatch function) compute range length again. Lets store length here? https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1008: matchRange.highlightNode = message.highlightMatch(matchRange.start, matchRange.end)[0]; Bad news: this calls WebInspector.highlightSearchResults for every match range. And this is inefficient, as it is iterating through the DOM every time. Good news: WebInspector.highlightSearchResults accepts an array of ranges to be highlighted, so this is easily fixable. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1174: // XXX: Can we remove this? Yes! https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1223: * @return {!string} wrong indent for comments (asterisks should form a column) https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1233: highlightMatch: function(start, end) arguments should be annotated as well https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1237: highlightNodes = WebInspector.highlightSearchResults(this._formattedCommand, please no wrap https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1239: this._element.scrollIntoViewIfNeeded(); Why do you need this call? It should not be needed. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:948: // XXX: Can we remove this? yes! https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1272: * @return {!string} no need for an exclamation mark - string is non-nullable as it's written with a small letter in the beginning https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1273: */ wrong comment alining regarding asterisks https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1274: getText: function () { brace on a new line https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1276: return ''; we use only double-quotes for strings https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1281: * @return {!Array.<!Element>} comment aligning + annotation for arguments https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1287: highlightNodes = WebInspector.highlightSearchResults(this._messageElement, no wrap https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1290: }, no trailing comma https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/elements/ElementsPanel.js:615: var matches = treeElement.listItemElement.getElementsByClassName( Please do not wrap. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/inspectorCommon.css (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/inspectorCommon.css:104: /* TODO: Come up with a design */ Please, remove the comment. We'll anyway land this after a review from our local UI specialist. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/inspectorCommon.css:105: .current-search-result { given the suggestion in one of other comments to not use highlightSearchResult function (and use highlightSearchResultWithStyleClass instead), this rule should move to some other console-specific stylesheet. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/ui/UIUtils.js:792: WebInspector.highlightSearchResults = function(element, resultRanges, changes) This method exists by mistake, it's not general. Let's not use it and rely on WebInspector.highlightRangesWithStyleClass instead. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/ui/UIUtils.js:801: WebInspector.removeSearchResultsHighlight = function(element) this method should accept one more argument - style class.
apavlov@chromium.org changed reviewers: + apavlov@chromium.org
https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1223: * @return {!string} Non-object types should not have ! in front of them ({!Object} but not {!string} or {!function()}), as they are non-nullable by default (as opposed to object types, which are nullable by default).
https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:51: this._regexMatchRanges = []; On 2014/11/07 16:27:16, lushnikov wrote: > please annotate this with the type of elemens in the array. > > You might want to typedef your classname for regexMatch object - please, take a > look at the CSSParser.js file for an example. Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:959: var message = this._visibleViewMessages[ On 2014/11/07 16:27:17, lushnikov wrote: > please no wrapping in the code Acknowledged. Out of curiousity though, how do you guys deal with long lines, by enabling soft-wrap in the editor? I'm totally used to a certain maximum line length, which is useful in avoiding having to side scroll when diffing for instance. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:967: _jumpToMatch: function(index) On 2014/11/07 16:27:16, lushnikov wrote: > annotate Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:972: index = mod(index, this._regexMatchRanges.length); On 2014/11/07 16:27:17, lushnikov wrote: > this line could be moved after the "if" Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:975: // Convert current match highlight to regular match highlight On 2014/11/07 16:27:17, lushnikov wrote: > remove comment Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:978: matchRange.highlightNode.classList.add(WebInspector.highlightedSearchResultClassName); On 2014/11/07 16:27:16, lushnikov wrote: > We can make this simpler. This line could be deleted together with line #984. > > After that, your rule for highlighting current-search-result > should have a selector with bigger specificity, e.g. > > .highlighted-search-result.current-search-result { > ... > } Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:984: matchRange.highlightNode.classList.remove(WebInspector.highlightedSearchResultClassName); On 2014/11/07 16:27:16, lushnikov wrote: > this is not needed as per previous comment Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:986: // Scroll the message itself into view On 2014/11/07 16:27:17, lushnikov wrote: > lets remove these comments regarding scroll and add a test instead to verify > that every match is actually shown to the user. Why are the comments problematic though? I had no idea initially, for example, that the message had to be scrolled into view before matchRange.highlightNode.scrollIntoViewIfNeeded() would have any effect. I agree that there should be a test in any case. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:993: _searchMessage: function(index) On 2014/11/07 16:27:16, lushnikov wrote: > please annotate > also, please move it closer to the performSearch method to improve ease of read Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:995: // Reset regex wrt. global search On 2014/11/07 16:27:16, lushnikov wrote: > please finish the comment with a period Acknowledged. Is this a common style guideline? https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1006: end: match.index + match[0].length - 1, On 2014/11/07 16:27:16, lushnikov wrote: > you compute "end" here, and afterward couple of layers below (in highlightMatch > function) compute range length again. Lets store length here? Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1008: matchRange.highlightNode = message.highlightMatch(matchRange.start, matchRange.end)[0]; On 2014/11/07 16:27:16, lushnikov wrote: > Bad news: this calls WebInspector.highlightSearchResults for every match range. > And this is inefficient, as it is iterating through the DOM every time. > > Good news: WebInspector.highlightSearchResults accepts an array of ranges to be > highlighted, so this is easily fixable. Acknowledged. Thanks. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1174: // XXX: Can we remove this? On 2014/11/07 16:27:17, lushnikov wrote: > Yes! Acknowledged. :) https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1223: * @return {!string} On 2014/11/07 16:27:16, lushnikov wrote: > wrong indent for comments (asterisks should form a column) Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1223: * @return {!string} On 2014/11/07 16:30:07, apavlov wrote: > Non-object types should not have ! in front of them ({!Object} but not {!string} > or {!function()}), as they are non-nullable by default (as opposed to object > types, which are nullable by default). Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1233: highlightMatch: function(start, end) On 2014/11/07 16:27:17, lushnikov wrote: > arguments should be annotated as well Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1237: highlightNodes = WebInspector.highlightSearchResults(this._formattedCommand, On 2014/11/07 16:27:16, lushnikov wrote: > please no wrap Acknowledged.
https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... File LayoutTests/inspector/console/console-search-can-highlight-current-match.html (right): https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search-can-highlight-current-match.html:22: setTimeout(InspectorTest.completeTest.bind(InspectorTest), 2000); On 2014/11/07 16:27:16, lushnikov wrote: > We do not commit these lines - just use them to debug. Please, remove Ah OK. https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search-can-highlight-current-match.html:46: } On 2014/11/07 16:27:16, lushnikov wrote: > you don't call "next()" in the end of the function. Acknowledged. https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... File LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html (right): https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html:51: function testFindMatches(next) On 2014/11/07 16:27:16, lushnikov wrote: > Could you please group all the tests into a single console-search-test.html file > with a single runTestSuite() command, which will run all these functions? > > This will save us a lot of memory - you patch adds 800+ lines of code, most of > which are test expectations. Ah, will do, thanks for the suggestion. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1272: * @return {!string} On 2014/11/07 16:27:19, lushnikov wrote: > no need for an exclamation mark - string is non-nullable as it's written with a > small letter in the beginning Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1273: */ On 2014/11/07 16:27:17, lushnikov wrote: > wrong comment alining regarding asterisks Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1274: getText: function () { On 2014/11/07 16:27:19, lushnikov wrote: > brace on a new line Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1276: return ''; On 2014/11/07 16:27:19, lushnikov wrote: > we use only double-quotes for strings Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1281: * @return {!Array.<!Element>} On 2014/11/07 16:27:19, lushnikov wrote: > comment aligning + annotation for arguments Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1287: highlightNodes = WebInspector.highlightSearchResults(this._messageElement, On 2014/11/07 16:27:19, lushnikov wrote: > no wrap Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1290: }, On 2014/11/07 16:27:19, lushnikov wrote: > no trailing comma Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/elements/ElementsPanel.js:615: var matches = treeElement.listItemElement.getElementsByClassName( On 2014/11/07 16:27:19, lushnikov wrote: > Please do not wrap. Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/inspectorCommon.css (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/inspectorCommon.css:104: /* TODO: Come up with a design */ On 2014/11/07 16:27:19, lushnikov wrote: > Please, remove the comment. We'll anyway land this after a review from our > local UI specialist. Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/inspectorCommon.css:105: .current-search-result { On 2014/11/07 16:27:19, lushnikov wrote: > given the suggestion in one of other comments to not use highlightSearchResult > function (and use highlightSearchResultWithStyleClass instead), this rule should > move to some other console-specific stylesheet. Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/ui/UIUtils.js:792: WebInspector.highlightSearchResults = function(element, resultRanges, changes) On 2014/11/07 16:27:19, lushnikov wrote: > This method exists by mistake, it's not general. Let's not use it and rely on > WebInspector.highlightRangesWithStyleClass instead. Acknowledged. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/ui/UIUtils.js:801: WebInspector.removeSearchResultsHighlight = function(element) On 2014/11/07 16:27:19, lushnikov wrote: > this method should accept one more argument - style class. Acknowledged.
https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... File LayoutTests/inspector/console/console-search-can-highlight-current-match.html (right): https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search-can-highlight-current-match.html:22: setTimeout(InspectorTest.completeTest.bind(InspectorTest), 2000); On 2014/11/07 16:27:16, lushnikov wrote: > We do not commit these lines - just use them to debug. Please, remove Done. https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search-can-highlight-current-match.html:46: } On 2014/11/07 16:27:16, lushnikov wrote: > you don't call "next()" in the end of the function. Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:51: this._regexMatchRanges = []; On 2014/11/07 16:27:16, lushnikov wrote: > please annotate this with the type of elemens in the array. > > You might want to typedef your classname for regexMatch object - please, take a > look at the CSSParser.js file for an example. Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:959: var message = this._visibleViewMessages[ On 2014/11/07 16:27:17, lushnikov wrote: > please no wrapping in the code Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:967: _jumpToMatch: function(index) On 2014/11/08 11:49:51, aknudsen wrote: > On 2014/11/07 16:27:16, lushnikov wrote: > > annotate > > Acknowledged. Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:972: index = mod(index, this._regexMatchRanges.length); On 2014/11/07 16:27:17, lushnikov wrote: > this line could be moved after the "if" Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:975: // Convert current match highlight to regular match highlight On 2014/11/07 16:27:17, lushnikov wrote: > remove comment Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:978: matchRange.highlightNode.classList.add(WebInspector.highlightedSearchResultClassName); On 2014/11/07 16:27:16, lushnikov wrote: > We can make this simpler. This line could be deleted together with line #984. > > After that, your rule for highlighting current-search-result > should have a selector with bigger specificity, e.g. > > .highlighted-search-result.current-search-result { > ... > } Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:984: matchRange.highlightNode.classList.remove(WebInspector.highlightedSearchResultClassName); On 2014/11/07 16:27:16, lushnikov wrote: > this is not needed as per previous comment Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:993: _searchMessage: function(index) On 2014/11/07 16:27:16, lushnikov wrote: > please annotate > also, please move it closer to the performSearch method to improve ease of read Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:995: // Reset regex wrt. global search On 2014/11/07 16:27:16, lushnikov wrote: > please finish the comment with a period Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1006: end: match.index + match[0].length - 1, On 2014/11/07 16:27:16, lushnikov wrote: > you compute "end" here, and afterward couple of layers below (in highlightMatch > function) compute range length again. Lets store length here? I simplified, by sending an array of SourceRanges to message.highlightMatches instead. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1008: matchRange.highlightNode = message.highlightMatch(matchRange.start, matchRange.end)[0]; On 2014/11/07 16:27:16, lushnikov wrote: > Bad news: this calls WebInspector.highlightSearchResults for every match range. > And this is inefficient, as it is iterating through the DOM every time. > > Good news: WebInspector.highlightSearchResults accepts an array of ranges to be > highlighted, so this is easily fixable. Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1174: // XXX: Can we remove this? On 2014/11/07 16:27:17, lushnikov wrote: > Yes! Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1223: * @return {!string} On 2014/11/07 16:27:16, lushnikov wrote: > wrong indent for comments (asterisks should form a column) Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1223: * @return {!string} On 2014/11/07 16:30:07, apavlov wrote: > Non-object types should not have ! in front of them ({!Object} but not {!string} > or {!function()}), as they are non-nullable by default (as opposed to object > types, which are nullable by default). Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1233: highlightMatch: function(start, end) On 2014/11/07 16:27:17, lushnikov wrote: > arguments should be annotated as well Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1237: highlightNodes = WebInspector.highlightSearchResults(this._formattedCommand, On 2014/11/07 16:27:16, lushnikov wrote: > please no wrap Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1239: this._element.scrollIntoViewIfNeeded(); On 2014/11/07 16:27:17, lushnikov wrote: > Why do you need this call? It should not be needed. Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:948: // XXX: Can we remove this? On 2014/11/07 16:27:19, lushnikov wrote: > yes! Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1272: * @return {!string} On 2014/11/07 16:27:19, lushnikov wrote: > no need for an exclamation mark - string is non-nullable as it's written with a > small letter in the beginning Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1273: */ On 2014/11/07 16:27:17, lushnikov wrote: > wrong comment alining regarding asterisks Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1274: getText: function () { On 2014/11/07 16:27:19, lushnikov wrote: > brace on a new line Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1276: return ''; On 2014/11/07 16:27:19, lushnikov wrote: > we use only double-quotes for strings Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1281: * @return {!Array.<!Element>} On 2014/11/07 16:27:19, lushnikov wrote: > comment aligning + annotation for arguments Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1287: highlightNodes = WebInspector.highlightSearchResults(this._messageElement, On 2014/11/07 16:27:19, lushnikov wrote: > no wrap Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1290: }, On 2014/11/07 16:27:19, lushnikov wrote: > no trailing comma Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/elements/ElementsPanel.js:615: var matches = treeElement.listItemElement.getElementsByClassName( On 2014/11/07 16:27:19, lushnikov wrote: > Please do not wrap. Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/inspectorCommon.css (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/inspectorCommon.css:104: /* TODO: Come up with a design */ On 2014/11/07 16:27:19, lushnikov wrote: > Please, remove the comment. We'll anyway land this after a review from our > local UI specialist. Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/inspectorCommon.css:105: .current-search-result { On 2014/11/07 16:27:19, lushnikov wrote: > given the suggestion in one of other comments to not use highlightSearchResult > function (and use highlightSearchResultWithStyleClass instead), this rule should > move to some other console-specific stylesheet. Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/ui/UIUtils.js:792: WebInspector.highlightSearchResults = function(element, resultRanges, changes) On 2014/11/07 16:27:19, lushnikov wrote: > This method exists by mistake, it's not general. Let's not use it and rely on > WebInspector.highlightRangesWithStyleClass instead. Done. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/ui/UIUtils.js:801: WebInspector.removeSearchResultsHighlight = function(element) On 2014/11/07 16:27:19, lushnikov wrote: > this method should accept one more argument - style class. Done.
https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... File LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html (right): https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html:51: function testFindMatches(next) On 2014/11/07 16:27:16, lushnikov wrote: > Could you please group all the tests into a single console-search-test.html file > with a single runTestSuite() command, which will run all these functions? > > This will save us a lot of memory - you patch adds 800+ lines of code, most of > which are test expectations. When considering merging tests, I thought about how they differ in setup logic. For example console-search-can-highlight-current-match.html and console-search-can-jump-between-matches.html, these differ in that they write to the console before running their tests. Do you think these two should be merged into a single suite for instance?
https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:959: var message = this._visibleViewMessages[ On 2014/11/08 11:49:50, aknudsen wrote: > On 2014/11/07 16:27:17, lushnikov wrote: > > please no wrapping in the code > > Acknowledged. > > Out of curiousity though, how do you guys deal with long lines, by enabling > soft-wrap in the editor? I'm totally used to a certain maximum line length, > which is useful in avoiding having to side scroll when diffing for instance. It depends, one enables line-wrapping, one gets larger display.. All in all, its a styleguide we all follow. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:986: // Scroll the message itself into view On 2014/11/08 11:49:51, aknudsen wrote: > On 2014/11/07 16:27:17, lushnikov wrote: > > lets remove these comments regarding scroll and add a test instead to verify > > that every match is actually shown to the user. > > Why are the comments problematic though? I had no idea initially, for example, > that the message had to be scrolled into view before > matchRange.highlightNode.scrollIntoViewIfNeeded() would have any effect. I agree > that there should be a test in any case. In this case, the comment doesn't add any new information to the line it comments (viewport.scrollItemIntoView is verbose and clear itself). In general, the policy is to write as little comments as possible and prefer self-describing code. https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:995: // Reset regex wrt. global search On 2014/11/08 11:49:51, aknudsen wrote: > On 2014/11/07 16:27:16, lushnikov wrote: > > please finish the comment with a period > > Acknowledged. > > Is this a common style guideline? Yes, http://www.chromium.org/blink/coding-style#TOC-Comments
> When considering merging tests, I thought about how they differ in setup logic. > For example console-search-can-highlight-current-match.html and > console-search-can-jump-between-matches.html, these differ in that they write to > the console before running their tests. Do you think these two should be merged > into a single suite for instance? Can we introduce a single setup which will be good enough to satisfy all the test cases? There's usually a way to do so
On 2014/11/19 06:57:00, lushnikov wrote: > > When considering merging tests, I thought about how they differ in setup > logic. > > For example console-search-can-highlight-current-match.html and > > console-search-can-jump-between-matches.html, these differ in that they write > to > > the console before running their tests. Do you think these two should be > merged > > into a single suite for instance? > > Can we introduce a single setup which will be good enough to satisfy all the > test cases? > There's usually a way to do so I will see what I can do.
On 2014/11/24 08:07:44, aknudsen wrote: > On 2014/11/19 06:57:00, lushnikov wrote: > > > When considering merging tests, I thought about how they differ in setup > > logic. > > > For example console-search-can-highlight-current-match.html and > > > console-search-can-jump-between-matches.html, these differ in that they > write > > to > > > the console before running their tests. Do you think these two should be > > merged > > > into a single suite for instance? > > > > Can we introduce a single setup which will be good enough to satisfy all the > > test cases? > > There's usually a way to do so > > I will see what I can do. I've tried consolidating the test cases into a single suite, console-search.html. Better now?
Thank you for the effort! Just a few last nits before we can have this landed https://codereview.chromium.org/676193002/diff/200001/LayoutTests/inspector/c... File LayoutTests/inspector/console/console-search.html (right): https://codereview.chromium.org/676193002/diff/200001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search.html:62: // Find first match Please terminate all the comments here with period. https://codereview.chromium.org/676193002/diff/200001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search.html:110: addResult(InspectorTest.dumpConsoleMessagesIntoArray(false, true)); This adds a lot of output which is impossible to read through. Can we do some querySelector magic here? https://codereview.chromium.org/676193002/diff/200001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/200001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:947: * @param {number} index @return annotation is missing https://codereview.chromium.org/676193002/diff/200001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/676193002/diff/200001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1278: getText: function () We have getter text() and method getText, which is confusing. Lets rename getText -> renderedText to emphasize the difference.
https://codereview.chromium.org/676193002/diff/200001/LayoutTests/inspector/c... File LayoutTests/inspector/console/console-search.html (right): https://codereview.chromium.org/676193002/diff/200001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search.html:62: // Find first match On 2014/12/04 16:07:34, lushnikov wrote: > Please terminate all the comments here with period. Done. https://codereview.chromium.org/676193002/diff/200001/LayoutTests/inspector/c... LayoutTests/inspector/console/console-search.html:110: addResult(InspectorTest.dumpConsoleMessagesIntoArray(false, true)); On 2014/12/04 16:07:34, lushnikov wrote: > This adds a lot of output which is impossible to read through. Can we do some > querySelector magic here? Done. https://codereview.chromium.org/676193002/diff/200001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/200001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:947: * @param {number} index On 2014/12/04 16:07:34, lushnikov wrote: > @return annotation is missing Done. https://codereview.chromium.org/676193002/diff/200001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/676193002/diff/200001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleViewMessage.js:1278: getText: function () On 2014/12/04 16:07:34, lushnikov wrote: > We have getter text() and method getText, which is confusing. Lets rename > getText -> renderedText to emphasize the difference. Done.
I haven't tested my latest changes, since the current Chromium head isn't buildable :( I consistently get C++ compilation errors when trying to build (ninja -C out/Release blink_tests).
On 2014/12/05 22:36:49, aknudsen wrote: > I haven't tested my latest changes, since the current Chromium head isn't > buildable :( I consistently get C++ compilation errors when trying to build > (ninja -C out/Release blink_tests). Have you ran gclient sync after pulling in the latest changes from upstream? To sync with HEAD: git fetch origin master git rebase origin/master gclient sync ninja -C out/Debug (upload rebase, SEPARATELY from any changes to make reviewing easier) git cl upload If that did not help, remove out/ and run gclient runhooks before running ninja again. (FYI: yesterday, a patch landed that speeds up the dev cycle by removing the need for recompilation of Chrome when only webui files are changed, I think that devtools pages are also among them: https://crbug.com/439182.)
On Sat, Dec 6, 2014 at 12:05 AM, <rob@robwu.nl> wrote: > On 2014/12/05 22:36:49, aknudsen wrote: > >> I haven't tested my latest changes, since the current Chromium head isn't >> buildable :( I consistently get C++ compilation errors when trying to >> build >> (ninja -C out/Release blink_tests). >> > > Have you ran gclient sync after pulling in the latest changes from > upstream? > > To sync with HEAD: > git fetch origin master > git rebase origin/master > gclient sync > ninja -C out/Debug > Yeah, I did a git pull --rebase origin master before running gclient sync and finally building. > If that did not help, remove out/ and run gclient runhooks before running > ninja > again. > Thanks, I've tried removing out/ now. However, do I need to remove anything else than out/{Release,Debug}? If I remove all of out/ I need to sync again... What does gclient runhooks do? Arve To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Nah, still fails: ./../third_party/WebKit/Source/core/inspector/ScriptProfile.cpp:70:32: error: no member named 'GetHitLineCount' in 'v8::CpuProfileNode'; did you mean 'GetHitCount'? unsigned lineCount = node->GetHitLineCount(); ^~~~~~~~~~~~~~~ GetHitCount ../../v8/include/v8-profiler.h:54:12: note: 'GetHitCount' declared here unsigned GetHitCount() const; ^ ../../third_party/WebKit/Source/core/inspector/ScriptProfile.cpp:74:32: error: no member named 'LineTick' in 'v8::CpuProfileNode' Vector<v8::CpuProfileNode::LineTick> entries(lineCount); ~~~~~~~~~~~~~~~~~~~~^ ../../third_party/WebKit/Source/core/inspector/ScriptProfile.cpp:75:15: error: no member named 'GetLineTicks' in 'v8::CpuProfileNode' if (node->GetLineTicks(&entries[0], lineCount)) { ~~~~ ^ ../../third_party/WebKit/Source/core/inspector/ScriptProfile.cpp:75:29: error: use of undeclared identifier 'entries' if (node->GetLineTicks(&entries[0], lineCount)) { ^ ../../third_party/WebKit/Source/core/inspector/ScriptProfile.cpp:78:26: error: use of undeclared identifier 'entries' .setLine(entries[i].line) ^ ../../third_party/WebKit/Source/core/inspector/ScriptProfile.cpp:79:27: error: use of undeclared identifier 'entries' .setTicks(entries[i].hit_count); ^ 6 errors generated. On Sat, Dec 6, 2014 at 12:52 AM, Arve Knudsen <arve.knudsen@gmail.com> wrote: > On Sat, Dec 6, 2014 at 12:05 AM, <rob@robwu.nl> wrote: > >> On 2014/12/05 22:36:49, aknudsen wrote: >> >>> I haven't tested my latest changes, since the current Chromium head isn't >>> buildable :( I consistently get C++ compilation errors when trying to >>> build >>> (ninja -C out/Release blink_tests). >>> >> >> Have you ran gclient sync after pulling in the latest changes from >> upstream? >> >> To sync with HEAD: >> git fetch origin master >> git rebase origin/master >> gclient sync >> ninja -C out/Debug >> > > Yeah, I did a git pull --rebase origin master before running gclient sync > and finally building. > > >> If that did not help, remove out/ and run gclient runhooks before >> running ninja >> again. >> > > Thanks, I've tried removing out/ now. However, do I need to remove > anything else than out/{Release,Debug}? If I remove all of out/ I need to > sync again... What does gclient runhooks do? > > Arve > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
First of all, sorry for the long delay, and thank you for all the work you've done here. I've played with your patch, and it looks great for me. Please, see inline comments. https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1023: var message = this._visibleViewMessages[matchRange.messageIndex]; You should not call clearHighlights() more then once per every message - it gonna cost you DOM querying/manipulation. https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1051: // Scroll the message itself into view can we get rid of these comments?
https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1023: var message = this._visibleViewMessages[matchRange.messageIndex]; On 2014/12/17 13:21:23, lushnikov wrote: > You should not call clearHighlights() more then once per every message - it > gonna cost you DOM querying/manipulation. Acknowledged. https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1051: // Scroll the message itself into view On 2014/12/17 13:21:23, lushnikov wrote: > can we get rid of these comments? Do you want to also remove the comments at 1053-1054?
https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1023: var message = this._visibleViewMessages[matchRange.messageIndex]; On 2014/12/17 13:21:23, lushnikov wrote: > You should not call clearHighlights() more then once per every message - it > gonna cost you DOM querying/manipulation. Done.
Btw guys, I'm not able to test in Canary at this point. I get an exception in the JavaScript console when trying to debug DevTools of f.ex. http://chromium.org: Uncaught (in promise) ReferenceError {stack: (...), message: "DevToolsHost is not defined"} message: "DevToolsHost is not defined" stack: (...) get stack: function () { [native code] } set stack: function () { [native code] } __proto__: Error
On 2014/12/21 20:04:40, aknudsen wrote: > Btw guys, I'm not able to test in Canary at this point. I get an exception in > the JavaScript console when trying to debug DevTools of f.ex. > http://chromium.org: > > Uncaught (in promise) ReferenceError {stack: (...), message: "DevToolsHost > is not defined"} > message: "DevToolsHost is not defined" > stack: (...) > get stack: function () { [native code] } > set stack: function () { [native code] } > __proto__: Error Which URL do use for external front-end? The http://localhost:8090/front_end/inspector.html works for me (it changed recently, so it worth checking)
lgtm given the last nit fixed yay! :) https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1051: // Scroll the message itself into view On 2014/12/21 17:58:23, aknudsen wrote: > On 2014/12/17 13:21:23, lushnikov wrote: > > can we get rid of these comments? > > Do you want to also remove the comments at 1053-1054? I'd prefer it to, yes
One more nit from me. https://codereview.chromium.org/676193002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/inspectorCommon.css (right): https://codereview.chromium.org/676193002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/inspectorCommon.css:154: Undo this unnecessary space change (it's the only modification in this file).
On 2014/12/23 13:41:10, lushnikov wrote: > On 2014/12/21 20:04:40, aknudsen wrote: > > Btw guys, I'm not able to test in Canary at this point. I get an exception in > > the JavaScript console when trying to debug DevTools of f.ex. > > http://chromium.org: > > > > Uncaught (in promise) ReferenceError {stack: (...), message: "DevToolsHost > > is not defined"} > > message: "DevToolsHost is not defined" > > stack: (...) > > get stack: function () { [native code] } > > set stack: function () { [native code] } > > __proto__: Error > > Which URL do use for external front-end? The > http://localhost:8090/front_end/inspector.html works for me (it changed > recently, so it worth checking) I don't really know what it all means to be honest, but the following command (got it from https://developer.chrome.com/devtools/docs/contributing) now works for me on OS X: '/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --remote-debugging-port=9222 --no-first-run --user-data-dir=~/temp/chrome-dev-profile http://localhost:9222\#http://localhost:8000/front_end/inspector.html'. The command I was using previously, which stopped working, is: '/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --remote-debugging-port=9222 --no-first-run --user-data-dir=blink/chromeServerProfile http://localhost:9222\#http://localhost:8000/front_end/devtools.html'. Does my current Canary launch command line look OK to you?
All done I think. Merry Christmas! https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_e... Source/devtools/front_end/console/ConsoleView.js:1051: // Scroll the message itself into view On 2014/12/23 13:43:33, lushnikov wrote: > On 2014/12/21 17:58:23, aknudsen wrote: > > On 2014/12/17 13:21:23, lushnikov wrote: > > > can we get rid of these comments? > > > > Do you want to also remove the comments at 1053-1054? > > I'd prefer it to, yes Done. https://codereview.chromium.org/676193002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/inspectorCommon.css (right): https://codereview.chromium.org/676193002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/inspectorCommon.css:154: On 2014/12/23 13:53:35, robwu wrote: > Undo this unnecessary space change (it's the only modification in this file). Done.
The CQ bit was checked by lushnikov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/676193002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10...)
apavlov: Could you put your LGTM on this CL? Lushnikov has already LGed this CL, but he is not an owner.
The CQ bit was checked by arve.knudsen@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/676193002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
lgtm
The CQ bit was checked by lushnikov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/676193002/300001
Message was sent while issue was closed.
Committed patchset #14 (id:300001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188234 |