Skip to content
This repository has been archived by the owner on Sep 15, 2021. It is now read-only.

perform scanning in a worker, fixes #7 #43

Merged
merged 2 commits into from
Mar 11, 2014
Merged

Conversation

mozfreddyb
Copy link

This will fix #7 and moves all scanning into a worker scanworker.js:
I had to change rules.js so that it doesn't require ScanJS to be loaded in the same scope (it's in the worker now!). We might want to solve this differently, if you guys don't like the current workaround.

I hope this request doesn't break pull request #42, otherwise I can happily rebase either.

@pwnetrationguru
Copy link

Looks like this can't be automatically merged into master after merging #42, so if you don't mine to fix conflicts and I'll review :)

@pwnetrationguru
Copy link

Also, just curious, does this now provide a loading animation while its running?

@mozfreddyb
Copy link
Author

Hm no. Do you think we should show a throbber while the worker is busy?
I should be able to introduce this, when I repair this branch.

@pwnetrationguru
Copy link

Yah! A throbber would be awesome.

Below is currently how our testing framework mocha does it, and I REALLY like it. Would also allow a user to start navigating finished files before scan is done. Might be a little bit more difficult so we could break it up into a separate PR. If you want to include it in this one, I'm totally good with that as they are related.

What do you think of throbber in the header below?

1

@mozfreddyb
Copy link
Author

I don't think percentage makes sense, as we have J of N files, and it should be a few seconds only. But a throbbler on the top right still makes sense, as well as the number of failures (we can't really measure failures).

I think we should do this in a different pull request though. I'm not really good with UI things and I'm unsure about how to tackle this right now. If you feel condifent, I'd actually prefer if you take a look at it, is that OK? :)

@pwnetrationguru
Copy link

@mozfreddyb, sounds good.

Let's just get the worker things merged in, and I can deal with a throbber in the UX uplift! :)

Conflicts:
	client/index.html
@pwnetrationguru
Copy link

from what I can tell, this lgtms 🐔

pwnetrationguru pushed a commit that referenced this pull request Mar 11, 2014
perform scanning in a worker, fixes #7
@pwnetrationguru pwnetrationguru merged commit 61a1cd4 into master Mar 11, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

execute the scan.js bits in a worker?
2 participants