Enable Sdch in Cronet

Enable sdch in Cronet

This CL combined rdsmith@'s CL(747453004) and mef@'s
CL(1073193002), and added tests to make sure sdch works for
both the old and the new API.

BUG=414885
Committed: https://crrev.com/b8a6d56ffe39823af7b09ce6b816d3cd9fbac2a6
Cr-Commit-Position: refs/heads/master@{#327517}

[xunjieli, 2015-04-14 17:52:41]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org, rdsmith@chromium.org

[xunjieli, 2015-04-14 17:58:50]

I added two basic tests to make sure Sdch is working in Cronet. In the future,
we can work on more complex tests. PTAL.

[alexraikman, 2015-04-14 18:11:32]

alexraikman@google.com changed reviewers:
+ alexraikman@google.com

[alexraikman, 2015-04-14 18:11:33]

https://codereview.chromium.org/1085903002/diff/1/components/cronet/android/t...
File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java
(right):

https://codereview.chromium.org/1085903002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:86:
// Make a request to fetch /sdch/test.
I am not sure how this check is useful. In theory, SDCH can be unused because
the dictionary is still being downloaded.

https://codereview.chromium.org/1085903002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:129:
// Make a request to fetch /sdch/test.
ditto

[xunjieli, 2015-04-14 18:21:51]

https://codereview.chromium.org/1085903002/diff/1/components/cronet/android/t...
File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java
(right):

https://codereview.chromium.org/1085903002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:86:
// Make a request to fetch /sdch/test.
On 2015/04/14 18:11:33, alexraikman wrote:
> I am not sure how this check is useful. In theory, SDCH can be unused because
> the dictionary is still being downloaded.

Yea. I just wanted to make sure that when sdch is not enabled, fetching
/sdch/test will not give the encoded response. I guess this check is not too
useful, I'll remove it if you find that having this check is confusing.

[alexraikman, 2015-04-14 18:35:34]

On 2015/04/14 18:21:51, xunjieli wrote:
>
https://codereview.chromium.org/1085903002/diff/1/components/cronet/android/t...
> File
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java
> (right):
> 
>
https://codereview.chromium.org/1085903002/diff/1/components/cronet/android/t...
>
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:86:
> // Make a request to fetch /sdch/test.
> On 2015/04/14 18:11:33, alexraikman wrote:
> > I am not sure how this check is useful. In theory, SDCH can be unused
because
> > the dictionary is still being downloaded.
> 
> Yea. I just wanted to make sure that when sdch is not enabled, fetching
> /sdch/test will not give the encoded response. I guess this check is not too
> useful, I'll remove it if you find that having this check is confusing.

It's not confusing, but rather misleading. If one were to remove Thread.sleep()
in positive tests above, the result would be no sdch as well. My advice: remove.

[xunjieli, 2015-04-14 18:39:47]

On 2015/04/14 18:35:34, alexraikman wrote:
> On 2015/04/14 18:21:51, xunjieli wrote:
> >
>
https://codereview.chromium.org/1085903002/diff/1/components/cronet/android/t...
> > File
> components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1085903002/diff/1/components/cronet/android/t...
> >
>
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:86:
> > // Make a request to fetch /sdch/test.
> > On 2015/04/14 18:11:33, alexraikman wrote:
> > > I am not sure how this check is useful. In theory, SDCH can be unused
> because
> > > the dictionary is still being downloaded.
> > 
> > Yea. I just wanted to make sure that when sdch is not enabled, fetching
> > /sdch/test will not give the encoded response. I guess this check is not too
> > useful, I'll remove it if you find that having this check is confusing.
> 
> It's not confusing, but rather misleading. If one were to remove
Thread.sleep()
> in positive tests above, the result would be no sdch as well. My advice:
remove.

Fair enough. Done. Thanks!

[Randy Smith (Not in Mondays), 2015-04-15 17:40:06]

rdsmith@chromium.org changed reviewers:
+ ellyjones@google.com
- rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2015-04-15 17:40:07]

I'm going to yield to my sense of
laziness/overwhelm/gotta-actually-get-some-80%-work-done and hand this off to
Elly, who's now driving SDCH.

[xunjieli, 2015-04-16 15:04:18]

On 2015/04/15 17:40:07, rdsmith wrote:
> I'm going to yield to my sense of
> laziness/overwhelm/gotta-actually-get-some-80%-work-done and hand this off to
> Elly, who's now driving SDCH.

Thanks, Elly! I would appreciate very much if you could take a look.
Our embedder would like to experiment with sdch and get some data soon, so it'd
be awesome if sdch could be enabled.

[mef, 2015-04-16 16:04:20]

Sorry, I think I forgot to mail my comments.

This is really cool!

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:166: new
net::SdchOwner(context_->sdch_manager(), context_.get()));
Add DCHECK(context_->sdch_manager())? 
I know, that it should exist, but wouldn't hurt to verify.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:94:
scoped_ptr<net::SdchOwner> sdch_owner_;
can we forward-declare it here, in header file? See line 28.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:104:
long getUrlRequestContextAdapter() {
maybe call it getUrlRequestContextAdapterForTesting()? I see it being used,
albeit inconsistently.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/test/native_test_server.cc (right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:120:
response->AddCustomHeader("Get-Dictionary", "/sdch/dict/LeQxM80O");
Is it required by the spec to add 'Get-Dictionary' to the response only if
client accepts sdch encoding?

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:122:
response->set_content("The quick brown fox jumps over the lazy dog.\n");
Could this be an explicit test data like 'sdch_plain.html' and
'sdch_plain.html.headers'?

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:128: if (it !=
request.headers.end()) {
Could this be just an explicit test data like 'sdch_encoded.html' and
'sdch_encoded.html.headers'?

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:167: void
RegisterHostResolverProcOnNetworkThreadLegacyAPI(
maybe add TODO to remove this when we remove legacy api?

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.h (right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.h:19: #include
"net/sdch/sdch_owner.h"
forward declare?

[Elly Fong-Jones, 2015-04-16 18:42:59]

ellyjones@chromium.org changed reviewers:
+ ellyjones@chromium.org

[Elly Fong-Jones, 2015-04-16 18:43:00]

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java
(right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:65:
Thread.sleep(5000);
it seems like this test is going to be inherently flaky until this TODO is done

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/test/native_test_server.cc (right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:48: class
LocalHostResolverProc : public net::HostResolverProc {
I think you want RuleBasedHostResolverProc here, then call
->AddRule(fake_sdch_domain, "127.0.0.1") and it does the rest of this for you.

[xunjieli, 2015-04-16 19:36:58]

Thanks for the review! PTAL.
(Btw Rietveld complained about upload in progress when I was diffing the
patches, although i was done uploading. Not sure why, hope it will go away).

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:166: new
net::SdchOwner(context_->sdch_manager(), context_.get()));
On 2015/04/16 16:04:19, mef wrote:
> Add DCHECK(context_->sdch_manager())? 
> I know, that it should exist, but wouldn't hurt to verify.

Done.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:94:
scoped_ptr<net::SdchOwner> sdch_owner_;
On 2015/04/16 16:04:19, mef wrote:
> can we forward-declare it here, in header file? See line 28.

Done.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:104:
long getUrlRequestContextAdapter() {
On 2015/04/16 16:04:19, mef wrote:
> maybe call it getUrlRequestContextAdapterForTesting()? I see it being used,
> albeit inconsistently.

Done.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java
(right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:65:
Thread.sleep(5000);
On 2015/04/16 18:43:00, Elly Jones wrote:
> it seems like this test is going to be inherently flaky until this TODO is
done

Unfortunately I can't come up with a better solution. But I think 5s delay is
quite long to make sure the test succeed.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/test/native_test_server.cc (right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:48: class
LocalHostResolverProc : public net::HostResolverProc {
On 2015/04/16 18:43:00, Elly Jones wrote:
> I think you want RuleBasedHostResolverProc here, then call
> ->AddRule(fake_sdch_domain, "127.0.0.1") and it does the rest of this for you.

Done. Thanks! That's convenient :)

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:120:
response->AddCustomHeader("Get-Dictionary", "/sdch/dict/LeQxM80O");
On 2015/04/16 16:04:20, mef wrote:
> Is it required by the spec to add 'Get-Dictionary' to the response only if
> client accepts sdch encoding?

Yes, I believe so :) It reads "A compatible server may instruct a compatible
user agent to download one or more new dictionaries by including the
Get-Dictionary header in the HTTP response."
(https://docs.google.com/a/chromium.org/document/d/1REMkwjXY5yFOkJwtJPjCMwZ4Sh...)

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:122:
response->set_content("The quick brown fox jumps over the lazy dog.\n");
On 2015/04/16 16:04:20, mef wrote:
> Could this be an explicit test data like 'sdch_plain.html' and
> 'sdch_plain.html.headers'?

Done. I named it index since it can be potentially used to multiple advertise
dictionaries.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:128: if (it !=
request.headers.end()) {
On 2015/04/16 16:04:20, mef wrote:
> Could this be just an explicit test data like 'sdch_encoded.html' and
> 'sdch_encoded.html.headers'?

Done.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/test/native_test_server.cc:167: void
RegisterHostResolverProcOnNetworkThreadLegacyAPI(
On 2015/04/16 16:04:20, mef wrote:
> maybe add TODO to remove this when we remove legacy api?

Done.

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.h (right):

https://codereview.chromium.org/1085903002/diff/40001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.h:19: #include
"net/sdch/sdch_owner.h"
On 2015/04/16 16:04:20, mef wrote:
> forward declare?

Done.

[mef, 2015-04-20 15:40:54]

looks pretty good. 

I think we need to add a test where server advertises non-existent dictionary,
to make sure that sdch manager doesn't crash and burn on that.

https://codereview.chromium.org/1085903002/diff/80001/components/cronet/andro...
File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java
(right):

https://codereview.chromium.org/1085903002/diff/80001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:65:
Thread.sleep(5000);
Suggest extracting this is into a specific function like
waitForDictionaryDownload().

FWIW, would it make sense to sleep for 1 second to get sdch manager to trigger
download, and then request sdch dictionary explicitly. 

The explicit request should be serviced from http cache once sdch manager's
request is completed.

[xunjieli, 2015-04-21 15:01:48]

Patchset #6 (id:100001) has been deleted

[xunjieli, 2015-04-21 15:07:02]

Misha, I updated the tests except the part about fetching dictionary explicitly
part. How do you test or make sure the request is serviced from http cache?

https://codereview.chromium.org/1085903002/diff/120001/components/cronet/andr...
File components/cronet/android/test/assets/test/sdch/index (right):

https://codereview.chromium.org/1085903002/diff/120001/components/cronet/andr...
components/cronet/android/test/assets/test/sdch/index:1: This is an index page.
Since this file doesn't do anything other advertise dictionaries, I changed the
content. The content doesn't matter (isn't used in encoding).

[xunjieli, 2015-04-27 21:35:29]

Patchset #11 (id:220001) has been deleted

[xunjieli, 2015-04-27 21:42:08]

On 2015/04/20 15:40:54, mef wrote:
> looks pretty good. 
> 
> I think we need to add a test where server advertises non-existent dictionary,
> to make sure that sdch manager doesn't crash and burn on that.
> 
>
https://codereview.chromium.org/1085903002/diff/80001/components/cronet/andro...
> File
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java
> (right):
> 
>
https://codereview.chromium.org/1085903002/diff/80001/components/cronet/andro...
>
components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:65:
> Thread.sleep(5000);
> Suggest extracting this is into a specific function like
> waitForDictionaryDownload().
> 
> FWIW, would it make sense to sleep for 1 second to get sdch manager to trigger
> download, and then request sdch dictionary explicitly. 
> 
Done. Btw, sdch manager triggers dictionary fetching when it sees the
Get-Dictionary header immediately if there's no queued request (which is the
case for our unit tests), so we don't need to wait for 1 second. However, I
added a 1 second delay for the manager to add the dictionary to its list of
dictionaries.
PTAL.

[mef, 2015-04-28 20:37:11]

Looks pretty good. 

Do we persist the list of available dictionaries? If yes, do we have a test for
that?

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
File components/cronet/android/test/native_test_server.cc (right):

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:147: // Check for query
params to see which dictionaries to advertise.
Need comment on what query params are used to define which dictionaries to
advertise.

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:150: if
(!base::SplitStringIntoKeyValuePairs(url.query(), '=', '&', &pairs)) {
Suggest: using GetValueForKeyInQuery()

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:153: auto it =
request.headers.find("Accept-Encoding");
'it' is a bit generic, suggest calling it 'accept_encoding_header'.

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:157: for
(base::StringPairs::const_iterator it = pairs.begin();
This 'it' masks upper level 'it', which makes it confusing. 
Can you give 'it' more descriptive name?

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:158: it != pairs.end();
++it) {
Could we use range-based for loop here?
Why do we need a loop? To support multiple dictionaries?
Maybe populate a vector<string> and use base::JoinString()?

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:173: auto it =
request.headers.find("Avail-Dictionary");
more descriptive name than 'it'?

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:182:
response->set_content("Sdch is not used.\n");
maybe construct response based on another file with predefined suffix?

[xunjieli, 2015-04-29 02:05:09]

>Do we persist the list of available dictionaries? If yes, do we have a test 
> for that?

From comments in sdch_manager.h, it seems that dictionaries are only persisted
in memory, and not on disk. Elly, correct me if I'm wrong in my reading of the
code.

Thanks for the review. PTAL.

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
File components/cronet/android/test/native_test_server.cc (right):

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:147: // Check for query
params to see which dictionaries to advertise.
On 2015/04/28 20:37:11, mef wrote:
> Need comment on what query params are used to define which dictionaries to
> advertise.

Done.

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:150: if
(!base::SplitStringIntoKeyValuePairs(url.query(), '=', '&', &pairs)) {
On 2015/04/28 20:37:11, mef wrote:
> Suggest: using GetValueForKeyInQuery()

Done. Was thinking we should support multiple dictionaries. Since the
functionality isn't exercised, I will use GetValueForKeyInQuery.

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:153: auto it =
request.headers.find("Accept-Encoding");
On 2015/04/28 20:37:10, mef wrote:
> 'it' is a bit generic, suggest calling it 'accept_encoding_header'.

Done.

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:157: for
(base::StringPairs::const_iterator it = pairs.begin();
On 2015/04/28 20:37:11, mef wrote:
> This 'it' masks upper level 'it', which makes it confusing. 
> Can you give 'it' more descriptive name?

Done.

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:158: it != pairs.end();
++it) {
On 2015/04/28 20:37:10, mef wrote:
> Could we use range-based for loop here?
> Why do we need a loop? To support multiple dictionaries?
> Maybe populate a vector<string> and use base::JoinString()?

Done. Was thinking of supporting multiple dictionaries. However, since that
isn't used, I will just remove the loop. We can add the functionality later when
we need it. Less code is always better :)

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:173: auto it =
request.headers.find("Avail-Dictionary");
On 2015/04/28 20:37:10, mef wrote:
> more descriptive name than 'it'?

Done.

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:182:
response->set_content("Sdch is not used.\n");
On 2015/04/28 20:37:10, mef wrote:
> maybe construct response based on another file with predefined suffix?

Hmm.. but we just want to make sure here that sdch encoding isn't used. Reading
from file seems like a stretch for that purpose, not mention we will have 2
additional files to install at runtime. There are examples like this in this
file (response->set_content("Request has no body. :(")), so I'd prefer to keep
it like this, unless you have a strong feeling.

[Randy Smith (Not in Mondays), 2015-04-29 15:02:57]

On 2015/04/29 02:05:09, xunjieli wrote:
> >Do we persist the list of available dictionaries? If yes, do we have a test 
> > for that?
> 
> From comments in sdch_manager.h, it seems that dictionaries are only persisted
> in memory, and not on disk. Elly, correct me if I'm wrong in my reading of the
> code.

Elly's the expert, but we do have persistence implemented (at the SdchOwner
level) but behind a flag/configuration variable.  That might be something you
guys might want to specifically have available; Cronet could get the feature
before Chrome!  :-} :-|.


> 
> Thanks for the review. PTAL.
> 
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
> File components/cronet/android/test/native_test_server.cc (right):
> 
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
> components/cronet/android/test/native_test_server.cc:147: // Check for query
> params to see which dictionaries to advertise.
> On 2015/04/28 20:37:11, mef wrote:
> > Need comment on what query params are used to define which dictionaries to
> > advertise.
> 
> Done.
> 
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
> components/cronet/android/test/native_test_server.cc:150: if
> (!base::SplitStringIntoKeyValuePairs(url.query(), '=', '&', &pairs)) {
> On 2015/04/28 20:37:11, mef wrote:
> > Suggest: using GetValueForKeyInQuery()
> 
> Done. Was thinking we should support multiple dictionaries. Since the
> functionality isn't exercised, I will use GetValueForKeyInQuery.
> 
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
> components/cronet/android/test/native_test_server.cc:153: auto it =
> request.headers.find("Accept-Encoding");
> On 2015/04/28 20:37:10, mef wrote:
> > 'it' is a bit generic, suggest calling it 'accept_encoding_header'.
> 
> Done.
> 
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
> components/cronet/android/test/native_test_server.cc:157: for
> (base::StringPairs::const_iterator it = pairs.begin();
> On 2015/04/28 20:37:11, mef wrote:
> > This 'it' masks upper level 'it', which makes it confusing. 
> > Can you give 'it' more descriptive name?
> 
> Done.
> 
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
> components/cronet/android/test/native_test_server.cc:158: it != pairs.end();
> ++it) {
> On 2015/04/28 20:37:10, mef wrote:
> > Could we use range-based for loop here?
> > Why do we need a loop? To support multiple dictionaries?
> > Maybe populate a vector<string> and use base::JoinString()?
> 
> Done. Was thinking of supporting multiple dictionaries. However, since that
> isn't used, I will just remove the loop. We can add the functionality later
when
> we need it. Less code is always better :)
> 
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
> components/cronet/android/test/native_test_server.cc:173: auto it =
> request.headers.find("Avail-Dictionary");
> On 2015/04/28 20:37:10, mef wrote:
> > more descriptive name than 'it'?
> 
> Done.
> 
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
> components/cronet/android/test/native_test_server.cc:182:
> response->set_content("Sdch is not used.\n");
> On 2015/04/28 20:37:10, mef wrote:
> > maybe construct response based on another file with predefined suffix?
> 
> Hmm.. but we just want to make sure here that sdch encoding isn't used.
Reading
> from file seems like a stretch for that purpose, not mention we will have 2
> additional files to install at runtime. There are examples like this in this
> file (response->set_content("Request has no body. :(")), so I'd prefer to keep
> it like this, unless you have a strong feeling.

[xunjieli, 2015-04-29 15:12:41]

>
> Elly's the expert, but we do have persistence implemented (at the SdchOwner
> level) but behind a flag/configuration variable.  That might be something
> you
> guys might want to specifically have available; Cronet could get the
> feature
> before Chrome!  :-} :-|.


I see. Thanks for the explanation! Makes sense. I guess we have the
non-persistent mode in first. I am working on persisting http server
properties info, which has some overlap with this.



>
>  Thanks for the review. PTAL.
>>
>
>
>
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
>
>> File components/cronet/android/test/native_test_server.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
>
>> components/cronet/android/test/native_test_server.cc:147: // Check for
>> query
>> params to see which dictionaries to advertise.
>> On 2015/04/28 20:37:11, mef wrote:
>> > Need comment on what query params are used to define which dictionaries
>> to
>> > advertise.
>>
>
>  Done.
>>
>
>
>
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
>
>> components/cronet/android/test/native_test_server.cc:150: if
>> (!base::SplitStringIntoKeyValuePairs(url.query(), '=', '&', &pairs)) {
>> On 2015/04/28 20:37:11, mef wrote:
>> > Suggest: using GetValueForKeyInQuery()
>>
>
>  Done. Was thinking we should support multiple dictionaries. Since the
>> functionality isn't exercised, I will use GetValueForKeyInQuery.
>>
>
>
>
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
>
>> components/cronet/android/test/native_test_server.cc:153: auto it =
>> request.headers.find("Accept-Encoding");
>> On 2015/04/28 20:37:10, mef wrote:
>> > 'it' is a bit generic, suggest calling it 'accept_encoding_header'.
>>
>
>  Done.
>>
>
>
>
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
>
>> components/cronet/android/test/native_test_server.cc:157: for
>> (base::StringPairs::const_iterator it = pairs.begin();
>> On 2015/04/28 20:37:11, mef wrote:
>> > This 'it' masks upper level 'it', which makes it confusing.
>> > Can you give 'it' more descriptive name?
>>
>
>  Done.
>>
>
>
>
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
>
>> components/cronet/android/test/native_test_server.cc:158: it !=
>> pairs.end();
>> ++it) {
>> On 2015/04/28 20:37:10, mef wrote:
>> > Could we use range-based for loop here?
>> > Why do we need a loop? To support multiple dictionaries?
>> > Maybe populate a vector<string> and use base::JoinString()?
>>
>
>  Done. Was thinking of supporting multiple dictionaries. However, since
>> that
>> isn't used, I will just remove the loop. We can add the functionality
>> later
>>
> when
>
>> we need it. Less code is always better :)
>>
>
>
>
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
>
>> components/cronet/android/test/native_test_server.cc:173: auto it =
>> request.headers.find("Avail-Dictionary");
>> On 2015/04/28 20:37:10, mef wrote:
>> > more descriptive name than 'it'?
>>
>
>  Done.
>>
>
>
>
>
https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
>
>> components/cronet/android/test/native_test_server.cc:182:
>> response->set_content("Sdch is not used.\n");
>> On 2015/04/28 20:37:10, mef wrote:
>> > maybe construct response based on another file with predefined suffix?
>>
>
>  Hmm.. but we just want to make sure here that sdch encoding isn't used.
>>
> Reading
>
>> from file seems like a stretch for that purpose, not mention we will have
>> 2
>> additional files to install at runtime. There are examples like this in
>> this
>> file (response->set_content("Request has no body. :(")), so I'd prefer to
>> keep
>> it like this, unless you have a strong feeling.
>>
>
>
>
> https://codereview.chromium.org/1085903002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mef, 2015-04-29 15:37:29]

Thanks a lot for your comments and changes! LGTM mod nit.

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
File components/cronet/android/test/native_test_server.cc (right):

https://codereview.chromium.org/1085903002/diff/240001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:182:
response->set_content("Sdch is not used.\n");
On 2015/04/29 02:05:09, xunjieli wrote:
> On 2015/04/28 20:37:10, mef wrote:
> > maybe construct response based on another file with predefined suffix?
> 
> Hmm.. but we just want to make sure here that sdch encoding isn't used.
Reading
> from file seems like a stretch for that purpose, not mention we will have 2
> additional files to install at runtime. There are examples like this in this
> file (response->set_content("Request has no body. :(")), so I'd prefer to keep
> it like this, unless you have a strong feeling. 

sg, thanks!

https://codereview.chromium.org/1085903002/diff/260001/components/cronet/andr...
File components/cronet/android/test/native_test_server.cc (right):

https://codereview.chromium.org/1085903002/diff/260001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:36: const char
echo_body_path[] = "/echo_body";
nit: constant names should follow kMixedCase naming pattern.

[xunjieli, 2015-04-29 15:44:23]

Thanks for the review!

https://codereview.chromium.org/1085903002/diff/260001/components/cronet/andr...
File components/cronet/android/test/native_test_server.cc (right):

https://codereview.chromium.org/1085903002/diff/260001/components/cronet/andr...
components/cronet/android/test/native_test_server.cc:36: const char
echo_body_path[] = "/echo_body";
On 2015/04/29 15:37:29, mef wrote:
> nit: constant names should follow kMixedCase naming pattern.

Done.

[xunjieli, 2015-04-29 16:36:09]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-04-29 16:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1085903002/#ps280001
(title: "constants")

[commit-bot: I haz the power, 2015-04-29 16:36:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1085903002/280001

[commit-bot: I haz the power, 2015-04-29 17:36:37]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2015-04-29 17:37:32]

Patchset 13 (id:??) landed as
https://crrev.com/b8a6d56ffe39823af7b09ce6b816d3cd9fbac2a6
Cr-Commit-Position: refs/heads/master@{#327517}