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

Improvements catching images #32

Merged
merged 4 commits into from
Oct 4, 2019
Merged

Improvements catching images #32

merged 4 commits into from
Oct 4, 2019

Conversation

brianvanburken
Copy link
Contributor

I've used and tested, for a month, my fork of the extension containing a lot of changes. During this period I tested improvements for the following goals:

  1. catch more images with improved checking for cases;
  2. better feedback when the extension is enabled/disabled based on the current page;
  3. extension working all the time (bug reported in Extension occasionally stops working #23);

Also, to better understand the extension, I've added more documentation about cases and why it does some things a certain way.

For point 1 I've improved the shouldCompress.js with cases I came across. At the time of writing, I've used my fork for 32 days with the compression level set to 20% and have reached the following statistics: 55,747 images processed and 2.24GB data saved (91%). There is one case not added to this PR that catches more images: some images have a content type of application/octet-stream (example page: https://code.google.com/archive/p/repo-clean/).

Point 2, was tackled by also checking if the current page is from a private network or has a different protocol (for pages like about:config etc.).

Point 3, seems to be resolved. During my 32 days, I've never caught the bug. Before I caught it within two weeks. More testing is needed. The problem, from my debugging a lot, seems to originate from attaching and detaching the listeners. In this PR, I attach the listeners once on startup and keep it that way. It does use a bit more memory and CPU when disabled (since it still listens to images). But, this only minimally since we exit early in the onBeforeRequest listener by checking if the extension is enabled. If the extension is disabled/removed by the user, the browser will clean up automatically (tested by adding debug points and disabling/removing the extension).

Copy link

@mandaputtra mandaputtra left a comment

Choose a reason for hiding this comment

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

Just found some typos

src/background.js Outdated Show resolved Hide resolved
@ayastreb ayastreb merged commit aa8536e into ayastreb:master Oct 4, 2019
@ayastreb
Copy link
Owner

ayastreb commented Oct 4, 2019

This looks great, thanks for contributing again!

@brianvanburken brianvanburken deleted the improvements-catching-images branch October 4, 2019 07:13
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.

3 participants