Skip to content

Conversation

@ENT8R
Copy link

@ENT8R ENT8R commented Oct 28, 2018

I hope everything is OK. (I didn't take a look into the whole codebase and don't know if this would be the right place to add it...)

@XhmikosR
Copy link
Collaborator

Please fix the test failures and rebase against upstream/master.

@ENT8R
Copy link
Author

ENT8R commented Mar 22, 2019

I fixed the indentation issues with fa17d5c
Github IDE screwed something up there... Sorry for that.

@XhmikosR
Copy link
Collaborator

XhmikosR commented Mar 22, 2019

Looks good now. One thing I don't know though because I haven't dug deep into the codebase myself either, is if we need to lowercase tags, their names and/or their values before the comparisons.

@alexlamsl
Copy link
Collaborator

HTTP headers are case insensitive, plus the follow two MDN links listed <meta> usage using different case schemes:
https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta

So I'd say we need to do a lower case comparison here. Also, can we get a test case or two for this new feature?

Just add a new section at the end of tests/minifier.js like so:

QUnit.test('minify Content-Security-Policy', function(assert) {
  assert.equal(minify(...), ...);
  ...
});
@ENT8R
Copy link
Author

ENT8R commented Mar 23, 2019

So now there are also some test cases and a case insensitive check with e066084

@alexlamsl alexlamsl closed this Mar 24, 2019
@alexlamsl alexlamsl reopened this Mar 24, 2019
@alexlamsl
Copy link
Collaborator

Changes LGTM − now if I can just figure out how to get the Cursed Integration job to start for this PR...

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

Labels

None yet

3 participants