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

Implement dictionary handling #77

Merged
merged 1 commit into from Mar 31, 2016

Conversation

Projects
None yet
2 participants
@dignifiedquire
Contributor

dignifiedquire commented Mar 29, 2016

This adds inflateSetDictionary and deflateSetDictionary.

@dignifiedquire dignifiedquire changed the title from Dict to Implement dictionary handling Mar 29, 2016

@dignifiedquire dignifiedquire referenced this pull request Mar 29, 2016

Merged

Testing libp2p-spdy on the browser #6

2 of 2 tasks complete
Show outdated Hide outdated lib/deflate.js
@@ -143,6 +144,7 @@ function Deflate(options) {
this.msg = ''; // error message
this.ended = false; // used to avoid multiple onEnd() calls
this.chunks = []; // chunks of compressed data
this._dict_set = false; // have we set the dictionary

This comment has been minimized.

@puzrin

puzrin Mar 29, 2016

Member

This flag seems to be completely useless.

@puzrin

puzrin Mar 29, 2016

Member

This flag seems to be completely useless.

This comment has been minimized.

@dignifiedquire

dignifiedquire Mar 29, 2016

Contributor

I use it below to ensure we are only calling setHeaderon the first push, or am I misunderstanding sth?

@dignifiedquire

dignifiedquire Mar 29, 2016

Contributor

I use it below to ensure we are only calling setHeaderon the first push, or am I misunderstanding sth?

This comment has been minimized.

@puzrin

puzrin Mar 29, 2016

Member

Search for this var name in editor. It's used in constructor only. deflateSetHeader() is also called in constructor only.

I don't know what you meaned, but this code is not used in push anyhow. At least, now.

@puzrin

puzrin Mar 29, 2016

Member

Search for this var name in editor. It's used in constructor only. deflateSetHeader() is also called in constructor only.

I don't know what you meaned, but this code is not used in push anyhow. At least, now.

This comment has been minimized.

@dignifiedquire

dignifiedquire Mar 29, 2016

Contributor

oh boy, it was late I guess -.- sorry about that

@dignifiedquire

dignifiedquire Mar 29, 2016

Contributor

oh boy, it was late I guess -.- sorry about that

Show outdated Hide outdated lib/zlib/deflate.js
s.insert = 0;
}
/* use the tail */
dictionary = dictionary.slice(dictLength - s.w_size);

This comment has been minimized.

@puzrin

puzrin Mar 29, 2016

Member

Not all browsers support Uint8Array.slice()

@puzrin

puzrin Mar 29, 2016

Member

Not all browsers support Uint8Array.slice()

This comment has been minimized.

@puzrin

puzrin Mar 29, 2016

Member
  • If speed is NOT deadly critical here, use utils.Buf8 + for for copy.
  • If speed IS deadly critical here - check if .subarray exists and use it (with .set if need true copy), with fallback to for.

PS. I guess speed is not highest priority for this code path (note that for is fast enougth for typed arrays)

@puzrin

puzrin Mar 29, 2016

Member
  • If speed is NOT deadly critical here, use utils.Buf8 + for for copy.
  • If speed IS deadly critical here - check if .subarray exists and use it (with .set if need true copy), with fallback to for.

PS. I guess speed is not highest priority for this code path (note that for is fast enougth for typed arrays)

This comment has been minimized.

@dignifiedquire

dignifiedquire Mar 29, 2016

Contributor

This should be called only once per setHeader call, and even then it might not be called at all, so will go the for route.

@dignifiedquire

dignifiedquire Mar 29, 2016

Contributor

This should be called only once per setHeader call, and even then it might not be called at all, so will go the for route.

Show outdated Hide outdated lib/zlib/deflate.js
@@ -1745,6 +1746,88 @@ function deflateEnd(strm) {
//
//}
/* ========================================================================= */

This comment has been minimized.

@puzrin

puzrin Mar 29, 2016

Member

This separator line seems to be out of style

@puzrin

puzrin Mar 29, 2016

Member

This separator line seems to be out of style

Show outdated Hide outdated lib/zlib/inflate.js
@@ -1490,13 +1525,13 @@ exports.inflate = inflate;
exports.inflateEnd = inflateEnd;
exports.inflateGetHeader = inflateGetHeader;
exports.inflateInfo = 'pako inflate (from Nodeca project)';
exports.inflateGetDictionary = inflateGetDictionary;

This comment has been minimized.

@puzrin

puzrin Mar 29, 2016

Member

inflateGetDictionary is still not implemented, should not be here.

@puzrin

puzrin Mar 29, 2016

Member

inflateGetDictionary is still not implemented, should not be here.

Show outdated Hide outdated test/helpers.js
'ation/xhtmltext/plainpublicmax-agecharset=iso-8859-1utf-8gzipdeflateHTTP/1',
'.1statusversionurl\x00'
].join(''));

This comment has been minimized.

@puzrin

puzrin Mar 29, 2016

Member

IMHO such constant should be in fixtures folder instead of helpers.

@puzrin

puzrin Mar 29, 2016

Member

IMHO such constant should be in fixtures folder instead of helpers.

@puzrin

This comment has been minimized.

Show comment
Hide comment
@puzrin

puzrin Mar 29, 2016

Member

Looks good, but needs some minor polish:

  • See comments above
  • Please fix tests (i guess, you executed those manually without linter, see make test)
  • Generate coverage report (see make cover) and check that code added by you is well covered with tests.
Member

puzrin commented Mar 29, 2016

Looks good, but needs some minor polish:

  • See comments above
  • Please fix tests (i guess, you executed those manually without linter, see make test)
  • Generate coverage report (see make cover) and check that code added by you is well covered with tests.
@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Mar 29, 2016

Contributor

Thanks for the quick review, I will address the comments tomorrow.

Contributor

dignifiedquire commented Mar 29, 2016

Thanks for the quick review, I will address the comments tomorrow.

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Mar 29, 2016

Contributor

@puzrin Okay I think I've addressed everything that you mentioned. Let me know when you think it's ready then I'll squash the commits one last time.

Contributor

dignifiedquire commented Mar 29, 2016

@puzrin Okay I think I've addressed everything that you mentioned. Let me know when you think it's ready then I'll squash the commits one last time.

@puzrin

This comment has been minimized.

Show comment
Hide comment
@puzrin

puzrin Mar 29, 2016

Member

Reviewed, LGTM.

Please squash and i'll review again for sure.

Member

puzrin commented Mar 29, 2016

Reviewed, LGTM.

Please squash and i'll review again for sure.

Implement dictionary handling.
This adds the methods

- `deflateSetDictionary`
- `inflateSetDictionary`

as well as calling `setDictionary` at the right point when passing
the a `dictionary` option to one of

- `pako.deflate`
- `new pako.Deflate`
- `pako.inflate`
- `new pako.Inflate`
@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire
Contributor

dignifiedquire commented Mar 30, 2016

@puzrin done

@puzrin puzrin merged commit b40c7cd into nodeca:master Mar 31, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
s.wrap = 0; /* avoid computing Adler-32 in read_buf */
/* if dictionary would fill window, just replace the history */
if (dictLength >= s.w_size) {

This comment has been minimized.

@puzrin

puzrin Mar 31, 2016

Member

This branch is not covered with tests. That's not requirement, but it you have a time - take a look, if this can be covered.

@puzrin

puzrin Mar 31, 2016

Member

This branch is not covered with tests. That's not requirement, but it you have a time - take a look, if this can be covered.

@puzrin

This comment has been minimized.

Show comment
Hide comment
@puzrin

puzrin Mar 31, 2016

Member

Merged. Thanks a lot for your efforts.

Should i do release immediately or do you need some time to test this feature with your libs?

Member

puzrin commented Mar 31, 2016

Merged. Thanks a lot for your efforts.

Should i do release immediately or do you need some time to test this feature with your libs?

@dignifiedquire dignifiedquire deleted the dignifiedquire:dict branch Apr 1, 2016

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Apr 1, 2016

Contributor

Thanks for merging and the great project! It would be great if you could publish a new version, as I'm currently working on bringing `browserify-zlib up to date.

Contributor

dignifiedquire commented Apr 1, 2016

Thanks for merging and the great project! It would be great if you could publish a new version, as I'm currently working on bringing `browserify-zlib up to date.

@puzrin

This comment has been minimized.

Show comment
Hide comment
@puzrin

puzrin Apr 1, 2016

Member

Published 1.0.1.

Member

puzrin commented Apr 1, 2016

Published 1.0.1.

@RichardLitt RichardLitt referenced this pull request Apr 20, 2016

Open

IPFS Weekly Updates #151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment