Skip to content
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

[FIX]Ignore cache requests & no track incognito host Issue #15 #34

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cguignol
Copy link
Contributor

Merge of Jolg42's work with recent evolution for Chrome + add testing.

Some of his work list adding "const" for the declaration of functions has not been added because of problems not solved about testability of the code.

…er#15

Merge of Jolg42's work with recent evolution for Chrome + add testing.
Jolg42
Jolg42 previously approved these changes Apr 14, 2020
Copy link

@Jolg42 Jolg42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't run the code but it looks ok (see suggestions)

script.js Outdated Show resolved Hide resolved
script.js Outdated Show resolved Hide resolved
script.js Outdated Show resolved Hide resolved
Co-Authored-By: Joël Galeran <Jolg42@users.noreply.github.com>
cguignol and others added 2 commits April 16, 2020 16:20
Co-Authored-By: Joël Galeran <Jolg42@users.noreply.github.com>
Co-Authored-By: Joël Galeran <Jolg42@users.noreply.github.com>
@cguignol
Copy link
Contributor Author

Thanks for the code review @Jolg42 , code smells better like that. I played automated test to verify that everything is OK.

@cguignol cguignol closed this Apr 16, 2020
@supertanuki
Copy link
Collaborator

@cguignol why this PR is closed?

@Jolg42
Copy link

Jolg42 commented May 12, 2020

Because it was "Merged" somehow 🤷‍♂️
https://github.com/cguignol/Carbonalyser/blob/master/script.js

@supertanuki
Copy link
Collaborator

@Jolg42 it's the forked repo

@Jolg42
Copy link

Jolg42 commented May 12, 2020

Oh yes indeed 😅

@cguignol
Copy link
Contributor Author

Smells like a mistake from me, I apologize and reopen the Pull Request ;)

@cguignol cguignol reopened this May 13, 2020
@@ -24,8 +24,14 @@ isChrome = () => {
};

headersReceivedListener = (requestDetails) => {
// Do not count bytes from requests from local cache
if (requestDetails.fromCache) return
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Suggested change
if (requestDetails.fromCache) return
if (requestDetails.fromCache || requestDetails.incognito) {
return;
}

And remove CONST_INCOGNITO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants