3 ms·

I'm not a fan of the comments. There's comments that are utterly useless like: // Of course, there are other factors that can force a response to always be

I'm not a fan of the comments. There's comments that are utterly useless like: // Of course, there are other factors that can force a response to always be validated or re-fetched. Gee thanks, what might those factors be? A comment like that is worse than not commenting at all. Or the real ones that get my goat, comments which just tell you what the code that follows obviously does: // If there is no Date header, then assume that the server response was // generated at the time when we received the response. Time date_value; if (!GetDateValue(&date_value)) date_value = response_time; There's even a line (1019) where they divide by 10 but don't explain why they do it. That's what I personally would comment. The code itself is fairly easy to read (though I do wonder why they bothered using TimeDelta as it just seems to make the code more complicated, and results in confusing code like this:). return TimeDelta(); // not fresh

squeaky-clean 10y ago
I think the comment you mention makes perfect sense within the rest of its context. It's saying the rule for determining freshness is "response_is_fresh = (freshness_lifetime > current_age)", however that's not always the case. And looking at the code, you can see it returns true early if lifetime is equal to a new TimeDelta. The GetFreshnessLifetime function below it then covers the additional cases where it returns that. Such as the headers being set to not cache, or the expiry time being earlier than the response's time (or current time if none is provided). I think it also makes sense to assume the comment is letting us know that just because HttpResponseHeaders::RequiresValidation returns false, that doesn't mean that's the only thing that can make it require/not require a re-fetch. > There's even a line (1019) where they divide by 10 but don't explain why they do it. That's what I personally would comment. This is covered near the head of the function, line 951. Using a constant such as heuristic_scalar instead of simply using '10' would be more readable though.
groby_b 10y ago
While I'm not on the net team - I just mangle the UI and annoy Ben ;) - I can wager a good guess: The net team used TimeDelta because for any software project of Chromium's size strong types are crucial. If they just returned an int, you'll sooner or later see it passed through five different places, and at the end location, nobody remembers if it's ms, time ticks, seconds, or even a unix time instead of a delta. TimeDelta removes that question. It also does things that have subtle issues you might miss if you did it "by hand", like saturated adds, multiplying with an integer value while handling overflow correctly, etc. These things might be overkill for smaller project, but once you have something with hundreds of contributors, every little bit helps keep the code base sane. As for the division by 10 - look up at line 953 :) Yes, it should be a named constant. And some of the comments could certainly be better. It's a work in progress. (And if you want to help with that work, we happily accept patches!)
- mattmanser 10y ago
  Yes, that seems like a good reason to use TimeDelta.

squeaky-clean 10y ago

I think the comment you mention makes perfect sense within the rest of its context. It's saying the rule for determining freshness is "response_is_fresh = (freshness_lifetime > current_age)", however that's not always the case. And looking at the code, you can see it returns true early if lifetime is equal to a new TimeDelta. The GetFreshnessLifetime function below it then covers the additional cases where it returns that. Such as the headers being set to not cache, or the expiry time being earlier than the response's time (or current time if none is provided). I think it also makes sense to assume the comment is letting us know that just because HttpResponseHeaders::RequiresValidation returns false, that doesn't mean that's the only thing that can make it require/not require a re-fetch. > There's even a line (1019) where they divide by 10 but don't explain why they do it. That's what I personally would comment. This is covered near the head of the function, line 951. Using a constant such as heuristic_scalar instead of simply using '10' would be more readable though.

groby_b 10y ago

While I'm not on the net team - I just mangle the UI and annoy Ben ;) - I can wager a good guess: The net team used TimeDelta because for any software project of Chromium's size strong types are crucial. If they just returned an int, you'll sooner or later see it passed through five different places, and at the end location, nobody remembers if it's ms, time ticks, seconds, or even a unix time instead of a delta. TimeDelta removes that question. It also does things that have subtle issues you might miss if you did it "by hand", like saturated adds, multiplying with an integer value while handling overflow correctly, etc. These things might be overkill for smaller project, but once you have something with hundreds of contributors, every little bit helps keep the code base sane. As for the division by 10 - look up at line 953 :) Yes, it should be a named constant. And some of the comments could certainly be better. It's a work in progress. (And if you want to help with that work, we happily accept patches!)

mattmanser 10y ago
Yes, that seems like a good reason to use TimeDelta.