Updated frontend: fixed search through Drawer panel. #621

marcominetti · 2015-04-17T17:14:06Z

Implemented DebuggerAgent.searchInContent.
Implemented PageAgent.searchInResource.
Added /lib/search.js to provide shared search functions.
Added resource/script source caching.
Fixed highlighting matches bug on minified-file lines.

marcominetti · 2015-04-22T16:24:23Z

@3y3 This is not an easy pick pr... I'll push some more commits with syntax and code cleanup for linter... but I know we'll talk about the source code caching in DebuggerAgent implementation... :D

3y3 · 2015-05-12T08:11:09Z

@marcominetti , sorry for pending of your features, but there is a lot of bugs, which I need to fix and release before review.

marcominetti · 2015-05-12T21:48:00Z

@3y3, wow I've seen the new issues right now... may I help you? Feel free to assign me something. ;)

3y3 · 2015-05-14T13:21:28Z

Hello, @marcominetti , I target you from Plugin system pr. Also you can look on 0.10.2 milestone, here are bugs and features with hight priority. It is not necessary, what all this bugs needs to be fixed, in some situations we need only to describe solution. For example #566 - you are friendly with new front-end, so you can target man to entry point for this feature (or implement it by yourself).
So look at list, and if you ready to fix something - notify me.

marcominetti · 2015-05-14T21:12:16Z

@3y3 I can work on #566 and #580...

marcominetti · 2015-05-14T21:12:41Z

Let me know if you have any priority... ;)

marcominetti · 2015-05-14T21:14:49Z

Please notify me when you will be ready for review this pr... cause I'm thinking about reworking it after changing the internal behaviour incase of --no-preload param...

3y3 · 2015-05-15T06:34:03Z

I can work on #566 and #580...

Nice! Thank you! For #580 look also on #628. I think it's a duplication. In #628 posted good example for tests. (For correct installation delete npm-shrinkwrap)

Please notify me when you will be ready for review this pr... cause I'm thinking about reworking it after changing the internal behaviour incase of --no-preload param...

--no-preload was changed somewhere?

marcominetti · 2015-05-15T08:49:41Z

--no-preload was changed somewhere?

Not yet, I will elaborate the clue later on reviewing/reworking this pr... btw the ni stack is built to run "locally", so the --no-preload does prevent backend to feed frontend with every available file from disk... (am I right?) I use it to have frontend load quickly and smoothly in large project...

The problem is that the --no-preload flag also avoids frontend feeding the static content provider (at front-end) for actually loaded files... This dramatically impact search, because, at first run, it needs to fetch source code for every loaded file... I will try to override the --no-preload behaviour assuring that script content is fetched automatically for loaded files and stored within the static content provider...

cr0cK · 2015-05-15T09:07:24Z

+1 for #628 !

a7madgamal · 2015-07-01T10:56:04Z

Guys please work on this its a really important feature!
Thanks a million in advance :)

marcominetti · 2015-07-01T11:57:25Z

hi @a7madgamal @3y3 I've successfully implemented the feature but there is a huge memory consumption cause I need to cache/preempt source files in arrays because search feature generates very huge number of getScriptSourceById/getResourceContent requests to the debugger through DebuggerClient and everything crashes... I could cache source in /tmp/ filesystem (but it's not so clever in cloud environments for the I/O)...

Solutions:

fix debugger client or debugger backend crash on huge requests (best)
implement an ordered queue at getScriptSourceById and getResourceContent (@3y3?)
keep this implementation and/or move cache on disk (worst)

You can try implementation with ljve-inspector module available on npm (I use it actually).

a7madgamal · 2015-07-01T12:03:32Z

I can't find it, could you provide the npm link?

marcominetti · 2015-07-01T12:06:46Z

https://www.npmjs.com/package/ljve-inspector

3y3 · 2015-07-01T14:28:19Z

@marcominetti , I'm not familiar with search API in DevTools, but if I understand right main problem - we receive lot of parallel requests like searchInContent and we try to require sources for each scriptId also in parallel mode. So, we need to review strategy of original DevTools, when there exists a linked local folder:

If in this case DevTools tries to search in all files (not only in loaded to front-end), then we need to completely avoid getScriptSourceById and use fs (this makes some problems for generated scripts, but I prefer to ignore this problems in initial implementation)
Otherwise we can move search logic to app (look at injection example in ConsoleAgent)

In all this cases we need to completely avoid caching.

So, main question on current step - are we need to search in preloaded resources?

yorkie · 2015-07-01T14:49:31Z

front-end-node/search/SearchOverrides.js

+      try {
+        regexObject = new RegExp('^.*?'+query+'.*?$|^.*?'+query+'.*?\n|\n.*?'+query+'.*?\n|\n.*?'+query+'.*?$', regexFlags);
+      } catch (e) {
+        // Silent catch.


is the try-catch block really necessary? which case the RegExp will throw an error?

@yorkie , I'm not sure that this is a best time to review internal structure of this pr. At current step we need to decide how to avoid caching. Read my last comments.

Relative to your question:

new RegExp('abc[')

Anyway it would be nice if you'll help me with review!

a7madgamal · 2015-07-01T15:00:21Z

@marcominetti I installed your fork and its working greatly but when i search and click on a line it always highlight the NEXT line!
I want you to confirm this and if it turns out to be a bug I think I can help you fix it quickly
HINT:check the function getLine(doc, n) return chunk.lines[n]; in the "codemerror.js" file should be n-1 if n > 0

@3y3 I think it's ok for the search process to be a little bit slower and works like 10 files at a time then next 10 etc. for a basic fix. After all it's better than nothing!

3y3 · 2015-07-01T15:02:40Z

@a7madgamal , I agree that we must sacrifice performance for stability

marcominetti · 2015-07-01T16:00:36Z

@a7madgamal yep the bug of next line is fixed within an unpublished version... I'll publish it after some cleanup...

marcominetti · 2015-07-01T16:02:50Z

@3y3, I'm busy but I'll try to comment code just to share the pr/strategy... and I think the best way to improve stability and avoid memory leak is to directly check the filesystem if the file exists and, if not, trying to get through debuggerclient with requests "pagination"... do you agree?

marcominetti · 2015-07-10T08:59:01Z

@3y3 hi, i'll have to work on this next weekend... just to confirm, i'll do:

remove caching and
- read script contents from fs
- eventually pool by X when fs is not available/exist
include fix for wrong-next-line

marcominetti · 2015-07-14T22:26:21Z

@3y3 I've rebased against 0.11.1, reverted most of the code and switched to original functions extracted from frontend... It seems to work now. Performance are not exciting but no memory leaks. We can drop my custom implementation with regexp (commented code in SearchOverrides.js and _xxx "private" named functions in search.js). After review I'll fix the jshint stuff...

marcominetti · 2015-07-14T22:28:22Z

lib/search.js

+  return new RegExp(regex, flags || "");
+}
+
+exports.performSearchInContent = function(content, query, caseSensitive, isRegex)


@3y3 this code is adapted from /front-end/common/ContentProvider.js implementation...

3y3 · 2015-07-15T07:03:14Z

Hello @marcominetti , sorry for ignoring your previous comment - I'm very busy at this time. I'll review your changes as soon as possible. I'm not sure that this will happen today, but definitely on this week.

fakenickels · 2016-01-08T13:14:15Z

Waiting for this =) @3y3

marcominetti · 2016-01-12T13:05:08Z

Hi @grsabreu, in the weekend I'll check the status against latest commits to the master branch... are you experiencing specific problems?

3y3 · 2016-01-13T05:49:29Z

@marcominetti , please don't miss time at current weekend. I'll release "Node Inspector Next" at this month (I hope) with new frontend and completely reimplemented server. So it's not reasonable to rebase at current master.

Node Inspector Next breaks compatibility with oldest node versions (compatibility starts with 4.*), but implements lot of new features.

I'm sorry that I pending this pr, but each time then I start to review it, there happens new broken node release.

marcominetti · 2016-01-13T06:04:36Z

@3y3 cool! Do you need help on nin?

a7madgamal · 2016-02-09T23:53:21Z

any updates?

3y3 added the Enhancement label Apr 21, 2015

bajtos assigned 3y3 Apr 27, 2015

marcominetti mentioned this pull request May 11, 2015

Feature request: Debugger: Please implement search in all source files #234

Closed

marcominetti force-pushed the fix/search-panel branch 2 times, most recently from 98e5453 to 497b200 Compare May 19, 2015 13:03

marcominetti force-pushed the fix/search-panel branch from 4f2c4e3 to 463de7b Compare June 8, 2015 08:02

3y3 force-pushed the master branch from ab44ba1 to 50e3a33 Compare June 28, 2015 13:44

yorkie reviewed Jul 1, 2015
View reviewed changes

Fix search through draw panel.

47b25d6

marcominetti force-pushed the fix/search-panel branch from 46f6503 to 47b25d6 Compare July 14, 2015 22:20

marcominetti reviewed Jul 14, 2015
View reviewed changes

3y3 force-pushed the master branch 2 times, most recently from cd779b6 to e3d08ea Compare September 28, 2015 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated frontend: fixed search through Drawer panel. #621

Updated frontend: fixed search through Drawer panel. #621

marcominetti commented Apr 17, 2015

marcominetti commented Apr 22, 2015

3y3 commented May 12, 2015

marcominetti commented May 12, 2015

3y3 commented May 14, 2015

marcominetti commented May 14, 2015

marcominetti commented May 14, 2015

marcominetti commented May 14, 2015

3y3 commented May 15, 2015

marcominetti commented May 15, 2015

cr0cK commented May 15, 2015

a7madgamal commented Jul 1, 2015

marcominetti commented Jul 1, 2015

a7madgamal commented Jul 1, 2015

marcominetti commented Jul 1, 2015

3y3 commented Jul 1, 2015

yorkie Jul 1, 2015

3y3 Jul 1, 2015

3y3 Jul 1, 2015

a7madgamal commented Jul 1, 2015

3y3 commented Jul 1, 2015

marcominetti commented Jul 1, 2015

marcominetti commented Jul 1, 2015

marcominetti commented Jul 10, 2015

marcominetti commented Jul 14, 2015

marcominetti Jul 14, 2015

3y3 commented Jul 15, 2015

fakenickels commented Jan 8, 2016

marcominetti commented Jan 12, 2016

3y3 commented Jan 13, 2016

marcominetti commented Jan 13, 2016

a7madgamal commented Feb 9, 2016

Updated frontend: fixed search through Drawer panel. #621

Are you sure you want to change the base?

Updated frontend: fixed search through Drawer panel. #621

Conversation

marcominetti commented Apr 17, 2015

marcominetti commented Apr 22, 2015

3y3 commented May 12, 2015

marcominetti commented May 12, 2015

3y3 commented May 14, 2015

marcominetti commented May 14, 2015

marcominetti commented May 14, 2015

marcominetti commented May 14, 2015

3y3 commented May 15, 2015

marcominetti commented May 15, 2015

cr0cK commented May 15, 2015

a7madgamal commented Jul 1, 2015

marcominetti commented Jul 1, 2015

a7madgamal commented Jul 1, 2015

marcominetti commented Jul 1, 2015

3y3 commented Jul 1, 2015

yorkie Jul 1, 2015

Choose a reason for hiding this comment

3y3 Jul 1, 2015

Choose a reason for hiding this comment

3y3 Jul 1, 2015

Choose a reason for hiding this comment

a7madgamal commented Jul 1, 2015

3y3 commented Jul 1, 2015

marcominetti commented Jul 1, 2015

marcominetti commented Jul 1, 2015

marcominetti commented Jul 10, 2015

marcominetti commented Jul 14, 2015

marcominetti Jul 14, 2015

Choose a reason for hiding this comment

3y3 commented Jul 15, 2015

fakenickels commented Jan 8, 2016

marcominetti commented Jan 12, 2016

3y3 commented Jan 13, 2016

marcominetti commented Jan 13, 2016

a7madgamal commented Feb 9, 2016