Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
784853004:
[Cronet] Implement methods to get response headers in HttpURLConnection. (Closed)
Description[Cronet] Implement methods to get response headers in HttpURLConnection.
This CL implements the following four methods:
- getHeaderFields()
- getHeaderField(String key)
- getHeaderField(int pos)
- getHeaderFieldKey(int pos)
BUG=398997
Committed: https://crrev.com/7a6ee3ff5cdc8d10f6d191c24510b6a20a004322
Cr-Commit-Position: refs/heads/master@{#314011}
Patch Set 1 : #Patch Set 2 : Self review #
Total comments: 9
Patch Set 3 : Remove SuppressFBWarnings #Patch Set 4 : Removed null from comparator and added a test case #Patch Set 5 : Just use String's comparator #Patch Set 6 : Rebased #
Total comments: 7
Patch Set 7 : Return empty map on exception #
Total comments: 4
Patch Set 8 : Cache unmodifable header list #
Total comments: 6
Patch Set 9 : Misha's comments #Patch Set 10 : Rebased #
Messages
Total messages: 28 (7 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
xunjieli@chromium.org changed reviewers: + mef@chromium.org, miloslav@google.com, mmenke@chromium.org
https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:73: @SuppressFBWarnings("ES_STRING_EQUALS") Rather than suppress the warning, maybe just: if (a == null) { if (b == null) { return 0; } else { return -1; } } if (b == null) ... It's a little uglier, but I think it's better than suppressing a compiler warning. Alternatively, may be a little prettier to first if with "if (a == null && b == null)" https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:77: } else if (a == null) { Actually...Can we even have NULL response header strings? Why do we even need this? If it can happen, we should have tests for it. https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:183: new TreeMap<String, List<String>>(FIELD_NAME_COMPARATOR); Hrm...The comparison is not "consistent with equals", per docs, but looks like, at least on Android, we have guaranteed reasonable behavior in that case. http://developer.android.com/reference/java/util/TreeMap.html https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:144: * Returns the name of the header field at the given position pos or null nit: Suggest a comma after pos.
https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:73: @SuppressFBWarnings("ES_STRING_EQUALS") On 2015/01/16 19:43:13, mmenke wrote: > Rather than suppress the warning, maybe just: > > if (a == null) { > if (b == null) { > return 0; > } else { > return -1; > } > } > if (b == null) ... > > It's a little uglier, but I think it's better than suppressing a compiler > warning. > > Alternatively, may be a little prettier to first if with "if (a == null && b == > null)" Done. That's solved my problem. Didn't see it earlier. Thank you! https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:77: } else if (a == null) { On 2015/01/16 19:43:13, mmenke wrote: > Actually...Can we even have NULL response header strings? Why do we even need > this? > > If it can happen, we should have tests for it. The default implementation has a null key which corresponds to the http status line. We have not added it. But I think we should keep it in case we add null key in the future? https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:183: new TreeMap<String, List<String>>(FIELD_NAME_COMPARATOR); On 2015/01/16 19:43:13, mmenke wrote: > Hrm...The comparison is not "consistent with equals", per docs, but looks like, > at least on Android, we have guaranteed reasonable behavior in that case. > > http://developer.android.com/reference/java/util/TreeMap.html Acknowledged. https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:144: * Returns the name of the header field at the given position pos or null On 2015/01/16 19:43:13, mmenke wrote: > nit: Suggest a comma after pos. Done.
https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:77: } else if (a == null) { On 2015/01/16 20:01:12, xunjieli wrote: > On 2015/01/16 19:43:13, mmenke wrote: > > Actually...Can we even have NULL response header strings? Why do we even need > > this? > > > > If it can happen, we should have tests for it. > > The default implementation has a null key which corresponds to the http status > line. We have not added it. But I think we should keep it in case we add null > key in the future? My opinion on this (And others may disagree, though they're wrong, of course) is that the NULL key is a hack, and we should instead have an API to expose the response line. If we want to support that behavior in the HttpUrlConnection wrapper (Which I think makes sense), then we should actually create a new map with the extra entry added.
Patchset #4 (id:140001) has been deleted
On 2015/01/16 20:03:59, mmenke wrote: > https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... > File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java > (right): > > https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:77: } > else if (a == null) { > On 2015/01/16 20:01:12, xunjieli wrote: > > On 2015/01/16 19:43:13, mmenke wrote: > > > Actually...Can we even have NULL response header strings? Why do we even > need > > > this? > > > > > > If it can happen, we should have tests for it. > > > > The default implementation has a null key which corresponds to the http status > > line. We have not added it. But I think we should keep it in case we add null > > key in the future? > > My opinion on this (And others may disagree, though they're wrong, of course) is > that the NULL key is a hack, and we should instead have an API to expose the > response line. > > If we want to support that behavior in the HttpUrlConnection wrapper (Which I > think makes sense), then we should actually create a new map with the extra > entry added. I think your suggestion totally makes sense. I have removed the null case and added a test. Thanks!
On 2015/01/16 20:23:02, xunjieli wrote: > On 2015/01/16 20:03:59, mmenke wrote: > > > https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... > > File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java > > (right): > > > > > https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... > > components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:77: > } > > else if (a == null) { > > On 2015/01/16 20:01:12, xunjieli wrote: > > > On 2015/01/16 19:43:13, mmenke wrote: > > > > Actually...Can we even have NULL response header strings? Why do we even > > need > > > > this? > > > > > > > > If it can happen, we should have tests for it. > > > > > > The default implementation has a null key which corresponds to the http > status > > > line. We have not added it. But I think we should keep it in case we add > null > > > key in the future? > > > > My opinion on this (And others may disagree, though they're wrong, of course) > is > > that the NULL key is a hack, and we should instead have an API to expose the > > response line. > > > > If we want to support that behavior in the HttpUrlConnection wrapper (Which I > > think makes sense), then we should actually create a new map with the extra > > entry added. > > I think your suggestion totally makes sense. I have removed the null case and > added a test. Thanks! Think we could get away with just using String.CASE_INSENSITIVE_ORDER and without the new test, but LGTM, either way.
On 2015/01/16 20:24:37, mmenke wrote: > On 2015/01/16 20:23:02, xunjieli wrote: > > On 2015/01/16 20:03:59, mmenke wrote: > > > > > > https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... > > > File > components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java > > > (right): > > > > > > > > > https://codereview.chromium.org/784853004/diff/100001/components/cronet/andro... > > > > components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:77: > > } > > > else if (a == null) { > > > On 2015/01/16 20:01:12, xunjieli wrote: > > > > On 2015/01/16 19:43:13, mmenke wrote: > > > > > Actually...Can we even have NULL response header strings? Why do we > even > > > need > > > > > this? > > > > > > > > > > If it can happen, we should have tests for it. > > > > > > > > The default implementation has a null key which corresponds to the http > > status > > > > line. We have not added it. But I think we should keep it in case we add > > null > > > > key in the future? > > > > > > My opinion on this (And others may disagree, though they're wrong, of > course) > > is > > > that the NULL key is a hack, and we should instead have an API to expose the > > > response line. > > > > > > If we want to support that behavior in the HttpUrlConnection wrapper (Which > I > > > think makes sense), then we should actually create a new map with the extra > > > entry added. > > > > I think your suggestion totally makes sense. I have removed the null case and > > added a test. Thanks! > > Think we could get away with just using String.CASE_INSENSITIVE_ORDER and > without the new test, but LGTM, either way. You are totally right. Less code is better. I've gotten rid of it. Thanks!
LGTM https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:166: if (map.containsKey(entry.first)) { There is no null key containing the status line any more, right? https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:126: return null; Do we really want to return null? That doesn't seem right. Is there a comment about why they are doing that in L? https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:618: } catch (Exception e) { Maybe the more specific UnsupportedOperationException is more appropriate (so that it's not e.g. NPE). Also below.
Thanks for the review! https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:166: if (map.containsKey(entry.first)) { On 2015/01/20 15:25:10, miloslav wrote: > There is no null key containing the status line any more, right? Yes, you are right. There is no null key support. https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:126: return null; On 2015/01/20 15:25:10, miloslav wrote: > Do we really want to return null? That doesn't seem right. Is there a comment > about why they are doing that in L? Sorry about the confusion. I was mistaken. On k it returns null, but returns empty map on L. I have changed the code to match the newer implementation. I also updated the test. Thanks so much for spotting this! https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:618: } catch (Exception e) { On 2015/01/20 15:25:10, miloslav wrote: > Maybe the more specific UnsupportedOperationException is more appropriate (so > that it's not e.g. NPE). > > Also below. Done.
https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:141: Map<String, List<String>> map = mResponseInfo.getAllHeaders(); Getting all headers on every call to getHeaderField() seems really inefficient as it involves a lengthy map buildup. Would it make sense to either cache headers map in CronetHttpURLConnection or in ResponseInfo? Alternatively, would it make sense to getAllHeadersAsList() and just loop here looking for first fieldName? This would at least avoid all the copies / allocations and shorten the loop in comparison with getAllHeaders.
https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:141: Map<String, List<String>> map = mResponseInfo.getAllHeaders(); On 2015/01/23 17:08:02, mef wrote: > Getting all headers on every call to getHeaderField() seems really inefficient > as it involves a lengthy map buildup. > Would it make sense to either cache headers map in CronetHttpURLConnection or in > ResponseInfo? > > Alternatively, would it make sense to getAllHeadersAsList() and just loop here > looking for first fieldName? This would at least avoid all the copies / > allocations and shorten the loop in comparison with getAllHeaders. +1 for caching getAllHeaders in ResponseInfo. Also the unmodifiable collection returned by NativeResponseInfo.getAllHeadersAsList should be cached too. Are ResponseInfo instances supposed to be modified after construction?
https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:141: Map<String, List<String>> map = mResponseInfo.getAllHeaders(); On 2015/01/23 19:17:33, miloslav wrote: > On 2015/01/23 17:08:02, mef wrote: > > Getting all headers on every call to getHeaderField() seems really inefficient > > as it involves a lengthy map buildup. > > Would it make sense to either cache headers map in CronetHttpURLConnection or > in > > ResponseInfo? > > > > Alternatively, would it make sense to getAllHeadersAsList() and just loop here > > looking for first fieldName? This would at least avoid all the copies / > > allocations and shorten the loop in comparison with getAllHeaders. > > +1 for caching getAllHeaders in ResponseInfo. Also the unmodifiable collection > returned by NativeResponseInfo.getAllHeadersAsList should be cached too. > > Are ResponseInfo instances supposed to be modified after construction? I don't think ResponseInfo instances are modified after construction. I cached the headers in NativeResponseInfo, as ResponseInfo is an interface. Should we cache them in ResponseInfo? that will involve making ResponseInfo as an abstract class. WDYT? PTAL.
LGTM https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:141: Map<String, List<String>> map = mResponseInfo.getAllHeaders(); On 2015/01/23 20:06:41, xunjieli wrote: > On 2015/01/23 19:17:33, miloslav wrote: > > On 2015/01/23 17:08:02, mef wrote: > > > Getting all headers on every call to getHeaderField() seems really > inefficient > > > as it involves a lengthy map buildup. > > > Would it make sense to either cache headers map in CronetHttpURLConnection > or > > in > > > ResponseInfo? > > > > > > Alternatively, would it make sense to getAllHeadersAsList() and just loop > here > > > looking for first fieldName? This would at least avoid all the copies / > > > allocations and shorten the loop in comparison with getAllHeaders. > > > > +1 for caching getAllHeaders in ResponseInfo. Also the unmodifiable collection > > returned by NativeResponseInfo.getAllHeadersAsList should be cached too. > > > > Are ResponseInfo instances supposed to be modified after construction? > > I don't think ResponseInfo instances are modified after construction. > I cached the headers in NativeResponseInfo, as ResponseInfo is an interface. > Should we cache them in ResponseInfo? that will involve making ResponseInfo as > an abstract class. WDYT? PTAL. Sorry, by ResponseInfo instances I was referring to its implementations, which currently is only NativeResponseInfo, so caching there is fine. For a moment I got confused that NativeResponseInfo.getAllHeaders does not cache the result, so the code here should be fine. I asked whether NativeInfo is to be modified after construction, because if it doesn't then making all fields final and computing the map at construction could be easier (and as a bonus thread safe). However, since it is fine now, it is not worth it changing it.
On 2015/01/23 20:21:55, miloslav wrote: > LGTM > > https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java > (right): > > https://codereview.chromium.org/784853004/diff/200001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:141: > Map<String, List<String>> map = mResponseInfo.getAllHeaders(); > On 2015/01/23 20:06:41, xunjieli wrote: > > On 2015/01/23 19:17:33, miloslav wrote: > > > On 2015/01/23 17:08:02, mef wrote: > > > > Getting all headers on every call to getHeaderField() seems really > > inefficient > > > > as it involves a lengthy map buildup. > > > > Would it make sense to either cache headers map in CronetHttpURLConnection > > or > > > in > > > > ResponseInfo? > > > > > > > > Alternatively, would it make sense to getAllHeadersAsList() and just loop > > here > > > > looking for first fieldName? This would at least avoid all the copies / > > > > allocations and shorten the loop in comparison with getAllHeaders. > > > > > > +1 for caching getAllHeaders in ResponseInfo. Also the unmodifiable > collection > > > returned by NativeResponseInfo.getAllHeadersAsList should be cached too. > > > > > > Are ResponseInfo instances supposed to be modified after construction? > > > > I don't think ResponseInfo instances are modified after construction. > > I cached the headers in NativeResponseInfo, as ResponseInfo is an interface. > > Should we cache them in ResponseInfo? that will involve making ResponseInfo as > > an abstract class. WDYT? PTAL. > > Sorry, by ResponseInfo instances I was referring to its implementations, which > currently is only NativeResponseInfo, so caching there is fine. > > For a moment I got confused that NativeResponseInfo.getAllHeaders does not cache > the result, so the code here should be fine. > > I asked whether NativeInfo is to be modified after construction, because if it > doesn't then making all fields final and computing the map at construction could > be easier (and as a bonus thread safe). However, since it is fine now, it is not > worth it changing it. Ah, I see! Makes sense. Thanks for the follow-up! I think I will leave the map as non-final for now. Misha, WDYT? PTAL. Thank you!
Pinging Misha for review ...
https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/784853004/diff/180001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:166: if (map.containsKey(entry.first)) { On 2015/01/20 16:12:21, xunjieli wrote: > On 2015/01/20 15:25:10, miloslav wrote: > > There is no null key containing the status line any more, right? > > Yes, you are right. There is no null key support. I think that's a right thing for CronetUrlRequest, but is it right thing for CronetHttpURLConnection? https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:159: return mUnmodifiableAllHeaders; Do I understand correctly that the reason to return unmodifiableList is to prevent embedder from inadvertently changing it? https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:135: public final String getHeaderField(String fieldName) { Is implementation of getHeaderField enough to support getHeaderField{Date|Int|Long} or do we need to implement those explicitly? https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:174: try { nit: maybe factor out most of getHeaderField{Key} into something like getHeaderFieldPair?
Thanks for the review! https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:159: return mUnmodifiableAllHeaders; On 2015/01/30 16:22:51, mef wrote: > Do I understand correctly that the reason to return unmodifiableList is to > prevent embedder from inadvertently changing it? Yes, this seems to be an requirement for HttpURLConnection methods, so I thought it'd be a good practice for the underlying ResponseInfo as well. https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:135: public final String getHeaderField(String fieldName) { On 2015/01/30 16:22:52, mef wrote: > Is implementation of getHeaderField enough to support > getHeaderField{Date|Int|Long} or do we need to implement those explicitly? We can rely on the default implementations which reuse getHeaderField/getHeaderFieldKey. OkHttp does the same. https://codereview.chromium.org/784853004/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:174: try { On 2015/01/30 16:22:52, mef wrote: > nit: maybe factor out most of getHeaderField{Key} into something like > getHeaderFieldPair? Done.
lgtm
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/784853004/260001
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/7a6ee3ff5cdc8d10f6d191c24510b6a20a004322 Cr-Commit-Position: refs/heads/master@{#314011} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
