-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ignore cache requests & no track incognito host #15
Conversation
Hi @Jolg42 and thanks for this contribution. I will test it as soon as possible. |
This is in deed very important in order to have meaningful results. Thanks @Jolg42 I can't wait to see the difference. Meanwhile, however it's great work @supertanuki I believe it should be mentioned somewhere that these data are not be taken "as is" for the moment. |
Is there any problem holding the PR back? It should make quite a difference on the estimated footprint. |
We will take a look on this PR. There is a conflict to revolve due to the chrome support code modification |
Hi @Jolg42 , I've made at the end of 2019 several modifications about chrome extension support. I watched your code, and he's a bit far from the current code from master branch. I think you should get the more recent from the branch and redo your modifications... so I'm here to help you 👍 I made several modifications from your work this afternoon, and it seems that I succeeded not only to merge your code, but also I made test alignement to maintain a acceptable test coverage. Can you contact me directly to see how I can properly give you my code for a merge? I'm a bit rusted with Gitlab 👴 |
Hi @cguignol 😄 I guess you can commit it directly and close this PR. That would be easier. I'm curious to see the change, please send a link here if you can 😉 |
I had to remove several "const" statements because it prevents the code to be tested (function call and mock problems). If you find a solution, be pleased to make the modifications. I'll make the commit soon ;) |
…er#15 Merge of Jolg42's work with recent evolution for Chrome + add testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code has been merged here : #34 (comment)
Is there something preventing this one to be merged ? I'm also maintaining my own version locally to have more accurate results, I'd like to see it upstream so everyone can benefit from it |
Fixes #12 as requests
fromCache: true
are noew ignored.Replace host name by Incognito if the request was made in Incognito mode.
Voilà 😄