Re-land r93365 - add RefCountedString

Re-land r93365 - add RefCountedString

Added RefCountedString, as this is what many RefCountedMemory users seem to want
Made data member of RefCountedBytes private, as per style guide
Changed base64 APIs to accept StringPiece, as it's sometimes better and never worse than string.

Fix chromeos build; additional files updated comapred to previous patch:
chrome/browser/ui/login/login_prompt_ui.cc
chrome/browser/ui/webui/active_downloads_ui.cc
chrome/browser/ui/webui/chromeos/choose_mobile_network_ui.cc
chrome/browser/ui/webui/chromeos/enterprise_enrollment_ui.cc
chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc
chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc
chrome/browser/ui/webui/chromeos/login/login_ui.cc
chrome/browser/ui/webui/chromeos/login/login_ui_helpers.cc
chrome/browser/ui/webui/chromeos/login/login_ui_helpers.h
chrome/browser/ui/webui/chromeos/login/mock_login_ui_helpers.h
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc
chrome/browser/ui/webui/chromeos/register_page_ui.cc
chrome/browser/ui/webui/chromeos/sim_unlock_ui.cc
chrome/browser/ui/webui/chromeos/system_info_ui.cc
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc



BUG=None
TEST=All existing tests pass. Extended ref_counted_memory_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93617

[joth, 2011-07-19 14:51:01]

[many cc: watches left in, by popular request]

Elliot, Brett, could you take a first look at the base/.. changes (maybe using a
random sample of other files to understand the motivation) and if you agree with
this I'll then do the rounds getting LGTMs from all those other subsystems too.
(Or, I could contrive to leave |data| public for now and do that in a followup)


Briefly: whilst doing my other CL I noticed the RefCountedBytes::data member
should not be public. While poking around with that, I found tens of instances
of the following not-so-nice pattern:

  std::string response = ....
  // Send the response.
  scoped_refptr<RefCountedBytes> html_bytes(new RefCountedBytes);
  html_bytes->data.resize(response.size());  // clear mem
  std::copy(response.begin(), response.end(),
            html_bytes->data.begin());       // copy mem
  SendResponse(request_id, html_bytes);  

That code does a spurious memory clear and copy; this patch allows us to send
the |response| string direct:
  std::string response = ....
  SendResponse(request_id, base::RefCountedString::TakeString(&response));

(And is easier to read, to boot)

Thanks!

[Ami GONE FROM CHROMIUM, 2011-07-19 16:58:08]

> (And is easier to read, to boot)

Drive-by: would it be even easier to read if SendResponse had an overload that
took a std::string* and did the swap to a refcounted object internally?

[joth, 2011-07-19 17:18:08]

On 19 July 2011 17:58, <fischman@chromium.org> wrote:

>  (And is easier to read, to boot)
>>
>
> Drive-by: would it be even easier to read if SendResponse had an overload
> that
> took a std::string* and did the swap to a refcounted object internally?
>
> SendResponse is a virtual base-class method, overloading it feels unkind on
any child classes. I guess to keep the naming convention I would need it to
be something like SendResponseTakeString anyway - but that seems a mouthful
and even then having it mutate one of its arguments might be harder to
understand than just having two function calls for the two concepts.
Plus: based on what I've seen here, I'd bet beer that with time someone will
copy some code using the string*, and then hand-write a for loop to copy
their vector<uint8> over into a new string in order to send it. (Not that
not having it is a guarantee against this...)





..Your question has highlighted a completely independent issue to me:
SendResponse isn't guaranteed to call AddRef/Release on |bytes|, so it could
get leaked if the caller also doesn't do this.  not going to complicate this
patch by fixing that as well though.



>
>
http://codereview.chromium.**org/7397021/<http://codereview.chromium.org/7397...
>

[Elliot Glaysher, 2011-07-19 17:59:02]

On 2011/07/19 14:51:01, joth wrote:
>   std::string response = ....
>   SendResponse(request_id, base::RefCountedString::TakeString(&response));

The idea of removing the intermediate copies is good. Note the comment about the
one instance where things could be even better.

> (Or, I could contrive to leave |data| public for now and do that in a
followup)

I don't really think this matters either way.

http://codereview.chromium.org/7397021/diff/5001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/options_ui.cc (right):

http://codereview.chromium.org/7397021/diff/5001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/options_ui.cc:131:
IDR_OPTIONS_HTML).CopyToString(&response);
Both versions here are wrong.

Data that comes from the ResourceBundle should be put directly into
RefCountedStaticMemory through ResourceBundle::LoadDataResourceBytes. This is
the reason I made the RefCountedMemory system in the first place.

The idea is that memory that has been mmapped shouldn't be copied at all and
isn't owned by anyone.

[joth, 2011-07-19 19:25:22]

On 19 July 2011 18:59, <erg@chromium.org> wrote:

> On 2011/07/19 14:51:01, joth wrote:
>
>>   std::string response = ....
>>   SendResponse(request_id, base::RefCountedString::**
>> TakeString(&response));
>>
>
> The idea of removing the intermediate copies is good. Note the comment
> about the
> one instance where things could be even better.
>
>
>  (Or, I could contrive to leave |data| public for now and do that in a
>>
> followup)
>
> I don't really think this matters either way.
>
>
>
> http://codereview.chromium.**org/7397021/diff/5001/chrome/**
>
browser/ui/webui/options/**options_ui.cc<http://codereview.chromium.org/7397021/diff/5001/chrome/browser/ui/webui/options/options_ui.cc>
> File chrome/browser/ui/webui/**options/options_ui.cc (right):
>
> http://codereview.chromium.**org/7397021/diff/5001/chrome/**
>
browser/ui/webui/options/**options_ui.cc#newcode131<http://codereview.chromium.org/7397021/diff/5001/chrome/browser/ui/webui/options/options_ui.cc#newcode131>
> chrome/browser/ui/webui/**options/options_ui.cc:131:
> IDR_OPTIONS_HTML).**CopyToString(&response);
> Both versions here are wrong.
>
> Data that comes from the ResourceBundle should be put directly into
> RefCountedStaticMemory through ResourceBundle::**LoadDataResourceBytes.
> This is the reason I made the RefCountedMemory system in the first
> place.
>
> Ah yes, I see. Done, thanks.


> The idea is that memory that has been mmapped shouldn't be copied at all
> and isn't owned by anyone.
>
>
http://codereview.chromium.**org/7397021/<http://codereview.chromium.org/7397...
>

[Elliot Glaysher, 2011-07-19 19:30:10]

LGTM

[brettw, 2011-07-19 19:58:44]

http://codereview.chromium.org/7397021/diff/5001/base/memory/ref_counted_memo...
File base/memory/ref_counted_memory.h (right):

http://codereview.chromium.org/7397021/diff/5001/base/memory/ref_counted_memo...
base/memory/ref_counted_memory.h:103: std::string* data() { return &data_; }
Is there any reason not to return this as a ref? I guess I would have done:
  const std::string& data() const { return data_; }
  std::string& data() { return data_; }
Maybe the same for RefCountedBytes, which would clean up some of the callers?
What do you think?

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
File base/memory/ref_counted_memory.cc (right):

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
base/memory/ref_counted_memory.cc:74: } //  namespace base
Need an extra space before // (2 total).

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
File base/memory/ref_counted_memory.h (right):

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
base/memory/ref_counted_memory.h:114: }  // namespace
"namespace base"

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
File base/memory/ref_counted_memory_unittest.cc (right):

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
base/memory/ref_counted_memory_unittest.cc:45: } //  namespace base
Need extra space before //

[joth, 2011-07-20 12:53:02]

Thanks Brett, comments addressed.

I'm now looking for OWNER approvals (and drive-by reviews from those on cc)

brettw: base, content/renderer
pkasting: chrome/browser/ui (although that's not set noparent, so technically
erg's lgtm already covered it)
erg: are you OK to speak for the rest of chrome/browser, aside from ui? This has
no OWNERS.


Many thanks
Jonathan

http://codereview.chromium.org/7397021/diff/5001/base/memory/ref_counted_memo...
File base/memory/ref_counted_memory.h (right):

http://codereview.chromium.org/7397021/diff/5001/base/memory/ref_counted_memo...
base/memory/ref_counted_memory.h:103: std::string* data() { return &data_; }
On 2011/07/19 19:58:45, brettw wrote:
> Is there any reason not to return this as a ref? I guess I would have done:
>   const std::string& data() const { return data_; }
>   std::string& data() { return data_; }
> Maybe the same for RefCountedBytes, which would clean up some of the callers?
> What do you think?

I was overpowered by the "references must be const" rule.
Done. This does mean I'm now sometimes taking the address of the returned ref
which is a little odd in my eye, but technically fine.

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
File base/memory/ref_counted_memory.cc (right):

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
base/memory/ref_counted_memory.cc:74: } //  namespace base
On 2011/07/19 19:58:45, brettw wrote:
> Need an extra space before // (2 total).

Done.

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
File base/memory/ref_counted_memory.h (right):

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
base/memory/ref_counted_memory.h:114: }  // namespace
On 2011/07/19 19:58:45, brettw wrote:
> "namespace base"

Done.

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
File base/memory/ref_counted_memory_unittest.cc (right):

http://codereview.chromium.org/7397021/diff/11001/base/memory/ref_counted_mem...
base/memory/ref_counted_memory_unittest.cc:45: } //  namespace base
On 2011/07/19 19:58:45, brettw wrote:
> Need extra space before //

Done.

[brettw, 2011-07-20 17:10:22]

base/content LGTM

[joth, 2011-07-20 17:52:18]

+pkasting (forgot to add to reviewer list on previous mail).
Could you check the chrome/browser/ui files, or delegate to another OWNER?

TIA!

[Peter Kasting, 2011-07-20 17:57:39]

On 2011/07/20 17:52:18, joth wrote:
> +pkasting (forgot to add to reviewer list on previous mail).
> Could you check the chrome/browser/ui files, or delegate to another OWNER?

All your changes in chrome/browser/ui are in webui/.  Therefore I suggest one of
the owners in webui/OWNERS, either arv or estade.

[joth, 2011-07-20 18:42:01]

estade: could you rubber stamp the chrome/browser/ui/webui/.. changes? Thanks!

[Evan Stade, 2011-07-20 18:51:25]

rs lgtm

[joth, 2011-07-21 11:47:23]

Updated patch with ChromeOS files (as mentioned in the CL description) in order
to re-land.

rjkroege - would you mind doing a quick sanity check on these ChromeOS changes?
In particular, I removed CreateHTMLBytes from HTMLOperationsInterface as it
seems unnecessary to mock out RefCountedString creation. (and besides,
mock_login_ui_helpers.h seems completely unused anyway, afaict. Can that file be
deleted? And its abstract base class?)

Cheers!

[commit-bot: I haz the power, 2011-07-21 13:14:36]

Try job failure for 7397021-20003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[bulach, 2011-07-22 11:16:53]

LGTM

two nits below, hopefully the try bots will be happier next cycle..

http://codereview.chromium.org/7397021/diff/26001/base/memory/ref_counted_mem...
File base/memory/ref_counted_memory.h (right):

http://codereview.chromium.org/7397021/diff/26001/base/memory/ref_counted_mem...
base/memory/ref_counted_memory.h:48: virtual const unsigned char* front() const;
nit: OVERRIDE here and below

http://codereview.chromium.org/7397021/diff/26001/chrome/browser/ui/webui/tex...
File chrome/browser/ui/webui/textfields_ui.cc (right):

http://codereview.chromium.org/7397021/diff/26001/chrome/browser/ui/webui/tex...
chrome/browser/ui/webui/textfields_ui.cc:33:
.GetRawDataResource(IDR_TEXTFIELDS_HTML).as_string();
use LoadDataResourceBytes as options_ui.cc instead..