Changed RefCountedStaticMemory() to accept a void pointer.

Changed RefCountedStaticMemory() to accept a void pointer.

Using a void pointer saves lots of redundant casting.

Also added RefCountedMemory front_as<type> which saves typing by not having to
wrap each call with a reinterpret_cast<>.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250808

[João Eiras, 2014-01-07 16:58:10]

This review just adds some bits to RefCountedMemory to make it less verbose to
use.

[brettw, 2014-01-13 21:52:16]

Sorry for the delay, I wrote a comment but forgot to push send.

https://codereview.chromium.org/126103003/diff/1/base/memory/ref_counted_memo...
File base/memory/ref_counted_memory.h (right):

https://codereview.chromium.org/126103003/diff/1/base/memory/ref_counted_memo...
base/memory/ref_counted_memory.h:34: template<typename T> const T* front_as()
const
Style nit: if everything doesn't fit on one line, use the "regular" multiline
function call formatting.

https://codereview.chromium.org/126103003/diff/1/base/memory/ref_counted_memo...
base/memory/ref_counted_memory.h:49: RefCountedStaticMemory(const void* data,
size_t length)
I feel like void is right here. But then it seems weird that we expose that as a
char* in the interface and store it as that. I feel like this should be void*
everywhere, with the templatized getter if you want it as some format.

I'd suggest the API should bet:

virtual const void* Get() const;
template<typename T> const T* GetAs() const;
virtual size_t GetSize() const;

Since the current function names violate the style guide's naming for virtual
functions (should be CamelCase since they're not inline).

There are 100-something files that use this, but it should be super
search-and-replaceable. I was just changing hundreds of files for some base
naming stuff, it's not actually that bad.

We could do two passes, one to fix the API, the other to add this new stuff?

[João Eiras, 2014-01-15 15:48:41]

On 2014/01/13 21:52:16, brettw wrote:
> Sorry for the delay, I wrote a comment but forgot to push send.
> 
>
https://codereview.chromium.org/126103003/diff/1/base/memory/ref_counted_memo...
> File base/memory/ref_counted_memory.h (right):
> 
>
https://codereview.chromium.org/126103003/diff/1/base/memory/ref_counted_memo...
> base/memory/ref_counted_memory.h:34: template<typename T> const T* front_as()
> const
> Style nit: if everything doesn't fit on one line, use the "regular" multiline
> function call formatting.
> 
>
https://codereview.chromium.org/126103003/diff/1/base/memory/ref_counted_memo...
> base/memory/ref_counted_memory.h:49: RefCountedStaticMemory(const void* data,
> size_t length)
> I feel like void is right here. But then it seems weird that we expose that as
a
> char* in the interface and store it as that. I feel like this should be void*
> everywhere, with the templatized getter if you want it as some format.
> 
> I'd suggest the API should bet:
> 
> virtual const void* Get() const;
> template<typename T> const T* GetAs() const;
> virtual size_t GetSize() const;
> 
> Since the current function names violate the style guide's naming for virtual
> functions (should be CamelCase since they're not inline).
> 
> There are 100-something files that use this, but it should be super
> search-and-replaceable. I was just changing hundreds of files for some base
> naming stuff, it's not actually that bad.
> 
> We could do two passes, one to fix the API, the other to add this new stuff?

After looking at all the references to front() they either expect a 'unsigned
char', 'char' or in very few cases pass to memcpy ('void'). 'unsigned char' is
the most common. I counted 72, roughly.
So there would be a big rewrite effort with little benefit, and which would make
the calls for verbose (use FrontAs<unsigned char>). The de-facto standard seems
to be to use unsigned char whenever people want a raw buffer. Most stl
containers have begin/end and front/back methods, so changing from "front" to
"Get" here is not that beneficial. I also see many call sites that do (front(),,
front()+size()) to get the end of the container, and that would expect a
single-byte type for the buffer.

Replacing the API is tricky because they are many downstream projects which I
cannot influence. We'd need to add a new API, deprecate the old one, and wait a
bit. Again, for little benefit. 

I can change front_as to FrontAs at least.

Do you *really* want me to do that  ? (then who would review all the code ? :p
my reviews tend to get a bit stuck).

[brettw, 2014-01-26 06:44:12]

Sorry for the delay, I was stuck trying to land the build change.

[brettw, 2014-01-26 06:50:06]

I'm no longer as passionate about changing the API as I was before (see? you
benefitted from my horrendous review latency!). I was going to say we should
call the new one FrontAs to at least not make the style voilations worse, but
the asymmetry seems too weird.

So I'm OK with this now, can you fix the formatting nits in the previous
message?

https://codereview.chromium.org/126103003/diff/40001/base/memory/ref_counted_...
File base/memory/ref_counted_memory.h (right):

https://codereview.chromium.org/126103003/diff/40001/base/memory/ref_counted_...
base/memory/ref_counted_memory.h:34: template<typename T> const T* front_as()
const
Comment about {} placement and wrapping here still stands.

[João Eiras, 2014-02-11 15:28:04]

On 2014/01/26 06:50:06, brettw wrote:
> I'm no longer as passionate about changing the API as I was before (see? you
> benefitted from my horrendous review latency!). I was going to say we should
> call the new one FrontAs to at least not make the style voilations worse, but
> the asymmetry seems too weird.
> 
> So I'm OK with this now, can you fix the formatting nits in the previous
> message?
> 
>
https://codereview.chromium.org/126103003/diff/40001/base/memory/ref_counted_...
> File base/memory/ref_counted_memory.h (right):
> 
>
https://codereview.chromium.org/126103003/diff/40001/base/memory/ref_counted_...
> base/memory/ref_counted_memory.h:34: template<typename T> const T* front_as()
> const
> Comment about {} placement and wrapping here still stands.

Ping.

The formatting is fixed. Also rebased, and changed more call sites. How does it
look ?

Thanks.

[brettw, 2014-02-11 19:17:54]

Thanks! LGTM

[João Eiras, 2014-02-11 20:34:21]

Could you please help review the leftover files (everything not in base).

Thank you.

[sky, 2014-02-11 22:15:22]

LGTM

[João Eiras, 2014-02-12 11:00:07]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-02-12 11:01:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaoe@opera.com/126103003/200001

[commit-bot: I haz the power, 2014-02-12 11:24:17]

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

[commit-bot: I haz the power, 2014-02-12 11:24:19]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Ted C, 2014-02-12 17:40:42]

lgtm - web_contents_delegate_android.cc

[João Eiras, 2014-02-12 18:12:32]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-02-12 18:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaoe@opera.com/126103003/200001

[commit-bot: I haz the power, 2014-02-12 22:34:10]

Change committed as 250808