Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

Add <code> to sensitive tags in collapse_whitespace_filter. #1774

Merged
merged 2 commits into from Jun 3, 2018
Merged

Add <code> to sensitive tags in collapse_whitespace_filter. #1774

merged 2 commits into from Jun 3, 2018

Conversation

Dreamsorcerer
Copy link
Contributor

@Dreamsorcerer Dreamsorcerer commented Jun 1, 2018

Fixes #1772

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@@ -39,7 +39,7 @@ namespace {
// Tags within which we should never try to collapse whitespace (note that this
// is not _quite_ the same thing as kLiteralTags in html_lexer.cc):
const HtmlName::Keyword kSensitiveTags[] = {
HtmlName::kPre, HtmlName::kScript, HtmlName::kStyle, HtmlName::kTextarea
HtmlName::kCode, HtmlName::kPre, HtmlName::kScript, HtmlName::kStyle, HtmlName::kTextarea
Copy link
Contributor

Choose a reason for hiding this comment

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

very cool. Could you also add a test in collapse_whitespace_filter_test.cc? The pattern should be fairly obvious there.

@googlebot
Copy link

CLAs look good, thanks!

@oschaaf
Copy link
Member

oschaaf commented Jun 1, 2018

Nice, thanks!

@jmarantz
Copy link
Contributor

jmarantz commented Jun 2, 2018

@oschaaf do you know why CI is failing? I re-started it once, but both attempts ended in:

ld/apache/incubator-pagespeed-mod/third_party/giflib/src'...
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I'll try a third time. If that fails we can I guess patch/build/test and then merge, but I wanted to see if you thought the CI was failing like this generally and if we should so something about that.

@oschaaf
Copy link
Member

oschaaf commented Jun 2, 2018

@jmarantz I noticed the same with another PR lately, but have not investigated yet. perhaps one of the dependencies broke, I’ll take a look later today

@oschaaf
Copy link
Member

oschaaf commented Jun 2, 2018

So the test failed on But do proxy content if the Content-Type header is present..
This was very reproducible because a required script that serves test responses was not running on modpagespeed.com. I recently rebuild the machine from scrap to simplify some things, and forgot all about that script. Restarted the CI job, expect this to be fixed.
(Though sometimes CI may still flake because of the tests requiring an amount of time that is pretty close to the timeout limit.)

@jmarantz
Copy link
Contributor

jmarantz commented Jun 3, 2018

We should separately address our CI issues. In the meantime, I patched this change and confirmed it passes its own test...running checkin.make now. Shall we just go ahead & merge?

@oschaaf
Copy link
Member

oschaaf commented Jun 3, 2018

@jmarantz +1 for merging

With regard to giflib, that started failing when sourceforge starting migrating
datacenters. I already attempted a fix [1] earlier because the previous location started
responding with 404 ..
But apparently the git repo we now point to times out every now and then, ugh.

[1] 8196fc7

@jmarantz jmarantz merged commit e7f3033 into apache:master Jun 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants