[MD settings] i18n source stream filtering

[MD settings] i18n source stream filtering

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

(This TBR is for adding a .cc and .h file to an existing set of files).
TBR=clamy@chromium.org
BUG=None

Committed: https://crrev.com/613a10351e8b6f94e54dba52226ec9b4e5451060
Cr-Commit-Position: refs/heads/master@{#438890}

[dschuyler, 2016-12-01 00:14:09]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 00:17:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 00:33:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 00:33:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[dschuyler, 2016-12-01 01:43:34]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 01:43:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-01 01:44:19]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-12-01 01:45:23]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 01:45:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-01 01:47:07]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-12-01 02:07:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 02:07:28]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Dan Beam, 2016-12-01 05:57:21]

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

[Dan Beam, 2016-12-01 05:57:22]

i know this isn't ready for review yet, but this might help you

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.h (right):

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:26: SourceStream::SourceType type);
why can't this just take a TemplateReplacements as a ctor arg and we ditch
SetReplacements?

static std::unique_ptr<I18nSourceStream> Create(
    std::unique_ptr<SourceStream> previous,
    SourceStream::SourceType type,
    const ui::TemplateReplacements* replacements);

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:29: void SetReplacements(const
ui::TemplateReplacements* replacements);
also, this should be set_replacements() if it stays (it's just a vanilla setter,
not doing anything hard, nor is it virtual)

you could potentially even do it inline in the header...

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:408:
net::URLRequestJob::SetUpSourceStream();
i THINK this will fix your compile error

std::unique_ptr<net::SourceStream> source_stream =
    net::URLRequestJob::SetUpSourceStream();
if (!is_gzipped_)
  return std::move(source_stream);

std::unique_ptr<net::SourceStream> gzip_source_stream =
    net::GzipSourceStream::Create(std::move(source_stream), 
                                  net::SourceStream::TYPE_GZIP);
if (!replacements_)
  return std::move(gzip_source_stream);

std::unique_ptr<net::SourceStream> i18n_source_stream =
    content::I18nSourceStream::Create(std::move(gzip_source_stream),
                                      net::SourceStream::TYPE_NONE);
return std::move(i18n_source_stream);

[dschuyler, 2016-12-01 20:34:02]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 20:34:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-01 21:41:29]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 21:41:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 22:53:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 22:53:35]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-12-01 23:07:14]

Description was changed from

==========
[MD settings] i18n source stream filtering

Not for Review. This version is functional, but not great.

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

BUG=None
==========

to

==========
[MD settings] i18n source stream filtering

Not for Review. This version is functional, but not great. I believe there is a
potentiall thread safety issue that I want to look at further.

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

BUG=None
==========

[dschuyler, 2016-12-01 23:07:26]

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.h (right):

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:26: SourceStream::SourceType type);
On 2016/12/01 05:57:22, Dan Beam wrote:
> why can't this just take a TemplateReplacements as a ctor arg and we ditch
> SetReplacements?
> 
> static std::unique_ptr<I18nSourceStream> Create(
>     std::unique_ptr<SourceStream> previous,
>     SourceStream::SourceType type,
>     const ui::TemplateReplacements* replacements);

Done.

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:29: void SetReplacements(const
ui::TemplateReplacements* replacements);
On 2016/12/01 05:57:22, Dan Beam wrote:
> also, this should be set_replacements() if it stays (it's just a vanilla
setter,
> not doing anything hard, nor is it virtual)
> 
> you could potentially even do it inline in the header...

Let's go with removing it.

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2544683002/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:408:
net::URLRequestJob::SetUpSourceStream();
On 2016/12/01 05:57:22, Dan Beam wrote:
> i THINK this will fix your compile error
> 
> std::unique_ptr<net::SourceStream> source_stream =
>     net::URLRequestJob::SetUpSourceStream();
> if (!is_gzipped_)
>   return std::move(source_stream);
> 
> std::unique_ptr<net::SourceStream> gzip_source_stream =
>     net::GzipSourceStream::Create(std::move(source_stream), 
>                                   net::SourceStream::TYPE_GZIP);
> if (!replacements_)
>   return std::move(gzip_source_stream);
> 
> std::unique_ptr<net::SourceStream> i18n_source_stream =
>     content::I18nSourceStream::Create(std::move(gzip_source_stream),
>                                       net::SourceStream::TYPE_NONE);
> return std::move(i18n_source_stream);

It's problematic to use std::move on a return because it hinders optimization. I
think the error is related to returning a different type (a derived type, but
still different). I'd like to try returning the same type.

We don't want to early out on "if (!is_gzipped_)", because it a file may not be
zipped and still need i18n replacements.

I switched to doing the moves within the function and returning only the
source_stream without a move.

[Dan Beam, 2016-12-01 23:32:23]

otherwise looking good

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:18: DCHECK(replacements);
#include "base/logging.h" for DCHECK

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:36: net::IOBuffer* input_buffer,
you might need #include "net/base/io_buffer.h" when you /use/ IOBuffer, though

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.h (right):

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:12: #include
"base/memory/ref_counted.h"
^ what is this for?

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:13: #include "net/base/io_buffer.h"
you don't need this when net::IOBuffer is part of SourceStream's interface

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:51: bool upstream_end_reached)
override;
put these methods before the members_

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/w...
File content/browser/webui/web_ui_data_source_impl.cc (left):

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/w...
content/browser/webui/web_ui_data_source_impl.cc:277: if (response.get() &&
GetMimeType(path) == "text/html" &&
so how are we only running replacements on text/html without this block?

[dschuyler, 2016-12-02 20:14:01]

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:18: DCHECK(replacements);
On 2016/12/01 23:32:22, Dan Beam wrote:
> #include "base/logging.h" for DCHECK

Done.

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:36: net::IOBuffer* input_buffer,
On 2016/12/01 23:32:22, Dan Beam wrote:
> you might need #include "net/base/io_buffer.h" when you /use/ IOBuffer, though

Done.

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.h (right):

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:12: #include
"base/memory/ref_counted.h"
On 2016/12/01 23:32:23, Dan Beam wrote:
> ^ what is this for?

Done.

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:13: #include "net/base/io_buffer.h"
On 2016/12/01 23:32:23, Dan Beam wrote:
> you don't need this when net::IOBuffer is part of SourceStream's interface

Done.

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:51: bool upstream_end_reached)
override;
On 2016/12/01 23:32:23, Dan Beam wrote:
> put these methods before the members_

Done.

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/w...
File content/browser/webui/web_ui_data_source_impl.cc (left):

https://codereview.chromium.org/2544683002/diff/80001/content/browser/webui/w...
content/browser/webui/web_ui_data_source_impl.cc:277: if (response.get() &&
GetMimeType(path) == "text/html" &&
On 2016/12/01 23:32:23, Dan Beam wrote:
> so how are we only running replacements on text/html without this block?

Done.

[Dan Beam, 2016-12-02 23:19:11]

did you mean to upload a new patchset?

[dschuyler, 2016-12-02 23:21:35]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 23:22:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-02 23:22:51]

On 2016/12/02 23:19:11, Dan Beam wrote:
> did you mean to upload a new patchset?

I did mean to. It's now up as Patch#4.

[Dan Beam, 2016-12-02 23:54:47]

i don't think we need to add new members or API to process only text/html
resources

we should be able to do any of:

return nullptr from GetReplacements() if mime != "text/html"
only SetReplacements() if mime == "text/html"
only set up an i18n source stream if mime == "text/html"

preferrably in that order

[commit-bot: I haz the power, 2016-12-03 02:04:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 02:04:43]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-12-03 02:13:34]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 02:14:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-03 02:15:03]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 02:15:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-03 02:15:20]

Patchset #5 (id:120001) has been deleted

[dschuyler, 2016-12-03 02:17:46]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[dschuyler, 2016-12-03 02:17:51]

Patchset #5 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-12-03 02:18:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-03 02:19:02]

Description was changed from

==========
[MD settings] i18n source stream filtering

Not for Review. This version is functional, but not great. I believe there is a
potentiall thread safety issue that I want to look at further.

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

BUG=None
==========

to

==========
[MD settings] i18n source stream filtering

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

BUG=None
==========

[Dan Beam, 2016-12-03 02:25:29]

lgtm, but you should add a unit test for I18nSourceStream

https://codereview.chromium.org/2544683002/diff/160001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2544683002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:720: if
(source->source()->GetMimeType(path) == "text/html")
maybe transfer your TODO about this from the old mime-filtering location/code?

https://codereview.chromium.org/2544683002/diff/160001/content/browser/webui/...
File content/browser/webui/web_ui_data_source_impl.cc (right):

https://codereview.chromium.org/2544683002/diff/160001/content/browser/webui/...
content/browser/webui/web_ui_data_source_impl.cc:215: void
WebUIDataSourceImpl::FinalizeReplacements() {
nit: EnsureLoadTimeDataDefaultsAdded() ?

[dschuyler, 2016-12-03 02:38:38]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 02:39:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-03 02:42:57]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 02:43:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-03 02:43:23]

Patchset #6 (id:180001) has been deleted

[dschuyler, 2016-12-03 02:44:56]

dschuyler@chromium.org changed reviewers:
+ mmenke@chromium.org

[dschuyler, 2016-12-03 02:44:57]

https://codereview.chromium.org/2544683002/diff/160001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2544683002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:720: if
(source->source()->GetMimeType(path) == "text/html")
On 2016/12/03 02:25:28, Dan Beam wrote:
> maybe transfer your TODO about this from the old mime-filtering location/code?

Done.

https://codereview.chromium.org/2544683002/diff/160001/content/browser/webui/...
File content/browser/webui/web_ui_data_source_impl.cc (right):

https://codereview.chromium.org/2544683002/diff/160001/content/browser/webui/...
content/browser/webui/web_ui_data_source_impl.cc:215: void
WebUIDataSourceImpl::FinalizeReplacements() {
On 2016/12/03 02:25:28, Dan Beam wrote:
> nit: EnsureLoadTimeDataDefaultsAdded() ?

Done.

[commit-bot: I haz the power, 2016-12-03 04:47:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 04:47:47]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-12-07 01:53:13]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 01:53:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-07 01:54:41]

Added unit tests modeled after the gzip source stream unit tests.

[Dan Beam, 2016-12-07 03:12:46]

i don't really understand the unit test all too well, but i'm sure there's
something useful in there!  still lgtm

[commit-bot: I haz the power, 2016-12-07 04:14:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 04:14:14]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-08 23:29:53]

On 2016/12/07 03:12:46, Dan Beam wrote:
> i don't really understand the unit test all too well, but i'm sure there's
> something useful in there!  still lgtm

I'll get to this tomorrow.  Please, if you add someone as a reviewer, send out a
comment asking them to review the CL, and what parts of it you want them to
review.  Otherwise, they're unlikely to know you want a review from them.

[dschuyler, 2016-12-09 00:01:57]

On 2016/12/08 23:29:53, mmenke wrote:
> On 2016/12/07 03:12:46, Dan Beam wrote:
> > i don't really understand the unit test all too well, but i'm sure there's
> > something useful in there!  still lgtm
> 
> I'll get to this tomorrow.  Please, if you add someone as a reviewer, send out
a
> comment asking them to review the CL, and what parts of it you want them to
> review.  Otherwise, they're unlikely to know you want a review from them.

Thanks for the note. I don't have a specific bit of the CL that I'm looking for
your review on. Since Dan has already already had some feedback on the CL, feel
free to look at it in detail or skim it and RS LGTM it. Or as a middle ground,
here are some questions I'm curious about:
- Is it okay that I put the i18n_source_stream in the content dir (or is there a
desire that all source streams be in the net/filter dir)?
- At the moment, this 'stream' will accumulate the data, then process it, then
send on the results. That's not particularly stream-like, but the files involved
are small and I don't expect the downstream code to make much use of the data
until it's complete (it's simple html pages - not something that benefits
greatly from streaming like a movie or audio). Is that okay?

[mmenke, 2016-12-09 18:40:17]

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:48:
input_.append(input_buffer->data(), input_buffer_size);
I assume there's no way for non-chrome files to reach this point (Extension
requests, etc).

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:49: *consumed_bytes =
input_buffer_size;
I think both of these lines can be done unconditionally.  Currently, if you
always consume all the data, you'll be given a final call with an
input_buffer_size of 0, but I don't think that's part of the API contract. 
Maybe add a test case for this?

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:52: output_ =
ui::ReplaceTemplateExpressions(base::StringPiece(input_),
base::StringPiece not needed - it should be implicitly converted.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:52: output_ =
ui::ReplaceTemplateExpressions(base::StringPiece(input_),
Include string_piece.h (Even without the explicit conversion, need the header
for the implicit one)

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:54: input_.clear();
Shouldn't modify input_ here.  I don't think it breaks anything, it's not
expected.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:59: bytes_out =
std::min(static_cast<int>(output_.size()), output_buffer_size);
Doesn't really matter, but from a "this works even for insanely large strings
that it shouldn't be fed anyways", standpoint, this should be:

bytes_out = std::min(output_.size(), static_cast<size_t>(output_buffer_size));

(i.e., this has no issues with overflow, though it does do two casts instead of
one)

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:61: output_.erase(0, bytes_out);
Is it worth the erase?  The memory allocated by the std::string may well not be
reduced by this call, so this could just be a wasted copy.  Could instead
remember the offset of the last byte that was written.

[dschuyler, 2016-12-09 22:26:43]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 22:27:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-09 22:31:17]

Thanks!

I gave some explanation on some of the comments, I'm good with making the
remaining changes if you prefer.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:48:
input_.append(input_buffer->data(), input_buffer_size);
On 2016/12/09 18:40:17, mmenke wrote:
> I assume there's no way for non-chrome files to reach this point (Extension
> requests, etc).
Same here, but:

Is there something that should change if non-chrome files could be run through
this filter? IIUC, if something rouge did run data through this filter, what
they could do is get the i18n values by passing a buffer with the $i18n{} tags
with the keys. Which would be ineffective as an exploit afaik, since the i18n
values are just translated strings with no PII.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:49: *consumed_bytes =
input_buffer_size;
On 2016/12/09 18:40:17, mmenke wrote:
> I think both of these lines can be done unconditionally.  Currently, if you
> always consume all the data, you'll be given a final call with an
> input_buffer_size of 0, but I don't think that's part of the API contract. 
> Maybe add a test case for this?

I agree that they could be moved outside of the if(), but the condition would
then move to line 50 (like "if (upstream_end_reached && !input_.empty() &&
output_.empty())"). That would mean that the condition is still checked and
append() is called during the processing and the draining. I agree that the
append() would effectively be a no-op, but it may still be a cache miss to go
over to that code (to do nothing and come back, err, unless it's inlined). I
also think it's clearer to see the three stages with three blocks.

I did move the "*consumed_bytes = input_buffer_size;" line.

Please let me know if you're unconvinced and still want this changed ("please
change it" would be clear enough).

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:52: output_ =
ui::ReplaceTemplateExpressions(base::StringPiece(input_),
On 2016/12/09 18:40:17, mmenke wrote:
> base::StringPiece not needed - it should be implicitly converted.

Done.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:52: output_ =
ui::ReplaceTemplateExpressions(base::StringPiece(input_),
On 2016/12/09 18:40:17, mmenke wrote:
> base::StringPiece not needed - it should be implicitly converted.

Done.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:54: input_.clear();
On 2016/12/09 18:40:17, mmenke wrote:
> Shouldn't modify input_ here.  I don't think it breaks anything, it's not
> expected.

done.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:59: bytes_out =
std::min(static_cast<int>(output_.size()), output_buffer_size);
On 2016/12/09 18:40:17, mmenke wrote:
> Doesn't really matter, but from a "this works even for insanely large strings
> that it shouldn't be fed anyways", standpoint, this should be:
> 
> bytes_out = std::min(output_.size(), static_cast<size_t>(output_buffer_size));
> 
> (i.e., this has no issues with overflow, though it does do two casts instead
of
> one)

I can see that being good practice in general (convert to the larger type).
Since this won't have an affect unless the type for output_buffer_size changes;
and that change will generate a type mismatch message from std::min, it seems
good to skip the extra conversions and cast to int here. i.e. if the type of
output_buffer_size could change without a warning, then I'd want to make this a
size_t cast now to avoid errors (I would have done it that way twenty years ago,
for example).

Please let me know if you're unconvinced and still want this changed ("please
change it" would be clear enough).

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:61: output_.erase(0, bytes_out);
On 2016/12/09 18:40:17, mmenke wrote:
> Is it worth the erase?  The memory allocated by the std::string may well not
be
> reduced by this call, so this could just be a wasted copy.  Could instead
> remember the offset of the last byte that was written.

Depending on how erase() is implemented, this may be about the same either way.
I agree though, that it's nice to make it explicit - thanks for suggesting it.

Done.

[commit-bot: I haz the power, 2016-12-09 22:34:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 22:34:20]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mmenke, 2016-12-09 22:54:09]

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:48:
input_.append(input_buffer->data(), input_buffer_size);
On 2016/12/09 22:31:17, dschuyler wrote:
> On 2016/12/09 18:40:17, mmenke wrote:
> > I assume there's no way for non-chrome files to reach this point (Extension
> > requests, etc).
> Same here, but:
> 
> Is there something that should change if non-chrome files could be run through
> this filter? IIUC, if something rouge did run data through this filter, what
> they could do is get the i18n values by passing a buffer with the $i18n{} tags
> with the keys. Which would be ineffective as an exploit afaik, since the i18n
> values are just translated strings with no PII.

You're buffering the entire response in a single string, potentially OOMing the
browser if this were applied to an attacker-provided resource.  The other
filters all stream data as they get it.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:49: *consumed_bytes =
input_buffer_size;
On 2016/12/09 22:31:17, dschuyler wrote:
> On 2016/12/09 18:40:17, mmenke wrote:
> > I think both of these lines can be done unconditionally.  Currently, if you
> > always consume all the data, you'll be given a final call with an
> > input_buffer_size of 0, but I don't think that's part of the API contract. 
> > Maybe add a test case for this?
> 
> I agree that they could be moved outside of the if(), but the condition would
> then move to line 50 (like "if (upstream_end_reached && !input_.empty() &&
> output_.empty())"). That would mean that the condition is still checked and
> append() is called during the processing and the draining. I agree that the
> append() would effectively be a no-op, but it may still be a cache miss to go
> over to that code (to do nothing and come back, err, unless it's inlined). I
> also think it's clearer to see the three stages with three blocks.
> 
> I did move the "*consumed_bytes = input_buffer_size;" line.
> 
> Please let me know if you're unconvinced and still want this changed ("please
> change it" would be clear enough).

I'm not sure we're on the same page here.  The API contract does *not* guarantee
that if |upstream_end_reached| is true, then input_buffer will be empty.  The
fact that is currently always the case is just the behavior of the current
implementation.  So the current code does not adhere to the API that
FilterSourceStream requires.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:59: bytes_out =
std::min(static_cast<int>(output_.size()), output_buffer_size);
On 2016/12/09 22:31:16, dschuyler wrote:
> On 2016/12/09 18:40:17, mmenke wrote:
> > Doesn't really matter, but from a "this works even for insanely large
strings
> > that it shouldn't be fed anyways", standpoint, this should be:
> > 
> > bytes_out = std::min(output_.size(),
static_cast<size_t>(output_buffer_size));
> > 
> > (i.e., this has no issues with overflow, though it does do two casts instead
> of
> > one)
> 
> I can see that being good practice in general (convert to the larger type).
> Since this won't have an affect unless the type for output_buffer_size
changes;
> and that change will generate a type mismatch message from std::min, it seems
> good to skip the extra conversions and cast to int here. i.e. if the type of
> output_buffer_size could change without a warning, then I'd want to make this
a
> size_t cast now to avoid errors (I would have done it that way twenty years
ago,
> for example).
> 
> Please let me know if you're unconvinced and still want this changed ("please
> change it" would be clear enough).

There's nothing preventing output_buffer_size_ from being over 2 GB, so I'm not
sure what you mean by this changing?  Admittedly, it doesn't really matter here,
as, barring a bug, it shouldn't be that big.  Just thought I'm mention it.

[mmenke, 2016-12-09 22:55:21]

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:48:
input_.append(input_buffer->data(), input_buffer_size);
On 2016/12/09 22:54:08, mmenke wrote:
> On 2016/12/09 22:31:17, dschuyler wrote:
> > On 2016/12/09 18:40:17, mmenke wrote:
> > > I assume there's no way for non-chrome files to reach this point
(Extension
> > > requests, etc).
> > Same here, but:
> > 
> > Is there something that should change if non-chrome files could be run
through
> > this filter? IIUC, if something rouge did run data through this filter, what
> > they could do is get the i18n values by passing a buffer with the $i18n{}
tags
> > with the keys. Which would be ineffective as an exploit afaik, since the
i18n
> > values are just translated strings with no PII.
> 
> You're buffering the entire response in a single string, potentially OOMing
the
> browser if this were applied to an attacker-provided resource.  The other
> filters all stream data as they get it.

Also, just from a more general memory standpoint, even if we only use it for
chrome resources, this could be a problem on mobile, where memory is more
limited.

[dschuyler, 2016-12-10 00:02:31]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 00:03:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-10 00:05:55]

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:48:
input_.append(input_buffer->data(), input_buffer_size);
On 2016/12/09 22:54:08, mmenke wrote:
> On 2016/12/09 22:31:17, dschuyler wrote:
> > On 2016/12/09 18:40:17, mmenke wrote:
> > > I assume there's no way for non-chrome files to reach this point
(Extension
> > > requests, etc).
> > Same here, but:
> > 
> > Is there something that should change if non-chrome files could be run
through
> > this filter? IIUC, if something rouge did run data through this filter, what
> > they could do is get the i18n values by passing a buffer with the $i18n{}
tags
> > with the keys. Which would be ineffective as an exploit afaik, since the
i18n
> > values are just translated strings with no PII.
> 
> You're buffering the entire response in a single string, potentially OOMing
the
> browser if this were applied to an attacker-provided resource.  The other
> filters all stream data as they get it.

I'd like to make this more streaming, that's what the todo on line 45 is about.
This CL is intended to get a baseline i18n working.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:48:
input_.append(input_buffer->data(), input_buffer_size);
On 2016/12/09 22:55:21, mmenke wrote:
> On 2016/12/09 22:54:08, mmenke wrote:
> > On 2016/12/09 22:31:17, dschuyler wrote:
> > > On 2016/12/09 18:40:17, mmenke wrote:
> > > > I assume there's no way for non-chrome files to reach this point
> (Extension
> > > > requests, etc).
> > > Same here, but:
> > > 
> > > Is there something that should change if non-chrome files could be run
> through
> > > this filter? IIUC, if something rouge did run data through this filter,
what
> > > they could do is get the i18n values by passing a buffer with the $i18n{}
> tags
> > > with the keys. Which would be ineffective as an exploit afaik, since the
> i18n
> > > values are just translated strings with no PII.
> > 
> > You're buffering the entire response in a single string, potentially OOMing
> the
> > browser if this were applied to an attacker-provided resource.  The other
> > filters all stream data as they get it.
> 
> Also, just from a more general memory standpoint, even if we only use it for
> chrome resources, this could be a problem on mobile, where memory is more
> limited.

Acknowledged.

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:49: *consumed_bytes =
input_buffer_size;
On 2016/12/09 22:54:08, mmenke wrote:
> On 2016/12/09 22:31:17, dschuyler wrote:
> > On 2016/12/09 18:40:17, mmenke wrote:
> > > I think both of these lines can be done unconditionally.  Currently, if
you
> > > always consume all the data, you'll be given a final call with an
> > > input_buffer_size of 0, but I don't think that's part of the API contract.

> > > Maybe add a test case for this?
> > 
> > I agree that they could be moved outside of the if(), but the condition
would
> > then move to line 50 (like "if (upstream_end_reached && !input_.empty() &&
> > output_.empty())"). That would mean that the condition is still checked and
> > append() is called during the processing and the draining. I agree that the
> > append() would effectively be a no-op, but it may still be a cache miss to
go
> > over to that code (to do nothing and come back, err, unless it's inlined). I
> > also think it's clearer to see the three stages with three blocks.
> > 
> > I did move the "*consumed_bytes = input_buffer_size;" line.
> > 
> > Please let me know if you're unconvinced and still want this changed
("please
> > change it" would be clear enough).
> 
> I'm not sure we're on the same page here.  The API contract does *not*
guarantee
> that if |upstream_end_reached| is true, then input_buffer will be empty.  The
> fact that is currently always the case is just the behavior of the current
> implementation.  So the current code does not adhere to the API that
> FilterSourceStream requires.

I didn't know that. Thanks, that helps me understand the issue better (I feel
like I learned something; that makes me feel better about the change than just
changing it without knowing why).

https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:59: bytes_out =
std::min(static_cast<int>(output_.size()), output_buffer_size);
On 2016/12/09 22:54:08, mmenke wrote:
> On 2016/12/09 22:31:16, dschuyler wrote:
> > On 2016/12/09 18:40:17, mmenke wrote:
> > > Doesn't really matter, but from a "this works even for insanely large
> strings
> > > that it shouldn't be fed anyways", standpoint, this should be:
> > > 
> > > bytes_out = std::min(output_.size(),
> static_cast<size_t>(output_buffer_size));
> > > 
> > > (i.e., this has no issues with overflow, though it does do two casts
instead
> > of
> > > one)
> > 
> > I can see that being good practice in general (convert to the larger type).
> > Since this won't have an affect unless the type for output_buffer_size
> changes;
> > and that change will generate a type mismatch message from std::min, it
seems
> > good to skip the extra conversions and cast to int here. i.e. if the type of
> > output_buffer_size could change without a warning, then I'd want to make
this
> a
> > size_t cast now to avoid errors (I would have done it that way twenty years
> ago,
> > for example).
> > 
> > Please let me know if you're unconvinced and still want this changed
("please
> > change it" would be clear enough).
> 
> There's nothing preventing output_buffer_size_ from being over 2 GB, so I'm
not
> sure what you mean by this changing?  Admittedly, it doesn't really matter
here,
> as, barring a bug, it shouldn't be that big.  Just thought I'm mention it.

I wasn't seeing the signed vs unsigned issue. Thanks!

[commit-bot: I haz the power, 2016-12-10 02:05:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 02:05:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[dschuyler, 2016-12-14 00:15:58]

On 2016/12/10 00:05:55, dschuyler wrote:
>
https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
> File content/browser/webui/i18n_source_stream.cc (right):
> 
>
https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
> content/browser/webui/i18n_source_stream.cc:48:
> input_.append(input_buffer->data(), input_buffer_size);
> On 2016/12/09 22:54:08, mmenke wrote:
> > On 2016/12/09 22:31:17, dschuyler wrote:
> > > On 2016/12/09 18:40:17, mmenke wrote:
> > > > I assume there's no way for non-chrome files to reach this point
> (Extension
> > > > requests, etc).
> > > Same here, but:
> > > 
> > > Is there something that should change if non-chrome files could be run
> through
> > > this filter? IIUC, if something rouge did run data through this filter,
what
> > > they could do is get the i18n values by passing a buffer with the $i18n{}
> tags
> > > with the keys. Which would be ineffective as an exploit afaik, since the
> i18n
> > > values are just translated strings with no PII.
> > 
> > You're buffering the entire response in a single string, potentially OOMing
> the
> > browser if this were applied to an attacker-provided resource.  The other
> > filters all stream data as they get it.
> 
> I'd like to make this more streaming, that's what the todo on line 45 is
about.
> This CL is intended to get a baseline i18n working.
> 
>
https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
> content/browser/webui/i18n_source_stream.cc:48:
> input_.append(input_buffer->data(), input_buffer_size);
> On 2016/12/09 22:55:21, mmenke wrote:
> > On 2016/12/09 22:54:08, mmenke wrote:
> > > On 2016/12/09 22:31:17, dschuyler wrote:
> > > > On 2016/12/09 18:40:17, mmenke wrote:
> > > > > I assume there's no way for non-chrome files to reach this point
> > (Extension
> > > > > requests, etc).
> > > > Same here, but:
> > > > 
> > > > Is there something that should change if non-chrome files could be run
> > through
> > > > this filter? IIUC, if something rouge did run data through this filter,
> what
> > > > they could do is get the i18n values by passing a buffer with the
$i18n{}
> > tags
> > > > with the keys. Which would be ineffective as an exploit afaik, since the
> > i18n
> > > > values are just translated strings with no PII.
> > > 
> > > You're buffering the entire response in a single string, potentially
OOMing
> > the
> > > browser if this were applied to an attacker-provided resource.  The other
> > > filters all stream data as they get it.
> > 
> > Also, just from a more general memory standpoint, even if we only use it for
> > chrome resources, this could be a problem on mobile, where memory is more
> > limited.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
> content/browser/webui/i18n_source_stream.cc:49: *consumed_bytes =
> input_buffer_size;
> On 2016/12/09 22:54:08, mmenke wrote:
> > On 2016/12/09 22:31:17, dschuyler wrote:
> > > On 2016/12/09 18:40:17, mmenke wrote:
> > > > I think both of these lines can be done unconditionally.  Currently, if
> you
> > > > always consume all the data, you'll be given a final call with an
> > > > input_buffer_size of 0, but I don't think that's part of the API
contract.
> 
> > > > Maybe add a test case for this?
> > > 
> > > I agree that they could be moved outside of the if(), but the condition
> would
> > > then move to line 50 (like "if (upstream_end_reached && !input_.empty() &&
> > > output_.empty())"). That would mean that the condition is still checked
and
> > > append() is called during the processing and the draining. I agree that
the
> > > append() would effectively be a no-op, but it may still be a cache miss to
> go
> > > over to that code (to do nothing and come back, err, unless it's inlined).
I
> > > also think it's clearer to see the three stages with three blocks.
> > > 
> > > I did move the "*consumed_bytes = input_buffer_size;" line.
> > > 
> > > Please let me know if you're unconvinced and still want this changed
> ("please
> > > change it" would be clear enough).
> > 
> > I'm not sure we're on the same page here.  The API contract does *not*
> guarantee
> > that if |upstream_end_reached| is true, then input_buffer will be empty. 
The
> > fact that is currently always the case is just the behavior of the current
> > implementation.  So the current code does not adhere to the API that
> > FilterSourceStream requires.
> 
> I didn't know that. Thanks, that helps me understand the issue better (I feel
> like I learned something; that makes me feel better about the change than just
> changing it without knowing why).
> 
>
https://codereview.chromium.org/2544683002/diff/220001/content/browser/webui/...
> content/browser/webui/i18n_source_stream.cc:59: bytes_out =
> std::min(static_cast<int>(output_.size()), output_buffer_size);
> On 2016/12/09 22:54:08, mmenke wrote:
> > On 2016/12/09 22:31:16, dschuyler wrote:
> > > On 2016/12/09 18:40:17, mmenke wrote:
> > > > Doesn't really matter, but from a "this works even for insanely large
> > strings
> > > > that it shouldn't be fed anyways", standpoint, this should be:
> > > > 
> > > > bytes_out = std::min(output_.size(),
> > static_cast<size_t>(output_buffer_size));
> > > > 
> > > > (i.e., this has no issues with overflow, though it does do two casts
> instead
> > > of
> > > > one)
> > > 
> > > I can see that being good practice in general (convert to the larger
type).
> > > Since this won't have an affect unless the type for output_buffer_size
> > changes;
> > > and that change will generate a type mismatch message from std::min, it
> seems
> > > good to skip the extra conversions and cast to int here. i.e. if the type
of
> > > output_buffer_size could change without a warning, then I'd want to make
> this
> > a
> > > size_t cast now to avoid errors (I would have done it that way twenty
years
> > ago,
> > > for example).
> > > 
> > > Please let me know if you're unconvinced and still want this changed
> ("please
> > > change it" would be clear enough).
> > 
> > There's nothing preventing output_buffer_size_ from being over 2 GB, so I'm
> not
> > sure what you mean by this changing?  Admittedly, it doesn't really matter
> here,
> > as, barring a bug, it shouldn't be that big.  Just thought I'm mention it.
> 
> I wasn't seeing the signed vs unsigned issue. Thanks!


mmenke@, how does this CL look now?

[mmenke, 2016-12-14 16:01:50]

Sorry, took Monday off, and lost this in the flood of reviews I had waiting on
me yesterday.  Looking now.

[mmenke, 2016-12-14 16:10:45]

LGTM, sorry again for the delay.  A couple tiny suggestions.

https://codereview.chromium.org/2544683002/diff/260001/content/browser/webui/...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2544683002/diff/260001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:44: bool upstream_end_reached) {
optional:  Suggest DCHECK(output_.empty() || (upstream_end_reached &&
input_buffer_size == 0));

To document the invariant this code depends on.

https://codereview.chromium.org/2544683002/diff/260001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:46: int bytes_out = 0;
If you decide not to go with my suggestion below, you should at least move this
down to just above the last if statement.  Google style is to declare variables
just before first use, to reduce their scope.

https://codereview.chromium.org/2544683002/diff/260001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:55: if (drain_offset_ <
output_.size()) {
optional nit:  When there's nothing else to do, early return is often preferred
(Though in cases where if more code were added, you'd want it to be done in both
cases, early return may not be a good idea).

Anyhow, suggest going with:

if (drain_offset_ == output_.size())
  return;

(== is a bit clearer than >=)

And then you can also declare bytes_read down here, too.

[dschuyler, 2016-12-15 00:16:04]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 00:16:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-15 00:17:09]

https://codereview.chromium.org/2544683002/diff/260001/content/browser/webui/...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2544683002/diff/260001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:44: bool upstream_end_reached) {
On 2016/12/14 16:10:45, mmenke (Out Dec 17 to Jan 2) wrote:
> optional:  Suggest DCHECK(output_.empty() || (upstream_end_reached &&
> input_buffer_size == 0));
> 
> To document the invariant this code depends on.

Done.

https://codereview.chromium.org/2544683002/diff/260001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:46: int bytes_out = 0;
On 2016/12/14 16:10:45, mmenke (Out Dec 17 to Jan 2) wrote:
> If you decide not to go with my suggestion below, you should at least move
this
> down to just above the last if statement.  Google style is to declare
variables
> just before first use, to reduce their scope.

Done.

https://codereview.chromium.org/2544683002/diff/260001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:55: if (drain_offset_ <
output_.size()) {
On 2016/12/14 16:10:45, mmenke (Out Dec 17 to Jan 2) wrote:
> optional nit:  When there's nothing else to do, early return is often
preferred
> (Though in cases where if more code were added, you'd want it to be done in
both
> cases, early return may not be a good idea).
> 
> Anyhow, suggest going with:
> 
> if (drain_offset_ == output_.size())
>   return;
> 
> (== is a bit clearer than >=)
> 
> And then you can also declare bytes_read down here, too.

Done.

[commit-bot: I haz the power, 2016-12-15 01:17:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:17:32]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-12-15 01:21:07]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-12-15 01:21:07]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2544683002/#ps280001
(title: "review nits")

[commit-bot: I haz the power, 2016-12-15 01:21:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 01:31:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:31:24]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dschuyler, 2016-12-15 02:12:13]

dschuyler@chromium.org changed reviewers:
+ clamy@chromium.org

[dschuyler, 2016-12-15 02:12:14]

clamy@ for OWNER on content/browser/BUILD.gn
(I believe it's just that one file that needs owner lgtm).

[dschuyler, 2016-12-15 19:14:46]

Description was changed from

==========
[MD settings] i18n source stream filtering

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

BUG=None
==========

to

==========
[MD settings] i18n source stream filtering

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

(This TBR is for adding a .cc and .h file to an existing set of files).
TBR=clamy@chromium.org
BUG=None
==========

[dschuyler, 2016-12-15 19:14:58]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-12-15 19:15:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 19:22:39]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1481829298243040,
"parent_rev": "08e0d0b324100cdded0e8103ac5d3ddcc0f15b48", "commit_rev":
"ffe4f09ff9729d3a63a6f60548f93206c732af26"}

[commit-bot: I haz the power, 2016-12-15 19:23:09]

Description was changed from

==========
[MD settings] i18n source stream filtering

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

(This TBR is for adding a .cc and .h file to an existing set of files).
TBR=clamy@chromium.org
BUG=None
==========

to

==========
[MD settings] i18n source stream filtering

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

(This TBR is for adding a .cc and .h file to an existing set of files).
TBR=clamy@chromium.org
BUG=None

Review-Url: https://codereview.chromium.org/2544683002
==========

[commit-bot: I haz the power, 2016-12-15 19:23:11]

Committed patchset #10 (id:280001)

[commit-bot: I haz the power, 2016-12-15 19:24:46]

Description was changed from

==========
[MD settings] i18n source stream filtering

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

(This TBR is for adding a .cc and .h file to an existing set of files).
TBR=clamy@chromium.org
BUG=None

Review-Url: https://codereview.chromium.org/2544683002
==========

to

==========
[MD settings] i18n source stream filtering

This CL performs the i18n template replacement in a source stream so
that i18n replacement can be used with gzip streaming decompression.

(This TBR is for adding a .cc and .h file to an existing set of files).
TBR=clamy@chromium.org
BUG=None

Committed: https://crrev.com/613a10351e8b6f94e54dba52226ec9b4e5451060
Cr-Commit-Position: refs/heads/master@{#438890}
==========

[commit-bot: I haz the power, 2016-12-15 19:24:47]

Patchset 10 (id:??) landed as
https://crrev.com/613a10351e8b6f94e54dba52226ec9b4e5451060
Cr-Commit-Position: refs/heads/master@{#438890}