Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10387200:
Suppress pause-and-buffer behavior when the HTTP response won't satisfy future requests via cache. (Closed)
|
Created:
8 years, 7 months ago by Ami GONE FROM CHROMIUM Modified:
8 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSuppress pause-and-buffer behavior when the HTTP response won't satisfy future requests via cache.
BUG=123074
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138972
Patch Set 1 #
Total comments: 17
Patch Set 2 : CR responses. #Patch Set 3 : Alternative implementation requiring no further WebKit changes #
Total comments: 2
Patch Set 4 : Extracted GetReasonsForUncacheability and added tests. #
Total comments: 33
Patch Set 5 : rvargas CR #Patch Set 6 : indent-to-first-paren #
Total comments: 2
Patch Set 7 : Bitfield for reasons and fix typo picking out the Date header. #
Total comments: 3
Patch Set 8 : vector? we don't need no stinkin' vector\! #Patch Set 9 : Wow! Sometimes win_rel CQ failure actually means something other than flake! #Patch Set 10 : %zu is win-unfriendly; use %PRIuS instead. #
Messages
Total messages: 50 (0 generated)
This patch relies on issue 10383201 and http://webk.it/86917 landing first. Ricardo/Andrew: this is a speculative CL. WDYT? It seems to work well for the specific case we've seen most in corp (HTTP/1.0+206).
looks reasonable to me. https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... File webkit/media/buffered_resource_loader.cc (right): https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:371: kMaxReason = kNoCache + 1 // Always keep equal to the greatest value plus 1. nit: don't set a value? https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:407: if (response.cacheControlContainsNoCache()) does this check for no-store also? https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:413: UMA_HISTOGRAM_ENUMERATION("Media.OneOffReason", reasons[i], kMaxReason); note that there will be no way to tell what percentage of entries are "good".
https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... File webkit/media/buffered_resource_loader.cc (right): https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:364: kPre11PartialResponse = 2, // 206 but HTTP version < 1.1. nit: I read this as "Prell" (pee-are-eee-ell-ell) I don't know if "PreOneOne" is any better though! https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:383: if (version < WebURLResponse::HTTP_1_1 && code != kHttpOK) since this isn't an else branch wouldn't this statement be true if version was <1.1 and code was, say, 404? speaking of which, do we want to include non-200/206 responses in our counts? won't other random responses trickle into our stats and increment counts? https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:385: if (!response.hasCacheValidatorFields()) { OOC is this is ETag && Last-Modified or ETag || Last-Modified? https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:435: MightBeReusedFromCacheInFuture(response); do we want to log these requests for all protocols or just http/https? I would think the latter! https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:609: if (!might_be_reused_from_cache_in_future_ && strategy == kNeverDefer) I'm this -->||<-- close to thinking we should just remove this as slick as this change is... what if things change? what if we do decide to cache more aggressively during playback? The logging is informative but do wonder if we're better off removing kNeverDefer
Thanks for the quick reviews! https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... File webkit/media/buffered_resource_loader.cc (right): https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:364: kPre11PartialResponse = 2, // 206 but HTTP version < 1.1. On 2012/05/19 02:26:24, scherkus wrote: > nit: I read this as "Prell" (pee-are-eee-ell-ell) > > I don't know if "PreOneOne" is any better though! ...yeah. Leaving as is. https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:371: kMaxReason = kNoCache + 1 // Always keep equal to the greatest value plus 1. On 2012/05/19 01:19:41, rvargas wrote: > nit: don't set a value? Hah! Done. https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:383: if (version < WebURLResponse::HTTP_1_1 && code != kHttpOK) On 2012/05/19 02:26:24, scherkus wrote: > since this isn't an else branch wouldn't this statement be true if version was > <1.1 and code was, say, 404? Oops; leftover from previous code. > speaking of which, do we want to include non-200/206 responses in our counts? > won't other random responses trickle into our stats and increment counts? That's the point of kNoData (so we can account for them). https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:385: if (!response.hasCacheValidatorFields()) { On 2012/05/19 02:26:24, scherkus wrote: > OOC is this is ETag && Last-Modified or ETag || Last-Modified? || http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/... https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:407: if (response.cacheControlContainsNoCache()) On 2012/05/19 01:19:41, rvargas wrote: > does this check for no-store also? Nope. Thought about it and thought I didn't need it, but in retrospect, I do. https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:413: UMA_HISTOGRAM_ENUMERATION("Media.OneOffReason", reasons[i], kMaxReason); On 2012/05/19 01:19:41, rvargas wrote: > note that there will be no way to tell what percentage of entries are "good". Good point. Reworked slightly. https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:435: MightBeReusedFromCacheInFuture(response); On 2012/05/19 02:26:24, scherkus wrote: > do we want to log these requests for all protocols or just http/https? I would > think the latter! Hmm. On one hand HTTP will drown out all other stats. On the other cache/defer decisions aren't being made differently for http vs. non-http. Do you have an idea that that should change? https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:609: if (!might_be_reused_from_cache_in_future_ && strategy == kNeverDefer) On 2012/05/19 02:26:24, scherkus wrote: > I'm this -->||<-- close to thinking we should just remove this > > as slick as this change is... what if things change? what if we do decide to > cache more aggressively during playback? > > The logging is informative but do wonder if we're better off removing > kNeverDefer I don't want to drop pause-and-buffer where it works well, since it's a compelling user-visible feature.
lgtm -- it's true http/https will drown out responses but I feel there is some more work we need to do here (can you file bugs for the local source / not-pause-and-buffer-if-the-source-doesnt-involve-the-network?) https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... File webkit/media/buffered_resource_loader.cc (right): https://chromiumcodereview.appspot.com/10387200/diff/1/webkit/media/buffered_... webkit/media/buffered_resource_loader.cc:435: MightBeReusedFromCacheInFuture(response); On 2012/05/19 03:03:02, Ami Fischman wrote: > On 2012/05/19 02:26:24, scherkus wrote: > > do we want to log these requests for all protocols or just http/https? I would > > think the latter! > > Hmm. On one hand HTTP will drown out all other stats. On the other cache/defer > decisions aren't being made differently for http vs. non-http. Do you have an > idea that that should change? Maybe. For example, we report 100% buffered when WMPI::is_local_source_ is true. Pause-and-buffering (and needlessly transmitting bytes between browser<->renderer) is tantamount to doing unnecessary work with zero benefit.
Darin: PS#3 has the result of our IM from earlier today. WDYT? scherkus/rvargas: darin@ suggested that RRB's WebCore-internal-facing interface is likely the wrong thing to expose from WebKit's WebURLResponse, and that instead I could re-use net/http's logic myself by peeking at headers. Let me know if you see a problem with this approach. This patchset doesn't require any further WebKit changes.
> > can you file bugs for the local source / > not-pause-and-buffer-if-the-**source-doesnt-involve-the-**network? > Filed https://code.google.com/p/chromium/issues/detail?id=129120
https://chromiumcodereview.appspot.com/10387200/diff/9002/webkit/media/buffer... File webkit/media/buffered_resource_loader.cc (right): https://chromiumcodereview.appspot.com/10387200/diff/9002/webkit/media/buffer... webkit/media/buffered_resource_loader.cc:381: static bool MightBeReusedFromCacheInFuture(const WebURLResponse& response) { style nit: usually best not to interrupt the definition of class member functions with the definition of a static function. better to put static functions at the top of the file. that helps the reader not get as lost, and it encourages others to do the same. you might also consider moving this function to a helper file and then add a unit test. we might also consider adding a function like this to net/http/. https://chromiumcodereview.appspot.com/10387200/diff/9002/webkit/media/buffer... webkit/media/buffered_resource_loader.cc:382: std::vector<UncacheableReason> reasons; what's the point of building up this vector? why don't you just return early at each push_back? is there some future version of this code that will care about the reasons?
> https://chromiumcodereview.appspot.com/10387200/diff/9002/webkit/media/buffer... > File webkit/media/buffered_resource_loader.cc (right): > > https://chromiumcodereview.appspot.com/10387200/diff/9002/webkit/media/buffer... > webkit/media/buffered_resource_loader.cc:381: static bool > MightBeReusedFromCacheInFuture(const WebURLResponse& response) { > style nit: usually best not to interrupt the definition of class member > functions with the definition of a static function. better to put > static functions at the top of the file. Counterargument is to define single-use things nearest to their (only) use, much like variable declarations, and for the same reasons. > you might also > consider moving this function to a helper file and then add a unit test. > we might also consider adding a function like this to net/http/. Happy to move it when we get a second customer. > https://chromiumcodereview.appspot.com/10387200/diff/9002/webkit/media/buffer... > webkit/media/buffered_resource_loader.cc:382: > std::vector<UncacheableReason> reasons; > what's the point of building up this vector? why don't you just return > early at each push_back? is there some future version of this code that > will care about the reasons? I want to report all the disqualifying reasons to uma, not just the first I notice (in the arbitrary order the code happens to be in). This will enable making better decisions in the future about what use cases to go after next (pick them off in descending order of impact).
BTW an alternative I considered was giving HttpResponseHeaders an additional constructor that would take a function similar to HRH::EnumerateHeader() for getting header text, and feeding it a wrapper around WebURLResponse::httpHeaderField() (also, the new ctor would need to accept an HTTP status line or the components thereof). I backed off that idea since it seemed pretty invasive to HRH, and because it would mean future HRH development would be constrained to what could be divined from these sources. Let me know if you think the advantages outweigh the costs.
A better approach seems to be to go the other way around: leave the actual implementation of HasStrongValidators unchanged, and instead add something to http_util that builds the response headers and calls the proper method.
On Tue, May 22, 2012 at 9:43 AM, <rvargas@chromium.org> wrote: > A better approach seems to be to go the other way around: leave the actual > implementation of HasStrongValidators unchanged, and instead add something > to > http_util that builds the response headers and calls the proper method. > The only way to build an HRH is to concatenate the headers from WebURLResponse following a reconstructed status line. Somehow that seems like madness to me. If you & Darin think that's better, I'll do it. (in that case, I think no HttpUtil changes are necessary; simply construct an HRH from WUR in webkit/media and call the HRH methods directly). > http://codereview.chromium.**org/10387200/<http://codereview.chromium.org/103... >
> > build an HRH is to concatenate the headers from WebURLResponse following a > reconstructed status line. BTW, doing that also means losing the UMA reporting of the reasons for cache-unusefulness, which I was rather hoping to keep (as a way to plan future work). (replicating the UMA reporting in net/http code is possible but would require inspecting all the conditions for RequiresValidation and friends, not early-returning as soon as we have an answer; there is commentary in HttpCache::Transaction::RequiresValidation that makes me think you wouldn't want that)
On Tue, May 22, 2012 at 1:30 AM, Ami Fischman <fischman@chromium.org> wrote: > > > > https://chromiumcodereview.appspot.com/10387200/diff/9002/webkit/media/buffer... > > File webkit/media/buffered_resource_loader.cc (right): > > > > > https://chromiumcodereview.appspot.com/10387200/diff/9002/webkit/media/buffer... > > webkit/media/buffered_resource_loader.cc:381: static bool > > MightBeReusedFromCacheInFuture(const WebURLResponse& response) { > > style nit: usually best not to interrupt the definition of class member > > functions with the definition of a static function. better to put > > static functions at the top of the file. > > Counterargument is to define single-use things nearest to their (only) > use, much like variable declarations, and for the same reasons. > I've just seen these things grow hairs when lots of people hack collectively on the same code. Pretty soon you have a jumbled mess. People start putting code anywhere. The order is lost. The rule is that you should define functions in the .cc file in the same order they are declared in the .h file. This helps keep things organized. Helpers can then go at the top of the .cc file, or if they are substantial, then they should get their own file so that they can be unit tested. > you might also > > consider moving this function to a helper file and then add a unit test. > > we might also consider adding a function like this to net/http/. > > Happy to move it when we get a second customer. > second customer is not the only reason. the main reason is to enable unit testing. this function you are adding is fairly complex. it is begging to be unit tested. > > > https://chromiumcodereview.appspot.com/10387200/diff/9002/webkit/media/buffer... > > webkit/media/buffered_resource_loader.cc:382: > > std::vector<UncacheableReason> reasons; > > what's the point of building up this vector? why don't you just return > > early at each push_back? is there some future version of this code that > > will care about the reasons? > > I want to report all the disqualifying reasons to uma, not just the first > I notice (in the arbitrary order the code happens to be in). This will > enable making better decisions in the future about what use cases to go > after next (pick them off in descending order of impact). > I see. -Darin
WebURLResponse keeps a hash map of header / value pairs. That is a lossy representation of response headers! Converting back to HttpResponseHeaders can be done, but it may lose some information. Not sure if that matters here. Probably not for analysis of cache headers, but I'd be worried about someone using it for something else. -Darin On Tue, May 22, 2012 at 9:43 AM, <rvargas@chromium.org> wrote: > A better approach seems to be to go the other way around: leave the actual > implementation of HasStrongValidators unchanged, and instead add something > to > http_util that builds the response headers and calls the proper method. > > http://codereview.chromium.**org/10387200/<http://codereview.chromium.org/103... >
Just so that it's clear, I'm not suggesting exposing the reconstructed HttpResponseHeaders to anyone, nor changing the code at the BufferedReaourceLoader (that deals with WebURLResponse). Just having the HttpUtil method (with the proposed signature) call HttpResponseHeaders, where I believe the HasStrongValidators code belongs. (I tend to see HttpUtil code either as useful general-purpose code, or code that we don't know where to put, and this is neither). On 2012/05/22 18:09:26, darin wrote: > WebURLResponse keeps a hash map of header / value pairs. That is a lossy > representation of response headers! Converting back to HttpResponseHeaders > can be done, but it may lose some information. Not sure if that matters > here. Probably not for analysis of cache headers, but I'd be worried about > someone using it for something else. > > -Darin > > > On Tue, May 22, 2012 at 9:43 AM, <mailto:rvargas@chromium.org> wrote: > > > A better approach seems to be to go the other way around: leave the actual > > implementation of HasStrongValidators unchanged, and instead add something > > to > > http_util that builds the response headers and calls the proper method. > > > > > http://codereview.chromium.**org/10387200/%3Chttp://codereview.chromium.org/1...> > >
Are you suggesting making the method be static on HttpResponseHeaders so it can be called without an instance of HttpResponseHeaders? I thought you were suggesting that we allocate a HttpResponseHeaders in order to call methods on HttpResponseHeaders. -Darin On Tue, May 22, 2012 at 1:15 PM, <rvargas@chromium.org> wrote: > Just so that it's clear, I'm not suggesting exposing the reconstructed > HttpResponseHeaders to anyone, nor changing the code at the > BufferedReaourceLoader (that deals with WebURLResponse). > > Just having the HttpUtil method (with the proposed signature) call > HttpResponseHeaders, where I believe the HasStrongValidators code belongs. > > (I tend to see HttpUtil code either as useful general-purpose code, or > code that > we don't know where to put, and this is neither). > > > On 2012/05/22 18:09:26, darin wrote: > >> WebURLResponse keeps a hash map of header / value pairs. That is a lossy >> representation of response headers! Converting back to >> HttpResponseHeaders >> can be done, but it may lose some information. Not sure if that matters >> here. Probably not for analysis of cache headers, but I'd be worried >> about >> someone using it for something else. >> > > -Darin >> > > > On Tue, May 22, 2012 at 9:43 AM, <mailto:rvargas@chromium.org> wrote: >> > > > A better approach seems to be to go the other way around: leave the >> actual >> > implementation of HasStrongValidators unchanged, and instead add >> something >> > to >> > http_util that builds the response headers and calls the proper method. >> > >> > >> > > http://codereview.chromium.****org/10387200/%3Chttp://coderev** > iew.chromium.org/10387200/ <http://codereview.chromium.org/10387200/>> > >> > >> > > > http://codereview.chromium.**org/10387200/<http://codereview.chromium.org/103... >
On 2012/05/22 21:31:14, darin wrote:
> Are you suggesting making the method be static on HttpResponseHeaders so it
> can be called without an instance of HttpResponseHeaders? I thought you
> were suggesting that we allocate a HttpResponseHeaders in order to call
> methods on HttpResponseHeaders.
No static member:
static bool HttpUtil::HasStrongValidators(HttpVersion version, std::string
etag...) {
...
std::string raw_headers = AssemblerawHeaders(...)
HttpResponseHeaders headers(raw_headers);
return headers->HasStrongValidators();
}
So yes, we allocate an HttpResponseHeaders but that's part of the
implementation.
I guess it's a minor point and judging by this thread, probably not worth it.
>
> -Darin
>
> On Tue, May 22, 2012 at 1:15 PM, <mailto:rvargas@chromium.org> wrote:
>
> > Just so that it's clear, I'm not suggesting exposing the reconstructed
> > HttpResponseHeaders to anyone, nor changing the code at the
> > BufferedReaourceLoader (that deals with WebURLResponse).
> >
> > Just having the HttpUtil method (with the proposed signature) call
> > HttpResponseHeaders, where I believe the HasStrongValidators code belongs.
> >
> > (I tend to see HttpUtil code either as useful general-purpose code, or
> > code that
> > we don't know where to put, and this is neither).
> >
> >
> > On 2012/05/22 18:09:26, darin wrote:
> >
> >> WebURLResponse keeps a hash map of header / value pairs. That is a lossy
> >> representation of response headers! Converting back to
> >> HttpResponseHeaders
> >> can be done, but it may lose some information. Not sure if that matters
> >> here. Probably not for analysis of cache headers, but I'd be worried
> >> about
> >> someone using it for something else.
> >>
> >
> > -Darin
> >>
> >
> >
> > On Tue, May 22, 2012 at 9:43 AM, <mailto:rvargas@chromium.org> wrote:
> >>
> >
> > > A better approach seems to be to go the other way around: leave the
> >> actual
> >> > implementation of HasStrongValidators unchanged, and instead add
> >> something
> >> > to
> >> > http_util that builds the response headers and calls the proper method.
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.****org/10387200/%253Chttp://coderev**
> > iew.chromium.org/10387200/ <http://codereview.chromium.org/10387200/>>
> >
> >> >
> >>
> >
> >
> >
>
http://codereview.chromium.**org/10387200/%3Chttp://codereview.chromium.org/1...>
> >
On Tue, May 22, 2012 at 2:58 PM, <rvargas@chromium.org> wrote: > On 2012/05/22 21:31:14, darin wrote: > >> Are you suggesting making the method be static on HttpResponseHeaders so >> it >> can be called without an instance of HttpResponseHeaders? I thought you >> were suggesting that we allocate a HttpResponseHeaders in order to call >> methods on HttpResponseHeaders. >> > > No static member: > > static bool HttpUtil::HasStrongValidators(**HttpVersion version, > std::string > etag...) { > ... > std::string raw_headers = AssemblerawHeaders(...) > HttpResponseHeaders headers(raw_headers); > return headers->HasStrongValidators()**; > } > > So yes, we allocate an HttpResponseHeaders but that's part of the > implementation. > > I guess it's a minor point and judging by this thread, probably not worth > it. > Oh, I see... when I created HttpUtil, it was just tools used by HttpResponseHeaders. That's why I was thinking it made sense to extract bits of HttpResponseHeaders out into HttpUtil. (The name HttpUtil has always sucked, sorry!!) -Darin > > > -Darin >> > > On Tue, May 22, 2012 at 1:15 PM, <mailto:rvargas@chromium.org> wrote: >> > > > Just so that it's clear, I'm not suggesting exposing the reconstructed >> > HttpResponseHeaders to anyone, nor changing the code at the >> > BufferedReaourceLoader (that deals with WebURLResponse). >> > >> > Just having the HttpUtil method (with the proposed signature) call >> > HttpResponseHeaders, where I believe the HasStrongValidators code >> belongs. >> > >> > (I tend to see HttpUtil code either as useful general-purpose code, or >> > code that >> > we don't know where to put, and this is neither). >> > >> > >> > On 2012/05/22 18:09:26, darin wrote: >> > >> >> WebURLResponse keeps a hash map of header / value pairs. That is a >> lossy >> >> representation of response headers! Converting back to >> >> HttpResponseHeaders >> >> can be done, but it may lose some information. Not sure if that >> matters >> >> here. Probably not for analysis of cache headers, but I'd be worried >> >> about >> >> someone using it for something else. >> >> >> > >> > -Darin >> >> >> > >> > >> > On Tue, May 22, 2012 at 9:43 AM, <mailto:rvargas@chromium.org> wrote: >> >> >> > >> > > A better approach seems to be to go the other way around: leave the >> >> actual >> >> > implementation of HasStrongValidators unchanged, and instead add >> >> something >> >> > to >> >> > http_util that builds the response headers and calls the proper >> method. >> >> > >> >> > >> >> >> > >> > http://codereview.chromium.******org/10387200/%253Chttp://**coderev** >> > iew.chromium.org/10387200/ <http://codereview.chromium.**org/10387200/<http://codereview.chromium.org/103... >> >> >> > >> >> > >> >> >> > >> > >> > >> > > http://codereview.chromium.****org/10387200/%3Chttp://coderev** > iew.chromium.org/10387200/ <http://codereview.chromium.org/10387200/>> > >> > >> > > > http://codereview.chromium.**org/10387200/<http://codereview.chromium.org/103... >
On Tue, May 22, 2012 at 3:09 PM, Darin Fisher <darin@chromium.org> wrote: > > > On Tue, May 22, 2012 at 2:58 PM, <rvargas@chromium.org> wrote: > >> On 2012/05/22 21:31:14, darin wrote: >> >>> Are you suggesting making the method be static on HttpResponseHeaders so >>> it >>> can be called without an instance of HttpResponseHeaders? I thought you >>> were suggesting that we allocate a HttpResponseHeaders in order to call >>> methods on HttpResponseHeaders. >>> >> >> No static member: >> >> static bool HttpUtil::HasStrongValidators(**HttpVersion version, >> std::string >> etag...) { >> ... >> std::string raw_headers = AssemblerawHeaders(...) >> HttpResponseHeaders headers(raw_headers); >> return headers->HasStrongValidators()**; >> } >> >> So yes, we allocate an HttpResponseHeaders but that's part of the >> implementation. >> >> I guess it's a minor point and judging by this thread, probably not worth >> it. >> > > Oh, I see... when I created HttpUtil, it was just tools used by > HttpResponseHeaders. > That's why I was thinking it made sense to extract bits of > HttpResponseHeaders out > into HttpUtil. (The name HttpUtil has always sucked, sorry!!) > And for completeness, I created HttpUtil so I could unit test those bits. -Darin > > -Darin > > > >> >> >> -Darin >>> >> >> On Tue, May 22, 2012 at 1:15 PM, <mailto:rvargas@chromium.org> wrote: >>> >> >> > Just so that it's clear, I'm not suggesting exposing the reconstructed >>> > HttpResponseHeaders to anyone, nor changing the code at the >>> > BufferedReaourceLoader (that deals with WebURLResponse). >>> > >>> > Just having the HttpUtil method (with the proposed signature) call >>> > HttpResponseHeaders, where I believe the HasStrongValidators code >>> belongs. >>> > >>> > (I tend to see HttpUtil code either as useful general-purpose code, or >>> > code that >>> > we don't know where to put, and this is neither). >>> > >>> > >>> > On 2012/05/22 18:09:26, darin wrote: >>> > >>> >> WebURLResponse keeps a hash map of header / value pairs. That is a >>> lossy >>> >> representation of response headers! Converting back to >>> >> HttpResponseHeaders >>> >> can be done, but it may lose some information. Not sure if that >>> matters >>> >> here. Probably not for analysis of cache headers, but I'd be worried >>> >> about >>> >> someone using it for something else. >>> >> >>> > >>> > -Darin >>> >> >>> > >>> > >>> > On Tue, May 22, 2012 at 9:43 AM, <mailto:rvargas@chromium.org> wrote: >>> >> >>> > >>> > > A better approach seems to be to go the other way around: leave the >>> >> actual >>> >> > implementation of HasStrongValidators unchanged, and instead add >>> >> something >>> >> > to >>> >> > http_util that builds the response headers and calls the proper >>> method. >>> >> > >>> >> > >>> >> >>> > >>> > http://codereview.chromium.******org/10387200/%253Chttp://**coderev** >>> > iew.chromium.org/10387200/ <http://codereview.chromium.**org/10387200/<http://codereview.chromium.org/103... >>> >> >>> > >>> >> > >>> >> >>> > >>> > >>> > >>> >> >> http://codereview.chromium.****org/10387200/%3Chttp://coderev** >> iew.chromium.org/10387200/ <http://codereview.chromium.org/10387200/>> >> >>> > >>> >> >> >> http://codereview.chromium.**org/10387200/<http://codereview.chromium.org/103... >> > >
Alrighty, extracted the method and added tests in PS#4. Can I get an amen?
http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... File net/http/http_response_headers.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... net/http/http_response_headers.cc:1166: EnumerateHeader(NULL, "Last-Modified", &last_modified_header); FYI I notice a lot of lower-case headers in this file http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... File webkit/media/buffered_resource_loader.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... webkit/media/buffered_resource_loader.cc:380: "Media.UncacheableReason", reasons[i], kMaxReason); one nit: we would increment counts every time we recreated a connection for a single media source as opposed to the element (think about an ogg file doing a binary search when seeking! that's like ~8 HTTP connections!) I'd prefer to drop this code into the initial connection callback code inside BDS but I suspect that may have to wait until we break down BRL into two pieces (a piece that handles sending request + receiving response, and piece that handles the sliding window reading + buffering) http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc File webkit/media/cache_util.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:44: response.httpHeaderField("Last-Modified").utf8(), minor ocd nit again: I wonder if we should be consistent w/ casing http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util_un... File webkit/media/cache_util_unittest.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util_un... webkit/media/cache_util_unittest.cc:21: struct GTFUTestCase { does GTFU mean "Get Treasons For Uncacheability"?
http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... File net/http/http_response_headers.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... net/http/http_response_headers.cc:1166: EnumerateHeader(NULL, "Last-Modified", &last_modified_header); On 2012/05/23 01:16:03, scherkus wrote: > FYI I notice a lot of lower-case headers in this file Elected to stay consistent with existing capitalization in this file. http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... File webkit/media/buffered_resource_loader.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... webkit/media/buffered_resource_loader.cc:380: "Media.UncacheableReason", reasons[i], kMaxReason); On 2012/05/23 01:16:03, scherkus wrote: > one nit: we would increment counts every time we recreated a connection for a > single media source as opposed to the element (think about an ogg file doing a > binary search when seeking! that's like ~8 HTTP connections!) > > I'd prefer to drop this code into the initial connection callback code inside > BDS but I suspect that may have to wait until we break down BRL into two pieces > (a piece that handles sending request + receiving response, and piece that > handles the sliding window reading + buffering) But the whole idea is to classify reusability of individual responses! http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc File webkit/media/cache_util.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:44: response.httpHeaderField("Last-Modified").utf8(), On 2012/05/23 01:16:03, scherkus wrote: > minor ocd nit again: I wonder if we should be consistent w/ casing rfc2616 4.2: header names are case-insensitive (which the code accounts for). I went for consistency w/ http_response_headers.cc. http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util_un... File webkit/media/cache_util_unittest.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util_un... webkit/media/cache_util_unittest.cc:21: struct GTFUTestCase { On 2012/05/23 01:16:03, scherkus wrote: > does GTFU mean "Get Treasons For Uncacheability"? s/Treasons/Reasons/ Yes.
lgtm for my bits darin/rvargas? http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... File net/http/http_response_headers.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... net/http/http_response_headers.cc:1166: EnumerateHeader(NULL, "Last-Modified", &last_modified_header); On 2012/05/23 01:27:28, Ami Fischman wrote: > On 2012/05/23 01:16:03, scherkus wrote: > > FYI I notice a lot of lower-case headers in this file > > Elected to stay consistent with existing capitalization in this file. Yeah nevermind -- the stuff at the top of this file is all lowercase but the stuff nearer the bottom is capitalized Oh well. http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... File webkit/media/buffered_resource_loader.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... webkit/media/buffered_resource_loader.cc:380: "Media.UncacheableReason", reasons[i], kMaxReason); On 2012/05/23 01:27:28, Ami Fischman wrote: > On 2012/05/23 01:16:03, scherkus wrote: > > one nit: we would increment counts every time we recreated a connection for a > > single media source as opposed to the element (think about an ogg file doing a > > binary search when seeking! that's like ~8 HTTP connections!) > > > > I'd prefer to drop this code into the initial connection callback code inside > > BDS but I suspect that may have to wait until we break down BRL into two > pieces > > (a piece that handles sending request + receiving response, and piece that > > handles the sliding window reading + buffering) > > > But the whole idea is to classify reusability of individual responses! I suppose so but how could we answer that out of N people who set the src to "http://foo.com/bar.wav" they got something cacheable? Can we solve this with more histograms!? (first response vs all responses!?)
http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... File webkit/media/buffered_resource_loader.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... webkit/media/buffered_resource_loader.cc:380: "Media.UncacheableReason", reasons[i], kMaxReason); On 2012/05/23 01:34:19, scherkus wrote: > I suppose so but how could we answer that out of N people who set the src to > "http://foo.com/bar.wav" they got something cacheable? UMA is by its nature not per-site (aggregated along different dimension). > Can we solve this with more histograms!? (first response vs all responses!?) Could do. What I'm actually after is characterizing the useful-vs-unuseful as a first cut (l.377). l.380 is more about digging into the unuseful, if that turns out to be a sizeable population. I suspect we'll want to refine if that's the case, but this seems like a useful first cut to me.
http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... File webkit/media/buffered_resource_loader.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/buffered_reso... webkit/media/buffered_resource_loader.cc:380: "Media.UncacheableReason", reasons[i], kMaxReason); On 2012/05/23 01:38:25, Ami Fischman wrote: > On 2012/05/23 01:34:19, scherkus wrote: > > I suppose so but how could we answer that out of N people who set the src to > > "http://foo.com/bar.wav" they got something cacheable? > > UMA is by its nature not per-site (aggregated along different dimension). I'm not interested in the domain but setting the src attribute has a 1:1 mapping to creating a media engine + loading a URL, which makes for easier to comprehend numbers (at least for me!) > > Can we solve this with more histograms!? (first response vs all responses!?) > > Could do. > > What I'm actually after is characterizing the useful-vs-unuseful as a first cut > (l.377). l.380 is more about digging into the unuseful, if that turns out to be > a sizeable population. I suspect we'll want to refine if that's the case, but > this seems like a useful first cut to me. Agreed. I'm more interested in collecting some data over no data and refining later along w/ subsequent code shuffling, but taking this opportunity to mini-brainstorm about said possible refinements!
lgtm http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... File net/http/http_response_headers.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... net/http/http_response_headers.cc:1170: GetHttpVersion(), etag_header, last_modified_header, date_header); nit: this call should start enumerating arguments on the previous line. http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.cc File net/http/http_util.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.cc#newc... net/http/http_util.cc:704: HttpVersion version, nit: arguments should start on the previous line. http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.h File net/http/http_util.h (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.h#newco... net/http/http_util.h:191: // Returns true if this response has a strong etag or last-modified header. nit: this response? :) http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc File webkit/media/cache_util.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:25: enum { kHttpOK = 200, kHttpPartialContent = 206 }; nit: enums are supposed to be upper case. http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:41: if (code == kHttpPartialContent && !net::HttpUtil::HasStrongValidators( nit: could you break this at the && http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:65: cache_control_header.begin(), nit: prev line http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:71: cache_control_header.begin() + kMaxAgePrefixLen, nit:prev line http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:82: response.httpHeaderField("Expires").utf8().data(), &expires) && nit: I'd also move the first arg to the previousline http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util_un... File webkit/media/cache_util_unittest.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util_un... webkit/media/cache_util_unittest.cc:21: struct GTFUTestCase { ?
Darin: what say? http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... File net/http/http_response_headers.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... net/http/http_response_headers.cc:1170: GetHttpVersion(), etag_header, last_modified_header, date_header); On 2012/05/23 02:04:12, rvargas wrote: > nit: this call should start enumerating arguments on the previous line. That'd require ASCII art and more vertical space than this way (see comment elsewhere). http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.cc File net/http/http_util.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.cc#newc... net/http/http_util.cc:704: HttpVersion version, On 2012/05/23 02:04:12, rvargas wrote: > nit: arguments should start on the previous line. No, because that makes the second & subsequent args have to line up to as well. (google style guide says either FooBlah(blort, otherblort, ...); or FooBlah( blort, otherblort, ...); but does not mix: FooBlah(blort, otherblort, ...); with the specific bullet of: - All parameters should be aligned if possible. http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.h File net/http/http_util.h (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.h#newco... net/http/http_util.h:191: // Returns true if this response has a strong etag or last-modified header. On 2012/05/23 02:04:12, rvargas wrote: > nit: this response? :) Done. http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc File webkit/media/cache_util.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:25: enum { kHttpOK = 200, kHttpPartialContent = 206 }; On 2012/05/23 02:04:12, rvargas wrote: > nit: enums are supposed to be upper case. Yeah... This is a place where chromium's style guide conflicts w/ google style guide (which points out that MACRO_STYLE is likely to conflict with macros). media/ code goes w/ the kStyle in >90% of locations so I'm inclined to apply the consistency defense here. http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:41: if (code == kHttpPartialContent && !net::HttpUtil::HasStrongValidators( On 2012/05/23 02:04:12, rvargas wrote: > nit: could you break this at the && Done, though I don't think it helps... http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:65: cache_control_header.begin(), On 2012/05/23 02:04:12, rvargas wrote: > nit: prev line Nope :) http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:71: cache_control_header.begin() + kMaxAgePrefixLen, On 2012/05/23 02:04:12, rvargas wrote: > nit:prev line Ditto. http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util.cc... webkit/media/cache_util.cc:82: response.httpHeaderField("Expires").utf8().data(), &expires) && On 2012/05/23 02:04:12, rvargas wrote: > nit: I'd also move the first arg to the previousline Ditto. http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util_un... File webkit/media/cache_util_unittest.cc (right): http://codereview.chromium.org/10387200/diff/11002/webkit/media/cache_util_un... webkit/media/cache_util_unittest.cc:21: struct GTFUTestCase { On 2012/05/23 02:04:12, rvargas wrote: > ? Oops, renamed & doco'd.
http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... File net/http/http_response_headers.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_response_hea... net/http/http_response_headers.cc:1170: GetHttpVersion(), etag_header, last_modified_header, date_header); On 2012/05/23 03:55:23, Ami Fischman wrote: > On 2012/05/23 02:04:12, rvargas wrote: > > nit: this call should start enumerating arguments on the previous line. > > That'd require ASCII art and more vertical space than this way (see comment > elsewhere). Just look at the Google style guide. http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.cc File net/http/http_util.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.cc#newc... net/http/http_util.cc:704: HttpVersion version, (warning, rietveld may ruin formatting) bool HttpUtil::HasStrongValidators(HttpVersion version, const std::string& etag_header, const std::string& last_modified_header, const std::string& date_header) { This is not ascii art, and it follows the style guide to the letter. Like the rest of this file. (just see line 693) (the same thing applies everywhere that I added nits)
http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.cc File net/http/http_util.cc (right): http://codereview.chromium.org/10387200/diff/11002/net/http/http_util.cc#newc... net/http/http_util.cc:704: HttpVersion version, On 2012/05/23 18:26:09, rvargas wrote: > (warning, rietveld may ruin formatting) > > bool HttpUtil::HasStrongValidators(HttpVersion version, > const std::string& etag_header, > const std::string& last_modified_header, > const std::string& date_header) { > > This is not ascii art, and it follows the style guide to the letter. Like the > rest of this file. (just see line 693) > > (the same thing applies everywhere that I added nits) Ah; I thought you were asking for mixed style, because at least one of these call-sites would exceed 80 cols when indenting-to-open-paren. But looks like that's not what you were asking for, and none of the instances in question violate 80-cols (anymore; renaming between working versions must have got rid of that). Done.
OK, LGTM... one suggestion: http://codereview.chromium.org/10387200/diff/7003/webkit/media/cache_util_uni... File webkit/media/cache_util_unittest.cc (right): http://codereview.chromium.org/10387200/diff/7003/webkit/media/cache_util_uni... webkit/media/cache_util_unittest.cc:83: std::vector<UncacheableReason> reasons = just an observation, but if you instead defined the reasons as bit-masks, then you wouldn't have to hard-code string literals above that match the numeric values of the enums. you could instead write expressions like kNoCache | kNoStore, which would be a lot more readable!
Thanks for the reviews, all! http://codereview.chromium.org/10387200/diff/7003/webkit/media/cache_util_uni... File webkit/media/cache_util_unittest.cc (right): http://codereview.chromium.org/10387200/diff/7003/webkit/media/cache_util_uni... webkit/media/cache_util_unittest.cc:83: std::vector<UncacheableReason> reasons = On 2012/05/23 20:35:01, darin wrote: > just an observation, but if you instead defined the reasons as bit-masks, then > you wouldn't have to hard-code string literals above that match the numeric > values of the enums. you could instead write expressions like kNoCache | > kNoStore, which would be a lot more readable! True, but the reason I didn't do that is because that makes UMA-reporting more expensive. With 8 reasons now, that requires 256 buckets, doubling whenever a reason is added. Of course that also makes UMA reporting more informative (combinations are counted instead of individual reasons) but also more difficult to use (individual reasons have to be summed out of the combinations). In summary: I thought about it, but decided not to :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fischman@chromium.org/10387200/7003
On Wed, May 23, 2012 at 2:36 PM, <fischman@chromium.org> wrote: > Thanks for the reviews, all! > > > > http://codereview.chromium.**org/10387200/diff/7003/webkit/** > media/cache_util_unittest.cc<http://codereview.chromium.org/10387200/diff/7003/webkit/media/cache_util_unittest.cc> > File webkit/media/cache_util_**unittest.cc (right): > > http://codereview.chromium.**org/10387200/diff/7003/webkit/** > media/cache_util_unittest.cc#**newcode83<http://codereview.chromium.org/10387200/diff/7003/webkit/media/cache_util_unittest.cc#newcode83> > webkit/media/cache_util_**unittest.cc:83: std::vector<UncacheableReason> > reasons = > On 2012/05/23 20:35:01, darin wrote: > >> just an observation, but if you instead defined the reasons as >> > bit-masks, then > >> you wouldn't have to hard-code string literals above that match the >> > numeric > >> values of the enums. you could instead write expressions like >> > kNoCache | > >> kNoStore, which would be a lot more readable! >> > > True, but the reason I didn't do that is because that makes > UMA-reporting more expensive. With 8 reasons now, that requires 256 > buckets, doubling whenever a reason is added. > Of course that also makes UMA reporting more informative (combinations > are counted instead of individual reasons) but also more difficult to > use (individual reasons have to be summed out of the combinations). > In summary: I thought about it, but decided not to :) > You could implement exactly the same UMA histograms. I don't see why a bit-field is any different from an array in this case. -Darin > > http://codereview.chromium.**org/10387200/<http://codereview.chromium.org/103... >
Try job failure for 10387200-7003 (retry) on linux_rel for step "net_unittests". It's a second try, previously, step "net_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
> > You could implement exactly the same UMA histograms. > D'oh! I mapped your suggestion to my previous thought process without thinking it through. Indeed, once the reasons are bitmasks I can recreate the histograms I want by Log2'ing them, while retaining the readability benefit. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fischman@chromium.org/10387200/29001
https://chromiumcodereview.appspot.com/10387200/diff/29001/webkit/media/cache... File webkit/media/cache_util.cc (right): https://chromiumcodereview.appspot.com/10387200/diff/29001/webkit/media/cache... webkit/media/cache_util.cc:27: std::vector<UncacheableReason> GetReasonsForUncacheability( the next logical step is to return an integer that is just the union of the flags. https://chromiumcodereview.appspot.com/10387200/diff/29001/webkit/media/cache... webkit/media/cache_util.cc:38: reasons.push_back(kNoData); reasons |= kNoData; etc.
https://chromiumcodereview.appspot.com/10387200/diff/29001/webkit/media/cache... File webkit/media/cache_util.cc (right): https://chromiumcodereview.appspot.com/10387200/diff/29001/webkit/media/cache... webkit/media/cache_util.cc:27: std::vector<UncacheableReason> GetReasonsForUncacheability( On 2012/05/23 23:38:43, darin wrote: > the next logical step is to return an integer that is just the union of the > flags. I think that's a worse API, though. Don't you?
On Wed, May 23, 2012 at 4:44 PM, <fischman@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10387200/diff/** > 29001/webkit/media/cache_util.**cc<https://chromiumcodereview.appspot.com/10387200/diff/29001/webkit/media/cache_util.cc> > File webkit/media/cache_util.cc (right): > > https://chromiumcodereview.**appspot.com/10387200/diff/** > 29001/webkit/media/cache_util.**cc#newcode27<https://chromiumcodereview.appspot.com/10387200/diff/29001/webkit/media/cache_util.cc#newcode27> > webkit/media/cache_util.cc:27: std::vector<UncacheableReason> > GetReasonsForUncacheability( > On 2012/05/23 23:38:43, darin wrote: > >> the next logical step is to return an integer that is just the union >> > of the > >> flags. >> > > I think that's a worse API, though. Don't you? > Hmm... I guess enumerating over the bits is not as nice if that's what you mean. Certainly checking for empty is easy / clean and checking for specific reasons is easy / clean. Anyways, you don't need to change the code. I would probably do it to reduce codesighs :-/ -Darin > > https://chromiumcodereview.**appspot.com/10387200/<https://chromiumcodereview... >
Try job failure for 10387200-29001 (retry) on win_rel for step "content_unittests". It's a second try, previously, step "content_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fischman@chromium.org/10387200/38001
On 2012/05/23 23:56:13, darin wrote: > Hmm... I guess enumerating over the bits is not as nice if that's what you > mean. Yes, it was. > Certainly checking for empty is easy / clean and checking for > specific reasons is easy / clean. Anyways, you don't need to change the > code. I would probably do it to reduce codesighs :-/ Well, the CQ's mysterious failure is your gain. Done.
LGTM
Try job failure for 10387200-38001 (retry) on win_rel for steps "browser_tests, content_unittests" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fischman@chromium.org/10387200/38001
Try job failure for 10387200-38001 (retry) on win_rel for steps "browser_tests, content_unittests". It's a second try, previously, steps "browser_tests, content_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fischman@chromium.org/10387200/45002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fischman@chromium.org/10387200/40009
Change committed as 138972 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
