[iOS] Upstream WebP network client

[iOS] Upstream WebP network client

Committed: https://crrev.com/b04f6c5c7c6bee1a30494c3fc67bf3a8904fc129
Cr-Commit-Position: refs/heads/master@{#321578}

[droger, 2015-03-19 10:54:54]

Patchset #1 (id:1) has been deleted

[droger, 2015-03-19 10:55:03]

droger@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2015-03-19 17:06:34]

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
File components/webp_transcode/DEPS (right):

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
components/webp_transcode/DEPS:7: # webp_transcode should not depend on //ios
for library size reasons.
nit: since iOS-specific net files are now in //ios/net, maybe update this
comment to say that //ios/net is temporarily allowed until code is moved to
//net

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
File components/webp_transcode/webp_network_client.h (right):

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
components/webp_transcode/webp_network_client.h:20: (const
scoped_refptr<base::SequencedTaskRunner>&)runner;
style: I think this should be indented by 4 spaces, not 8 (this is probably a
bug of clang-format)

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
File components/webp_transcode/webp_network_client.mm (right):

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
components/webp_transcode/webp_network_client.mm:25: }  // namespace net
nit: no need for the "//  namespace net" for single-line namespaces

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
components/webp_transcode/webp_network_client.mm:159: (const
scoped_refptr<base::SequencedTaskRunner>&)runner {
style: should be 4 space indentation, not 8

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
components/webp_transcode/webp_network_client.mm:174: std::string mimePattern =
std::string("*") + kWebPMimeType + "*";
All those allocations for every native requests make me sad (there are at least
three gratuitious allocation on just this line). Moreover, the "*Constant*"
pattern matching is terrible (look at implementation
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...)
as it must match all Constant pattern against all possible substrings.

What about using std::string::find() instead?

if (acceptHeader.find(kWebPMimeType) != std::string::npos) {
  acceptHeader += std::string(",") + kWebPMimeType;

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
File components/webp_transcode/webp_network_client_factory.h (right):

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
components/webp_transcode/webp_network_client_factory.h:23: (const
scoped_refptr<base::SequencedTaskRunner>&)runner;
style: this should be 4 space indentation, not 8

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
File components/webp_transcode/webp_network_client_factory.mm (right):

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
components/webp_transcode/webp_network_client_factory.mm:29: (const
scoped_refptr<base::SequencedTaskRunner>&)runner {
style: same

[droger, 2015-03-20 10:51:42]

Thanks for the review.

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
File components/webp_transcode/DEPS (right):

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
components/webp_transcode/DEPS:7: # webp_transcode should not depend on //ios
for library size reasons.
On 2015/03/19 17:06:34, sdefresne wrote:
> nit: since iOS-specific net files are now in //ios/net, maybe update this
> comment to say that //ios/net is temporarily allowed until code is moved to
> //net

Not done: It's not yet very clear what will be the long term location for this
code. I'd prefer avoiding giving details now.

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
File components/webp_transcode/webp_network_client.h (right):

https://codereview.chromium.org/1022813002/diff/20001/components/webp_transco...
components/webp_transcode/webp_network_client.h:20: (const
scoped_refptr<base::SequencedTaskRunner>&)runner;
On 2015/03/19 17:06:34, sdefresne wrote:
> style: I think this should be indented by 4 spaces, not 8 (this is probably a
> bug of clang-format)

Yes it is a bug:
https://b.corp.google.com/u/0/issues/19628433

[droger, 2015-03-20 10:52:13]

droger@chromium.org changed reviewers:
+ rsesek@chromium.org

[droger, 2015-03-20 10:52:14]

+rsesek OWNER for the dependency on ocmock

[sdefresne, 2015-03-20 11:06:40]

lgtm

https://codereview.chromium.org/1022813002/diff/40001/components/webp_transco...
File components/webp_transcode/webp_network_client.mm (right):

https://codereview.chromium.org/1022813002/diff/40001/components/webp_transco...
components/webp_transcode/webp_network_client.mm:201: [NSString
stringWithCString:kWebPMimeType encoding:NSASCIIStringEncoding];
nit: can we have a constant for this?

static const NSString* kNSWebPMimeType = @"image/webp";

[Robert Sesek, 2015-03-20 15:38:16]

DEPS LGTM

[droger, 2015-03-20 16:40:00]

The CQ bit was checked by droger@chromium.org

[droger, 2015-03-20 16:40:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1022813002/#ps60001
(title: "Review comments")

[commit-bot: I haz the power, 2015-03-20 16:40:42]

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

[commit-bot: I haz the power, 2015-03-20 16:45:02]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-03-20 16:45:49]

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