Make image_decoder a Leaky LazyInstance

image_decoder has been refactored into a Leaky (accessible from multiple threads) LazyInstance (singleton), that uses one utility process running in batch mode to decode all images.

This CL fixes a performance problem with scrolling in bookmarks on Android.
Previously we were using a new image_decoder and therefore a
new utility process for each image we wanted to decode. Now we
use one shared image_decoder that has one utility process running in batch mode for decoding all images.

BUG=451201
Committed: https://crrev.com/a71414d3412695691b873508377ae67854062fd4
Cr-Commit-Position: refs/heads/master@{#322383}

[Theresa, 2015-02-17 23:38:23]

twellington@chromium.org changed reviewers:
+ dcheng@chromium.org, kkimlabs@chromium.org, thakis@chromium.org

[Theresa, 2015-02-17 23:38:24]

kkimlabs@ - for enhanced_bookmarks/overall review
thakis@ - for changes in chrome/browser and chrome/browser/bitmap_fetcher
dcheng@ - for chrome/utility changes

[Kibeom Kim (inactive), 2015-02-18 16:53:27]

I'm not familiar with utility process and have a couple of high-level questions:

- Why we don't share just one utility process for everything? Is that to isolate
security context?
- Is it OK to keep a utility process alive? For example, in this case to my
understanding, BookmarkImageServiceAndroid has BitmapBatchFetcher, and
BitmapBatchFetcher has ImageBatchDecoder so the utility process won't be killed
as long as Chrome is running.

[dcheng, 2015-02-18 17:43:34]

Apparently, UtilityProcessHost has something called "batch mode" (see
UtilityProcessHost::StartBatchMode and UtilityProcessHost::EndBatchMode). Maybe
we can use that?

[dcheng, 2015-02-18 17:43:49]

(And I should have lookde at the CL first. Ignore me!)

[Theresa, 2015-02-18 18:05:04]

Utility processes can be associated with a UtilityProcessHostClient. If
supplied, the client will be notified of incoming messages from the utility
process. In this case, image_batch_decoder is the client, so it will receive IPC
messages from the utility process. I think facilitating different clients is one
reason we don't share just one utility process for everything. Isolating the
context may be another reason (since the utility processes run as sandboxed
processes).

WRT to "is it okay to keep a utility process alive"; everything seems to be
working just fine with the process running but the class documentation does say
"short lived". Maybe it would be better to use a timer and call EndBatchMode()
if a certain # of milliseconds have passed without any requests for images?

[Kibeom Kim (inactive), 2015-02-18 18:23:19]

kkimlabs@chromium.org changed reviewers:
+ jam@chromium.org

[Kibeom Kim (inactive), 2015-02-18 18:23:19]

+jam@ for UtilityProcessHost usage.

[Nico, 2015-02-18 20:59:17]

https://codereview.chromium.org/931993002/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher.h (right):

https://codereview.chromium.org/931993002/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher.h:27: // BitmapBatchFetcher
instead.
To me, it seems like a better strategy to me to swap out the implementation of
this class transparently instead of changing all clients to use the new class –
they should be mostly interface-compatible right? That's what abstractions are
for, after all.

Maybe the implementation could pick one of the two behaviors based on a flag,
then you can roll this out behind a flag and don't need a new class at all – and
once you're happy with how the flag performs, you can turn it on by default and
rip out the old code.

[jam, 2015-02-18 21:21:22]

On 2015/02/18 18:05:04, twellington wrote:
> Utility processes can be associated with a UtilityProcessHostClient. If
> supplied, the client will be notified of incoming messages from the utility
> process. In this case, image_batch_decoder is the client, so it will receive
IPC
> messages from the utility process. I think facilitating different clients is
one
> reason we don't share just one utility process for everything. Isolating the
> context may be another reason (since the utility processes run as sandboxed
> processes).
> 
> WRT to "is it okay to keep a utility process alive"; everything seems to be
> working just fine with the process running but the class documentation does
say
> "short lived". Maybe it would be better to use a timer and call EndBatchMode()
> if a certain # of milliseconds have passed without any requests for images?

correct, you'll want to tear down the utility process after you're done with it.

do you know ahead of time how many images there are to decode?

[Theresa, 2015-02-18 21:26:34]

On 2015/02/18 20:59:17, Nico wrote:
>
https://codereview.chromium.org/931993002/diff/60001/chrome/browser/bitmap_fe...
> File chrome/browser/bitmap_fetcher/bitmap_fetcher.h (right):
> 
>
https://codereview.chromium.org/931993002/diff/60001/chrome/browser/bitmap_fe...
> chrome/browser/bitmap_fetcher/bitmap_fetcher.h:27: // BitmapBatchFetcher
> instead.
> To me, it seems like a better strategy to me to swap out the implementation of
> this class transparently instead of changing all clients to use the new class
–
> they should be mostly interface-compatible right? That's what abstractions are
> for, after all.
> 
> Maybe the implementation could pick one of the two behaviors based on a flag,
> then you can roll this out behind a flag and don't need a new class at all
– and
> once you're happy with how the flag performs, you can turn it on by default
and
> rip out the old code.

BitmapBatchFetcher and ImageBatchDecoder have different constructor signatures
and ::Start() signatures than BitmapFetcher and ImageDecoder. I can certainly
just swap out the implementation rather than introducing new classes, but all of
the dependent classes will have to be changed. I'm not opposed, it just touches
a lot of classes (~20 based on a quick code search).

Another option is to merge the batch version and regular version and keep the
old constructor/start methods around until all of the dependent classes are
changed.

[Theresa, 2015-02-18 21:33:36]

On 2015/02/18 21:21:22, jam wrote:
> On 2015/02/18 18:05:04, twellington wrote:
> > Utility processes can be associated with a UtilityProcessHostClient. If
> > supplied, the client will be notified of incoming messages from the utility
> > process. In this case, image_batch_decoder is the client, so it will receive
> IPC
> > messages from the utility process. I think facilitating different clients is
> one
> > reason we don't share just one utility process for everything. Isolating the
> > context may be another reason (since the utility processes run as sandboxed
> > processes).
> > 
> > WRT to "is it okay to keep a utility process alive"; everything seems to be
> > working just fine with the process running but the class documentation does
> say
> > "short lived". Maybe it would be better to use a timer and call
EndBatchMode()
> > if a certain # of milliseconds have passed without any requests for images?
> 
> correct, you'll want to tear down the utility process after you're done with
it.
> 
> do you know ahead of time how many images there are to decode?

The images are decoded on-demand, as the user scrolls through
chrome:://bookmarks. If the user launches Chrome and never goes to bookmarks, no
images are decoded. If they launch Chrome, have 5000 bookmarks, and scroll all
the way to the end, we'll decode 5000 images.

How this CL is written right now, the utility process for decoding bookmark
images just hangs around the entire time Chrome is running. Given the on-demand
nature of image decoding, we can certainly do better. We just want to reuse a
utility process in batch mode because if the user is scrolling quickly we
request a lot of images and spawning a new process for each doesn't perform
well.

[Theresa, 2015-02-20 01:33:43]

ping, jam@ - can you please advice on how "short lived" utility processes should
be?

[Nico, 2015-02-20 17:04:11]

On 2015/02/20 01:33:43, twellington wrote:
> ping, jam@ - can you please advice on how "short lived" utility processes
should
> be?

Extension processes hang around for 10 seconds after last use (
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
) – something in that area (10-30s) seems right for an image decoding utility
process to me too.

[Theresa, 2015-02-20 21:14:15]

On 2015/02/20 17:04:11, Nico wrote:
> On 2015/02/20 01:33:43, twellington wrote:
> > ping, jam@ - can you please advice on how "short lived" utility processes
> should
> > be?
> 
> Extension processes hang around for 10 seconds after last use (
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> ) – something in that area (10-30s) seems right for an image decoding
utility
> process to me too.

Thank you :)

My plan going forward is: 
- To make the utility process more short lived: don't start the utility process
until we get the first decode image request; start a short timeout (e.g. 1000ms)
and shutdown the utility process if no more requests come in before it runs out
(restart utility process if another request comes in, etc.)
- To merge bitmap_fetcher/bitmap_batch_fetcher and
image_fetcher/image_batch_fetcher: My impression is most of the classes using
the image_fetcher/bitmap_fetcher are only sending one request (or other areas
would be hitting the same performance problems we did). Rather than maintaining
both classes, one class could be used to facilitate both a batch mode and a
single use mode; existing clients that are only fetching one image will use the
single use mode. There may be some small refactoring to the clients classes to
change constructor/destructor args, but that should be about it.

[Nico, 2015-02-20 21:16:00]

On 2015/02/20 21:14:15, twellington wrote:
> On 2015/02/20 17:04:11, Nico wrote:
> > On 2015/02/20 01:33:43, twellington wrote:
> > > ping, jam@ - can you please advice on how "short lived" utility processes
> > should
> > > be?
> > 
> > Extension processes hang around for 10 seconds after last use (
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > ) – something in that area (10-30s) seems right for an image decoding
> utility
> > process to me too.
> 
> Thank you :)
> 
> My plan going forward is: 
> - To make the utility process more short lived: don't start the utility
process
> until we get the first decode image request; start a short timeout (e.g.
1000ms)
> and shutdown the utility process if no more requests come in before it runs
out
> (restart utility process if another request comes in, etc.)
> - To merge bitmap_fetcher/bitmap_batch_fetcher and
> image_fetcher/image_batch_fetcher: My impression is most of the classes using
> the image_fetcher/bitmap_fetcher are only sending one request (or other areas
> would be hitting the same performance problems we did). Rather than
maintaining
> both classes, one class could be used to facilitate both a batch mode and a
> single use mode; existing clients that are only fetching one image will use
the
> single use mode. There may be some small refactoring to the clients classes to
> change constructor/destructor args, but that should be about it.

What's the difference between single use and batch mode? Is it just if you keep
the utility process around for a bit? If so, couldn't you just do this
unconditionally?

[jam, 2015-02-20 21:30:15]

On 2015/02/20 21:14:15, twellington wrote:
> On 2015/02/20 17:04:11, Nico wrote:
> > On 2015/02/20 01:33:43, twellington wrote:
> > > ping, jam@ - can you please advice on how "short lived" utility processes
> > should
> > > be?
> > 
> > Extension processes hang around for 10 seconds after last use (
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > ) – something in that area (10-30s) seems right for an image decoding
> utility
> > process to me too.
> 
> Thank you :)
> 
> My plan going forward is: 
> - To make the utility process more short lived: don't start the utility
process
> until we get the first decode image request; start a short timeout (e.g.
1000ms)
> and shutdown the utility process if no more requests come in before it runs
out
> (restart utility process if another request comes in, etc.)

sgtm.

I don't care about the precise value as much as it exists :)

> - To merge bitmap_fetcher/bitmap_batch_fetcher and
> image_fetcher/image_batch_fetcher: My impression is most of the classes using
> the image_fetcher/bitmap_fetcher are only sending one request (or other areas
> would be hitting the same performance problems we did). Rather than
maintaining
> both classes, one class could be used to facilitate both a batch mode and a
> single use mode; existing clients that are only fetching one image will use
the
> single use mode. There may be some small refactoring to the clients classes to
> change constructor/destructor args, but that should be about it.

[Peter Kasting, 2015-02-23 22:03:33]

* Are there other places in our codebase using utility processes besides the
bitmap fetcher?  If so can we try to make all such places share a single process
so that we have at most one utility process at any given time?

* I echo Nico's question about what the difference between batch and single-use
mode is.  For example, what if while one class is batching 10 image requests,
another three classes each simultaneously make one request?  It seems like this
should result in one utility process with 13 total queued requests.  In general
I wonder if "batching" can be done transparently under the hood and be something
callers never need to worry about.  Then the image fetcher only has one type of
API, and that API is implemented to be maximally efficient.

[Theresa, 2015-02-23 22:32:18]

On 2015/02/23 22:03:33, Peter Kasting wrote:
> * Are there other places in our codebase using utility processes besides the
> bitmap fetcher?  If so can we try to make all such places share a single
process
> so that we have at most one utility process at any given time?

Yes, there are may other places we use utility_process_host and
utility_process_host_client. If a client is supplied to the UtilityProcessHost,
it will be notified of incoming messages from the utility process. The different
clients listen for different messages, so I don't think it makes sense to share
a single process for everything.

> * I echo Nico's question about what the difference between batch and
single-use
> mode is.  For example, what if while one class is batching 10 image requests,
> another three classes each simultaneously make one request?  It seems like
this
> should result in one utility process with 13 total queued requests.  In
general
> I wonder if "batching" can be done transparently under the hood and be
something
> callers never need to worry about.  Then the image fetcher only has one type
of
> API, and that API is implemented to be maximally efficient.

I haven't seen instances in our code base of multiple classes sharing a image
decoder (or a single utility process), so that scenario is a bit odd. Client
classes aren't responsible for batching.

A utility process that's not running in batch mode will create a new process
when it gets a call to UtilityProcessHostImpl::Send(). It is expected that this
method is only invoked once. When I was playing around with this code, IRC, if
the utility process isn't running in batch mode, multiple calls to Send() causes
the app to crash. A utility process running in batch mode creates the new
process when ::StartBatchMode() is called. The new process runs until
::StopBatchMode() is called. Send() may be invoked in between these two calls as
many times as you want.

In my current implementation for the image_decoder, if it's running in batch
mode, it will start the utility process in batch mode when the first
ImageDecoder::Start call is made. If no more Start requests have been made after
5 seconds, utility_process_host->StopBatchMode() is called; if other requests
have come in, the 5 second timeout is restarted. If another request comes in
after the utility process has been stopped, a new utility process is created and
the cycle starts again.

If the image decoder is not running in batch mode, the utility process only
hangs around for one invocation of ImageDecoder::Start (which is a few
milliseconds vs. 5+ seconds).

The API changes to image_decoder and bitmap_fetcher arguments from the
constructor to the Start methods of these classes. I provided a default
constructor arg for is_batch_mode = false, so that existing clients that have
been chugging along happily in the one-time-use mode have very minimal changes.

[Peter Kasting, 2015-02-23 22:52:06]

On 2015/02/23 22:32:18, twellington wrote:
> On 2015/02/23 22:03:33, Peter Kasting wrote:
> > * Are there other places in our codebase using utility processes besides the
> > bitmap fetcher?  If so can we try to make all such places share a single
> process
> > so that we have at most one utility process at any given time?
> 
> Yes, there are may other places we use utility_process_host and
> utility_process_host_client. If a client is supplied to the
UtilityProcessHost,
> it will be notified of incoming messages from the utility process. The
different
> clients listen for different messages, so I don't think it makes sense to
share
> a single process for everything.

Why does listening for different messages mean it doesn't make sense to share a
utility process?

This simply means the utility process will have different types of tasks to
process.  That's not an unusual thing.

One of the big performance problems we have in Chrome today is creation of too
many processes, so I'm very keen to reduce that.

> > * I echo Nico's question about what the difference between batch and
> single-use
> > mode is.  For example, what if while one class is batching 10 image
requests,
> > another three classes each simultaneously make one request?  It seems like
> this
> > should result in one utility process with 13 total queued requests.  In
> general
> > I wonder if "batching" can be done transparently under the hood and be
> something
> > callers never need to worry about.  Then the image fetcher only has one type
> of
> > API, and that API is implemented to be maximally efficient.
> 
> I haven't seen instances in our code base of multiple classes sharing a image
> decoder (or a single utility process), so that scenario is a bit odd. Client
> classes aren't responsible for batching.

The AiS code (currently being implemented) could in theory have several
different instances of a particular class each creating a single bitmap fetcher.

What I'm proposing is that all calls to fetch bitmaps, from any source, all get
sent to the same utility process.  Your CL implements manual batching from just
one source, which is an improvement over not batching at all, but still relies
on clients to say whether they want batch mode and doesn't address potential
problems from many separate clients wanting to simultaneously fetch images.

I don't think clients should ever say whether they want batch mode or not.  We
should always run in batch mode, and there should be zero or one utility
processes at any given time dedicated to decoding images -- if any client
requests an image while the process exists, that request is sent to that
process, otherwise the process is created.

This still doesn't accomplish my stated goal of making all utility process uses
of all types share a single process but it at least gets part way there.

[Theresa, 2015-02-23 23:38:29]

On 2015/02/23 22:52:06, Peter Kasting wrote:
> On 2015/02/23 22:32:18, twellington wrote:
> > On 2015/02/23 22:03:33, Peter Kasting wrote:
> > > * Are there other places in our codebase using utility processes besides
the
> > > bitmap fetcher?  If so can we try to make all such places share a single
> > process
> > > so that we have at most one utility process at any given time?
> > 
> > Yes, there are may other places we use utility_process_host and
> > utility_process_host_client. If a client is supplied to the
> UtilityProcessHost,
> > it will be notified of incoming messages from the utility process. The
> different
> > clients listen for different messages, so I don't think it makes sense to
> share
> > a single process for everything.
> 
> Why does listening for different messages mean it doesn't make sense to share
a
> utility process?
> 
> This simply means the utility process will have different types of tasks to
> process.  That's not an unusual thing.
> 
> One of the big performance problems we have in Chrome today is creation of too
> many processes, so I'm very keen to reduce that.
> 
> > > * I echo Nico's question about what the difference between batch and
> > single-use
> > > mode is.  For example, what if while one class is batching 10 image
> requests,
> > > another three classes each simultaneously make one request?  It seems like
> > this
> > > should result in one utility process with 13 total queued requests.  In
> > general
> > > I wonder if "batching" can be done transparently under the hood and be
> > something
> > > callers never need to worry about.  Then the image fetcher only has one
type
> > of
> > > API, and that API is implemented to be maximally efficient.
> > 
> > I haven't seen instances in our code base of multiple classes sharing a
image
> > decoder (or a single utility process), so that scenario is a bit odd. Client
> > classes aren't responsible for batching.
> 
> The AiS code (currently being implemented) could in theory have several
> different instances of a particular class each creating a single bitmap
fetcher.
> 
> What I'm proposing is that all calls to fetch bitmaps, from any source, all
get
> sent to the same utility process.  Your CL implements manual batching from
just
> one source, which is an improvement over not batching at all, but still relies
> on clients to say whether they want batch mode and doesn't address potential
> problems from many separate clients wanting to simultaneously fetch images.
> 
> I don't think clients should ever say whether they want batch mode or not.  We
> should always run in batch mode, and there should be zero or one utility
> processes at any given time dedicated to decoding images -- if any client
> requests an image while the process exists, that request is sent to that
> process, otherwise the process is created.
> 
> This still doesn't accomplish my stated goal of making all utility process
uses
> of all types share a single process but it at least gets part way there.

I'll work on making the bitmap fetcher a singleton.

I think if we want to pursue all utility process sharing a single process, that
should be handled through a separate effort.

[Theresa, 2015-02-24 17:35:10]

I have the bitmap_fetcher refactored into a singleton - I used LazyInstance so
that it's not initialized until it's actually needed. Some of the other
properties of LazyInstance (e.g. can have multiple LazyInstance of the same
type) aren't needed, though.
Would it be better to use singleton.h in this case?

The latest CL (which still has some compile issue and hasn't been self-reviewed,
so isn't quite ready) is a step in the right direction. All classes that use
bitmap_fetcher share a single instance, which means they all share one image
decoder and ultimately one utility process running in batch mode for image
decoding.

The image_decoder itself isn't a singleton, yet, though. The image decoder posts
callbacks to the supplied task_runner, which in most cases is the UI thread's
MessageLoopProxy, but in some cases it's the message loop for the IO thread, the
blocking pool, a background task pool, or the current thread. In order to be a
singleton, the task runner needs to be the same for all of clients (maybe
background task pool?) or the image decoder could maintain an instance per
task_runner.

[Peter Kasting, 2015-02-25 06:42:18]

On 2015/02/24 17:35:10, twellington wrote:
> I have the bitmap_fetcher refactored into a singleton - I used LazyInstance so
> that it's not initialized until it's actually needed. Some of the other
> properties of LazyInstance (e.g. can have multiple LazyInstance of the same
> type) aren't needed, though.
> Would it be better to use singleton.h in this case?

I don't know these classes well enough to answer that, but it's a good question.
 Definitely find _someone_ (via git blame, posting to chromium-dev, etc.) who
can answer it :)

> The image_decoder itself isn't a singleton, yet, though. The image decoder
posts
> callbacks to the supplied task_runner, which in most cases is the UI thread's
> MessageLoopProxy, but in some cases it's the message loop for the IO thread,
the
> blocking pool, a background task pool, or the current thread. In order to be a
> singleton, the task runner needs to be the same for all of clients (maybe
> background task pool?) or the image decoder could maintain an instance per
> task_runner.

Can the image decoder take a task runner per decode request (e.g. to keep in a
map or something) so it can call back to different places?

Though perhaps it's possible to take existing consumers of this functionality
and make them all use the same task runner.  I haven't looked at them to know.

[Theresa, 2015-02-25 19:45:07]

On 2015/02/25 06:42:18, Peter Kasting wrote:
> I don't know these classes well enough to answer that, but it's a good
question.
>  Definitely find _someone_ (via git blame, posting to chromium-dev, etc.) who
> can answer it :)

I found a chromium-dev discussion with some guidelines preferring LazyInstance
over Singleton -
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/single...


> Can the image decoder take a task runner per decode request (e.g. to keep in a
> map or something) so it can call back to different places?
> 
> Though perhaps it's possible to take existing consumers of this functionality
> and make them all use the same task runner.  I haven't looked at them to know.


I think the image decoder could take a task runner per decode and keep it in a
map somewhere (associated with the delegate).

Currently image_decoder is constructing the utility_process_host using the
task_runner_ supplied to its constructor as the client_task_runner and this
(image_decoder) as the client. Instead, it could construct the utility process
host with the IO thread's message loop proxy as the task runner (I think IO
thread is best, since most of the image decoder messages run on the IO thread
and the IO thread manages IPC anyway).
ImageDecoder::OnDecodeImageSucceeded/Failed would then post tasks back to the
task_runner associated with the delegate.

[Peter Kasting, 2015-02-25 21:30:22]

On 2015/02/25 19:45:07, twellington wrote:
> Currently image_decoder is constructing the utility_process_host using the
> task_runner_ supplied to its constructor as the client_task_runner and this
> (image_decoder) as the client. Instead, it could construct the utility process
> host with the IO thread's message loop proxy as the task runner (I think IO
> thread is best, since most of the image decoder messages run on the IO thread
> and the IO thread manages IPC anyway).
> ImageDecoder::OnDecodeImageSucceeded/Failed would then post tasks back to the
> task_runner associated with the delegate.

That sounds like it could work.  It also suggests that maybe if the utility
process host itself took a task runner per request (instead of one in the
constructor), then it could keep the map instead of the image decoder, and could
call the relevant callbacks directly instead of taking an extra IO loop hop. 
(Minimizing the total number of messages on the IO thread would be nice to
reduce overhead.)

[Theresa, 2015-02-26 00:29:01]

On 2015/02/25 21:30:22, Peter Kasting wrote:
> That sounds like it could work.  It also suggests that maybe if the utility
> process host itself took a task runner per request (instead of one in the
> constructor), then it could keep the map instead of the image decoder, and
could
> call the relevant callbacks directly instead of taking an extra IO loop hop. 
> (Minimizing the total number of messages on the IO thread would be nice to
> reduce overhead.)

The "request" method for utility process host is "Send", which is an override
from IPC::Send. utility_host needs the client_task_runner_ in it's
OnProcessLaunchFailed and OnProcessCrashed methods, which may be called before
any calls to UtilityProcessHost::Send if the utility process is running in batch
mode (the process is created in ::StartBatchMode).

The methods that use client_task_runner_ are all overrides from
BrowserChildProcessHostDelegate. OnMessageReceived, in particular, is an
override from BrowserChildProcessHostDelegate's base IPC::Listener.

As the code is right now, I don't think there's a way to tell if
OnMessageReceived was in response to a particular Send request. The
utility_process_host_ just calls process_->Send, and any messages generated from
the process_ end up invoking UtilityProcessHost::OnMessageReceived. I think
maintaining a map of ::Send -> client_task_runner_'s in utility process host
would require changes to IPC::Send, IPC::OnMessageReceived, and
OnProcessLaunchFailed/OnProcessCrashed.

[Theresa, 2015-02-27 03:42:52]

PTAL
(... 18 patch sets later, sorry if I've been spamming you guys at all)

ImageDecoder is a singleton now. It's internals have been moved to an inline
ImageDecoder::ImageDecoderImpl. ImageDecoderImpl extends
UtilityProcessHostClient, which is ref counted. Since ImageDecoder is now one
single leaky lazy instance, it isn't ref counted.

Since ImageDecoder is a singleton, BitmapFetcher does not need to be a singleton
and changes to that class were reverted.

Most of the classes in this CL have very minor changes required to use
ImageDecoder as a singleton. UserImageLoader has some extra changes (and should
be reviewed a little more closely), and ChromeUtilityMessages was modified to
support a delegate id argument for DecodeImage messages.

[Peter Kasting, 2015-02-27 04:59:46]

On 2015/02/27 03:42:52, twellington wrote:
> PTAL
> (... 18 patch sets later, sorry if I've been spamming you guys at all)
> 
> ImageDecoder is a singleton now. It's internals have been moved to an inline
> ImageDecoder::ImageDecoderImpl. ImageDecoderImpl extends
> UtilityProcessHostClient, which is ref counted. Since ImageDecoder is now one
> single leaky lazy instance, it isn't ref counted.
> 
> Since ImageDecoder is a singleton, BitmapFetcher does not need to be a
singleton
> and changes to that class were reverted.
> 
> Most of the classes in this CL have very minor changes required to use
> ImageDecoder as a singleton. UserImageLoader has some extra changes (and
should
> be reviewed a little more closely), and ChromeUtilityMessages was modified to
> support a delegate id argument for DecodeImage messages.

Awesome.  Thanks very much for doing this work.  This will ensure that other
people don't accidentally cause the same kinds of problems that led to the
original CL here.

(Not reviewing since I'm not a good owner for anything here)

[dcheng, 2015-02-27 16:47:18]

Some high (and not so high)-level thoughts.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:69:
scoped_ptr<std::string> data(new std::string);
Do we still need to heap-allocate this?

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:92: ImageInfo
image_info = image_info_queue_.front();
const ImageInfo& to save a copy?

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:126:
image_data_queue_.front());
How about adding the original raw image as a member of ImageInfo, to avoid
maintaining two parallel queues?

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:120:
chromeutility::DUMMY_IMAGE_DELEGATE_ID));
Should this/can this be cleaned up to use ImageDecoder in the future? This
seems... unfortunate.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:27: image_decoder_impl_ = new
ImageDecoderImpl();
It might be better to put this in the initializer. Then you mark the member
const as well?

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:27: image_decoder_impl_ = new
ImageDecoderImpl();
It might be better to put this in the initializer. Then you can mark the member
const as well.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:44:
ImageDecoder::ImageDecoderImpl::ImageDecoderImpl(){
clang-format should catch these random missing spaces, though it still mangles
IPC message map macros, so you'll need to be a bit careful.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:58: if (!task_runner) {
This seems like a programming error that we should be DCHECK()ing on, rather
than trying to handle it gracefully somehow.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:59: if (delegate) {
IMO, |delegate| should probably be required too.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:80: delegate_map_[delegate_id_counter_] =
delegate;
I'd recommend using IDMap here. Also, it's probably better to just maintain one
map than two parallel maps.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:123: if ((time(0) - last_request_) <
batch_mode_timeout_seconds_) {
I think we can use base::DelayTimer here to simplify the code, instead of having
a lot of our own tracking logic.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:27: // |decoder| is used to identify the image in
case of decoding several
This comment needs to be updated.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:51: scoped_refptr<base::SequencedTaskRunner>
task_runner,
Just an aside, it's slightly better to pass a scoped_refptr<T> arg as const
scoped_refptr<T>&. That being said, the majority of Chrome code doesn't bother
(and we may or may not change guidance on the future, depending on whether or
not scoped_refptr becomes movable). This is more informational than anything, as
it's probably not worth changing given the uncertain state of affairs.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:52: bool shrink_to_fit = false);
The Google C++ style guide forbids default args =(

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:59: class ImageDecoderImpl : public
content::UtilityProcessHostClient {
I can't say I'm enthused about this delegate. I don't have any good alternatives
though.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:106: int delegate_id_counter_ = 0;
Warning: personal preference. I prefer not to use in-class initialization if
there's only one constructor. But it doesn't seem like a big deal.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:123: time_t last_request_ = 0;
This should probably be base::TimeTicks.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:127: const int batch_mode_timeout_seconds_ = 5;
kBatchModeTimeoutSeconds

https://codereview.chromium.org/931993002/diff/340001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:29: static const int
DUMMY_IMAGE_DELEGATE_ID = -999;
I think we should be looking into how to clean up the extensions code, rather
than adding a magic value for it.

[Theresa, 2015-03-04 03:10:07]

I'm still debugging a few test failures in chromeos, either fixed or responded
to all comments from jam@ review.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:69:
scoped_ptr<std::string> data(new std::string);
On 2015/02/27 16:47:17, dcheng wrote:
> Do we still need to heap-allocate this?

Yes, a bunch of browser tests start failing without the heap allocation.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:92: ImageInfo
image_info = image_info_queue_.front();
On 2015/02/27 16:47:17, dcheng wrote:
> const ImageInfo& to save a copy?

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:126:
image_data_queue_.front());
On 2015/02/27 16:47:17, dcheng wrote:
> How about adding the original raw image as a member of ImageInfo, to avoid
> maintaining two parallel queues?

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:120:
chromeutility::DUMMY_IMAGE_DELEGATE_ID));
On 2015/02/27 16:47:17, dcheng wrote:
> Should this/can this be cleaned up to use ImageDecoder in the future? This
> seems... unfortunate.

I think it should be, but it may be a little tricky. This class is still going
to need its utility_host to send ParseJSON and DecodeImageBase64 messages. I
think extensions_hander.cc handles DecodeImageBase64 messages. ExtensionsHandler
sends new ChromeUtilityHost messages for DecodeImage_Failed and
DecodeImageAndSend in OnDecodeImageBase64, so I think webstore_install_helper.cc
stills needs IPCHandler logic for OnDecodeImageFailed and
OnDecodeImageSucceeded. Maybe DecodeImageBase64 could be rolled into the
ImageDecoder?

I'm not familiar enough with the extensions code (or how to build it and test
it) to make the change, but I can certainly file a bug if that's all right.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:27: image_decoder_impl_ = new
ImageDecoderImpl();
On 2015/02/27 16:47:17, dcheng wrote:
> It might be better to put this in the initializer. Then you can mark the
member
> const as well.

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:27: image_decoder_impl_ = new
ImageDecoderImpl();
On 2015/02/27 16:47:17, dcheng wrote:
> It might be better to put this in the initializer. Then you mark the member
> const as well?

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:44:
ImageDecoder::ImageDecoderImpl::ImageDecoderImpl(){
On 2015/02/27 16:47:17, dcheng wrote:
> clang-format should catch these random missing spaces, though it still mangles
> IPC message map macros, so you'll need to be a bit careful.

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:58: if (!task_runner) {
On 2015/02/27 16:47:17, dcheng wrote:
> This seems like a programming error that we should be DCHECK()ing on, rather
> than trying to handle it gracefully somehow.

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:59: if (delegate) {
On 2015/02/27 16:47:17, dcheng wrote:
> IMO, |delegate| should probably be required too.

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:80: delegate_map_[delegate_id_counter_] =
delegate;
On 2015/02/27 16:47:17, dcheng wrote:
> I'd recommend using IDMap here. Also, it's probably better to just maintain
one
> map than two parallel maps.

I changed it to one map instead of two parallel maps. IDMap can't be accessed
from multiple threads (it can be created on one thread and accessed from a
different, consistent thread post-construction, but not multiple threads
post-construction), so the DCHECK's in that class for CalledOnValidThread()
fail.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:123: if ((time(0) - last_request_) <
batch_mode_timeout_seconds_) {
On 2015/02/27 16:47:17, dcheng wrote:
> I think we can use base::DelayTimer here to simplify the code, instead of
having
> a lot of our own tracking logic.

The timer api's aren't thread safe, so they won't work here.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:27: // |decoder| is used to identify the image in
case of decoding several
On 2015/02/27 16:47:17, dcheng wrote:
> This comment needs to be updated.

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:51: scoped_refptr<base::SequencedTaskRunner>
task_runner,
On 2015/02/27 16:47:18, dcheng wrote:
> Just an aside, it's slightly better to pass a scoped_refptr<T> arg as const
> scoped_refptr<T>&. That being said, the majority of Chrome code doesn't bother
> (and we may or may not change guidance on the future, depending on whether or
> not scoped_refptr becomes movable). This is more informational than anything,
as
> it's probably not worth changing given the uncertain state of affairs.

Acknowledged.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:52: bool shrink_to_fit = false);
On 2015/02/27 16:47:17, dcheng wrote:
> The Google C++ style guide forbids default args =(

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:59: class ImageDecoderImpl : public
content::UtilityProcessHostClient {
On 2015/02/27 16:47:17, dcheng wrote:
> I can't say I'm enthused about this delegate. I don't have any good
alternatives
> though. 

Acknowledged. I agree, but I can't think of any good ways around it.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:106: int delegate_id_counter_ = 0;
On 2015/02/27 16:47:17, dcheng wrote:
> Warning: personal preference. I prefer not to use in-class initialization if
> there's only one constructor. But it doesn't seem like a big deal.

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:123: time_t last_request_ = 0;
On 2015/02/27 16:47:17, dcheng wrote:
> This should probably be base::TimeTicks.

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.h:127: const int batch_mode_timeout_seconds_ = 5;
On 2015/02/27 16:47:18, dcheng wrote:
> kBatchModeTimeoutSeconds

Done.

https://codereview.chromium.org/931993002/diff/340001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:29: static const int
DUMMY_IMAGE_DELEGATE_ID = -999;
On 2015/02/27 16:47:18, dcheng wrote:
> I think we should be looking into how to clean up the extensions code, rather
> than adding a magic value for it.

I agree, see webstore_install_helper.cc for thoughts.

[Theresa, 2015-03-05 16:53:05]

ptal

fixed failing tests

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/340001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:123: if ((time(0) - last_request_) <
batch_mode_timeout_seconds_) {
On 2015/03/04 03:10:06, twellington wrote:
> On 2015/02/27 16:47:17, dcheng wrote:
> > I think we can use base::DelayTimer here to simplify the code, instead of
> having
> > a lot of our own tracking logic.
> 
> The timer api's aren't thread safe, so they won't work here.

I rethought this and spent some time playing around with it last night. I can
work around the non-thread-safe nature by putting the DelayTimer creation and
Reset calls in StartBatchMode and DecodeImageInSandbox. I have a patchset
stashed that does this, but there are some crashes I need to debug. One thing I
don't like about using the DelayTimer is in the case I'm testing, the bookmarks
grid view, if you scroll past hundreds of bookmarks, hundreds of requests are
made to decode images and Reset is called on the timer hundreds of time causing
a bunch of churn. In the current approach, a delayed task is posted at most once
every 5 seconds (rather than reset hundreds of times), so I'd like to stick with
the current approach. If that's not okay, I can debug the crashes and change it
to a delay timer.

For the IDMap (other comments above), there is in theory a similar workaround
where I could insert and remove items solely from the methods that run on the IO
thread, but I'd have to work around the issue of scoped ptrs not being Bindable
because the task_runners would have to be passed to DecodeImageInSandbox.

[Theresa, 2015-03-10 14:14:48]

ping

[dcheng, 2015-03-10 14:27:02]

I'm in transit between NYC and TPE today, but I haven't forgotten about
this. Will try to review before boarding.

On Tue, Mar 10, 2015, 10:14 null <twellington@chromium.org> wrote:

> ping
>
> https://codereview.chromium.org/931993002/
>

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

[dcheng, 2015-03-10 15:49:12]

+kalman to comment on if/how we can simplify the extensions code here. I'm
guessing it's written that way because the extension installer already has a
utility process, so they wanted to reuse it. But it's a bit unfortunate that
implementation details of ImageDecoder are leaking into other bits of code, and
especially unfortunate that we have to hardcode a placeholder ID. I guess if we
guarantee that the extension utility helper never gets reused, it might be OK...

The DelayTimer thing makes sense. I still think we might be able to simplify
this somewhat, using RepeatingTimer, but I don't have time to think this through
atm.

Finally, I think it'd be useful to document the general control flow between
threads in a header comment somewhere, e.g. something like:

// This is a helper class for decoding images safely in a utility process. To
use this,
// call ImageDecoder::Start(...) on any thread. Internally, all IPCs with the
utility
// host happen on the IO thread, and then the results are posted back to the
task runner
// requested when Start(...) was called.

Right now, it's a bit fuzzy for me what happens on what thread. For example, I
think request_info_id_map_ can actually be modified on multiple threads without
locking.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:68: if (!batch_mode_started_) {
Would it make the code simpler if we did something like this:
if (!batch_mode_started_) {
  /* start task */
}

/* unconditionally post the task to DecodeImageInSandbox here */


That way, I think you can move the ID allocation and map mutation entirely to
the IO thread (in DecodeImageInSandbox), so we can just use IDMap. This means we
post two tasks on the initial startup, but I think that's OK?

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:124: if (utility_process_host_.get()) {
Nit: .get() should be unnecessary to check the null-ity of a smart pointer.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:148: RequestInfo* request_info =
request_info_id_map_.find(id)->second;
Maybe:
auto& entry = request_info_id_map_.find(id);
if (entry != request_info_map_.end()) {
  RequestInfo* request_info = entry->second; 
  /* stuff */
  request_info_id_map_.erase(entry);
}

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:153: this, decoded_image,
request_info->delegate));
You can eliminate an indirection here, I think. Just do:

base::Bind(&Delegate::OnImageDecoded, request_info->delegate).

This brings to mind another question though:
How do we guarantee delegate is valid by the time we call it? The old
ImageDecoder implementation allowed delegates to "cancel" by letting them set
the delegate to null. This might be covered elsewhere and I'm just not seeing it
since I'm skimming through the patch atm.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:157: request_info = NULL;
Note: we prefer nullptr to NULL in new code.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:163: RequestInfo* request_info =
request_info_id_map_.find(id)->second;
I think it's OK to just store this directly in the std::map rather than using an
indirection. The struct is pretty small, and strictly speaking, we would leak
any RequestInfo entries if ImageDecoderImpl is destroyed.

(This comment is obsolete if we transition to IDMap)

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.h:81: typedef std::map<int, RequestInfo*>
RequestMap;
I have the vague impression that using foo = bar; is preferred over typedefs
now, but I don't really care (based on https://chromium-cpp.appspot.com).

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.h:124: // value must be saved instead.
I don't quite follow this comment. base::Bind() does handle binding to
refcounted objects. What sort of error were you seeing?

[not at google - send to devlin, 2015-03-10 18:02:04]

kalman@chromium.org changed reviewers:
+ asargent@chromium.org, kalman@chromium.org

[not at google - send to devlin, 2015-03-10 18:02:06]

Antony are you familiar with dcheng's last comment?

[asargent_no_longer_on_chrome, 2015-03-10 19:40:42]

On 2015/03/10 18:02:06, kalman wrote:
> Antony are you familiar with dcheng's last comment?

I agree that exposing the concept of id's to ChromeContentUtilityClient
subclasses feels wrong. I think a better model would be to have the API take a
base::Callback for the success/failure outcome, and internally generate the id's
and keep a map of id-to-Callback that it uses to dispatch the right Callback
when the IPC from the utility process comes back with a result.

[dcheng, 2015-03-10 19:49:31]

ImageDecoder is this API. The problem is the extension install helper
bypasses it.

On Tue, Mar 10, 2015, 15:40 null <asargent@chromium.org> wrote:

> On 2015/03/10 18:02:06, kalman wrote:
> > Antony are you familiar with dcheng's last comment?
>
> I agree that exposing the concept of id's to ChromeContentUtilityClient
> subclasses feels wrong. I think a better model would be to have the API
> take a
> base::Callback for the success/failure outcome, and internally generate the
> id's
> and keep a map of id-to-Callback that it uses to dispatch the right
> Callback
> when the IPC from the utility process comes back with a result.
>
>
> https://codereview.chromium.org/931993002/
>

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

[asargent_no_longer_on_chrome, 2015-03-10 20:01:31]

On 2015/03/10 19:49:31, dcheng wrote:
> ImageDecoder is this API. The problem is the extension install helper
> bypasses it.
> 
> On Tue, Mar 10, 2015, 15:40 null <mailto:asargent@chromium.org> wrote:
> 
> > On 2015/03/10 18:02:06, kalman wrote:
> > > Antony are you familiar with dcheng's last comment?
> >
> > I agree that exposing the concept of id's to ChromeContentUtilityClient
> > subclasses feels wrong. I think a better model would be to have the API
> > take a
> > base::Callback for the success/failure outcome, and internally generate the
> > id's
> > and keep a map of id-to-Callback that it uses to dispatch the right
> > Callback
> > when the IPC from the utility process comes back with a result.
> >
> >
> > https://codereview.chromium.org/931993002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Oh my apologies, I didn't look closely enough at the CL. I think it would be
fine to switch the webstore_install_helper.cc code to using the ImageDecoder,
and get rid of the "*DecodeImageBase64" stuff in extensions_handler.{h,cc}.

[Theresa, 2015-03-10 23:30:14]

On 2015/03/10 20:01:31, Antony Sargent wrote:
> On 2015/03/10 19:49:31, dcheng wrote:
> > ImageDecoder is this API. The problem is the extension install helper
> > bypasses it.
> > 
> > On Tue, Mar 10, 2015, 15:40 null <mailto:asargent@chromium.org> wrote:
> > 
> > > On 2015/03/10 18:02:06, kalman wrote:
> > > > Antony are you familiar with dcheng's last comment?
> > >
> > > I agree that exposing the concept of id's to ChromeContentUtilityClient
> > > subclasses feels wrong. I think a better model would be to have the API
> > > take a
> > > base::Callback for the success/failure outcome, and internally generate
the
> > > id's
> > > and keep a map of id-to-Callback that it uses to dispatch the right
> > > Callback
> > > when the IPC from the utility process comes back with a result.
> > >
> > >
> > > https://codereview.chromium.org/931993002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Oh my apologies, I didn't look closely enough at the CL. I think it would be
> fine to switch the webstore_install_helper.cc code to using the ImageDecoder,
> and get rid of the "*DecodeImageBase64" stuff in extensions_handler.{h,cc}.

In digging through the code, it looks like there's only one place the
icon_base64_data parameter passed the WebStoreInstallHelper constructor isn't an
empty string (if it's empty, DecodeImageBase64 isn't called). It's in
webstore_private_api.cc, line 265, where string::icon_data is set to
params_->details.icon_data if it exists or the empty string if it doesn't. Would
the ImageDecoder need to be change to handle Base64?

[asargent_no_longer_on_chrome, 2015-03-10 23:50:44]

On 2015/03/10 23:30:14, twellington wrote:
> On 2015/03/10 20:01:31, Antony Sargent wrote:
> > On 2015/03/10 19:49:31, dcheng wrote:
> > > ImageDecoder is this API. The problem is the extension install helper
> > > bypasses it.
> > > 
> > > On Tue, Mar 10, 2015, 15:40 null <mailto:asargent@chromium.org> wrote:
> > > 
> > > > On 2015/03/10 18:02:06, kalman wrote:
> > > > > Antony are you familiar with dcheng's last comment?
> > > >
> > > > I agree that exposing the concept of id's to ChromeContentUtilityClient
> > > > subclasses feels wrong. I think a better model would be to have the API
> > > > take a
> > > > base::Callback for the success/failure outcome, and internally generate
> the
> > > > id's
> > > > and keep a map of id-to-Callback that it uses to dispatch the right
> > > > Callback
> > > > when the IPC from the utility process comes back with a result.
> > > >
> > > >
> > > > https://codereview.chromium.org/931993002/
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> > Oh my apologies, I didn't look closely enough at the CL. I think it would be
> > fine to switch the webstore_install_helper.cc code to using the
ImageDecoder,
> > and get rid of the "*DecodeImageBase64" stuff in extensions_handler.{h,cc}.
> 
> In digging through the code, it looks like there's only one place the
> icon_base64_data parameter passed the WebStoreInstallHelper constructor isn't
an
> empty string (if it's empty, DecodeImageBase64 isn't called). It's in
> webstore_private_api.cc, line 265, where string::icon_data is set to
> params_->details.icon_data if it exists or the empty string if it doesn't.
Would
> the ImageDecoder need to be change to handle Base64?

If I recall correctly, that private API is used by the webstore during the
install process to generate the install confirmation dialog before we download
the .crx file. I think I initially added the base64 encoded image parameter, but
later added an "icon_url" one, and I think the webstore exclusively uses the
icon_url approach and not the base64 parameter. So we may be able to just get
rid of that. I can start a discussion thread with the webstore eng folks. 

If they do still need it though, I think it would be fine to just do the
base64->raw bytes conversion in the browser process before handing off to
ImageDecoder, or if security team would prefer it happen in the utility process,
to just add a separate method for base64 decoding and do 2 round trips to the
utility process since this is an infrequent operation with small image sizes.

[Theresa, 2015-03-10 23:50:54]

ptal

tl;dr: Now using IDMap and RepeaterTimer; also moved all of the work in
image_decoder to the IO thread and got rid of unnecessary indirection. I still
need to do something with the webstore_install_helper, debug a crash I'm seeing
somewhat frequently, and possibly add a "cancel" mechanism (see comment in
image_decoder.cc).


details: I'm a little stumped on the crash because I can't reliably reproduce it
and the cpp stack trace doesn't include any of the classes I've been mucking
with. I think the crash most likely has something to do with how the SkBitmap is
getting passed back to the delegates, will investigate more tomorrow. The
salient part of the cpp stack trace is below - if anything jumps out at you, let
me know:



signal 6 (SIGABRT), code -6 in tid 31471 (Thread-16710)
pid: 31403, tid: 31471, name: Thread-16710  >>> com.google.android.apps.chrome
<<<


Stack Trace:
  RELADDR   FUNCTION                                                            
                                                                                
                                                                                
                                                                                
                      FILE:LINE
.....
  0025561f  base::subtle::RefCountedBase::Release() const                       
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/code/clankium/src/out/Debug/../../base/memory/ref_counted.h:62
(discriminator 10)
  v------>  base::RefCounted<gfx::internal::ImageStorage>::Release() const      
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/code/clankium/src/out/Debug/../../base/memory/ref_counted.h:132
  v------> 
scoped_refptr<gfx::internal::ImageStorage>::Release(gfx::internal::ImageStorage*)
                                                                                
                                                                                
                                                                                
         
/usr/local/google/code/clankium/src/out/Debug/../../base/memory/ref_counted.h:383
  v------>  _ZN13scoped_refptrIN3gfx8internal12ImageStorageEED4Ev               
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/code/clankium/src/out/Debug/../../base/memory/ref_counted.h:291
  0031f0c1  gfx::Image::~Image()                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/code/clankium/src/out/Debug/../../ui/gfx/image/image.cc:449
  00f4174d  enhanced_bookmarks::ImageRecord::~ImageRecord()                     
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/code/clankium/src/out/Debug/../../components/enhanced_bookmarks/image_record.h:15
(discriminator 1)
  v------>  _ZN9TupleLeafILj2EN18enhanced_bookmarks11ImageRecordEED4Ev          
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/code/clankium/src/out/Debug/../../base/tuple.h:163
  v------> 
_ZN13TupleBaseImplI13IndexSequenceIJLj0ELj1ELj2EEEJN4base8internal17UnretainedWrapperI10ImageStoreEE4GURLN18enhanced_bookmarks11ImageRecordEEED4Ev
                                                                                
                                                                                
                         
/usr/local/google/code/clankium/src/out/Debug/../../base/tuple.h:156
  v------> 
_ZN5TupleIJN4base8internal17UnretainedWrapperI10ImageStoreEE4GURLN18enhanced_bookmarks11ImageRecordEEED4Ev
                                                                                
                                                                                
                                                                 
/usr/local/google/code/clankium/src/out/Debug/../../base/tuple.h:145
  01cff7d9  base::internal::BindState<base::internal::RunnableAdapter<void
(ImageStore::*)(GURL const&, enhanced_bookmarks::ImageRecord const&)>, void
(ImageStore*, GURL const&, enhanced_bookmarks::ImageRecord const&),
base::internal::TypeList<base::internal::UnretainedWrapper<ImageStore>, GURL,
enhanced_bookmarks::ImageRecord> >::~BindState() 
/usr/local/google/code/clankium/src/out/Debug/../../base/bind_internal.h:408
  01cff7f5  base::internal::BindState<base::internal::RunnableAdapter<void
(ImageStore::*)(GURL const&, enhanced_bookmarks::ImageRecord const&)>, void
(ImageStore*, GURL const&, enhanced_bookmarks::ImageRecord const&),
base::internal::TypeList<base::internal::UnretainedWrapper<ImageStore>, GURL,
enhanced_bookmarks::ImageRecord> >::~BindState() 
/usr/local/google/code/clankium/src/out/Debug/../../base/bind_internal.h:408
  v------> 
base::DefaultRefCountedThreadSafeTraits<base::internal::BindStateBase>::Destruct(base::internal::BindStateBase
const*)                                                                         
                                                                                
                                                             
/usr/local/google/code/clankium/src/out/Debug/../../base/memory/ref_counted.h:155
...

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:68: if (!batch_mode_started_) {
On 2015/03/10 15:49:12, dcheng wrote:
> Would it make the code simpler if we did something like this:
> if (!batch_mode_started_) {
>   /* start task */
> }
> 
> /* unconditionally post the task to DecodeImageInSandbox here */
> 
> 
> That way, I think you can move the ID allocation and map mutation entirely to
> the IO thread (in DecodeImageInSandbox), so we can just use IDMap. This means
we
> post two tasks on the initial startup, but I think that's OK?

Yes, that would be simpler. I shuffled things around so that StartBatchMode is
called from DecodeImageInSandbox; all this method does now is post a task to
DecodeImageInSandbox. I think the new approach is much cleaner and easier to
follow. The latest patchset also moves all of the work (IDMap manipulation,
checks for !batch_mode_started_, etc) to the IO thread so that we don't have to
worry about objects being accessed from multiple threads.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:124: if (utility_process_host_.get()) {
On 2015/03/10 15:49:12, dcheng wrote:
> Nit: .get() should be unnecessary to check the null-ity of a smart pointer.

Done.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:148: RequestInfo* request_info =
request_info_id_map_.find(id)->second;
On 2015/03/10 15:49:12, dcheng wrote:
> Maybe:
> auto& entry = request_info_id_map_.find(id);
> if (entry != request_info_map_.end()) {
>   RequestInfo* request_info = entry->second; 
>   /* stuff */
>   request_info_id_map_.erase(entry);
> }

Acknowledged.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:153: this, decoded_image,
request_info->delegate));
On 2015/03/10 15:49:12, dcheng wrote:
> You can eliminate an indirection here, I think. Just do:
> 
> base::Bind(&Delegate::OnImageDecoded, request_info->delegate).
> 
> This brings to mind another question though:
> How do we guarantee delegate is valid by the time we call it? The old
> ImageDecoder implementation allowed delegates to "cancel" by letting them set
> the delegate to null. This might be covered elsewhere and I'm just not seeing
it
> since I'm skimming through the patch atm.

wrt to the indirection, ref_counted.h complains about ImageDecoder::Delegate not
having AddRef()/Release(). 

This is a snippet of the code I was trying to compile:
    request_info->task_runner->PostTask(
        FROM_HERE,
        base::Bind(&Delegate::OnImageDecoded,
                   request_info->delegate,
                   decoded_image));

Eliminated the indirection.

With the current patchset, there is no "cancel" mechanism built in to
image_decoder. The one class I saw explicitly ignoring results was
change_picture_options_handler.cc. The way I read the code in that class,
ignoring the results is for convenience/to avoid unnecessary work, but not
setting the delegate to NULL (and therefore not ignoring the result) shouldn't
break things.

If that's incorrect (or cancel is desired for other reasons), I can look at ways
to add back a cancel mechanism.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:157: request_info = NULL;
On 2015/03/10 15:49:12, dcheng wrote:
> Note: we prefer nullptr to NULL in new code.

Done.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:163: RequestInfo* request_info =
request_info_id_map_.find(id)->second;
On 2015/03/10 15:49:12, dcheng wrote:
> I think it's OK to just store this directly in the std::map rather than using
an
> indirection. The struct is pretty small, and strictly speaking, we would leak
> any RequestInfo entries if ImageDecoderImpl is destroyed.
> 
> (This comment is obsolete if we transition to IDMap)

Acknowledged.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.h:81: typedef std::map<int, RequestInfo*>
RequestMap;
On 2015/03/10 15:49:12, dcheng wrote:
> I have the vague impression that using foo = bar; is preferred over typedefs
> now, but I don't really care (based on https://chromium-cpp.appspot.com).

Acknowledged.

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.h:124: // value must be saved instead.
On 2015/03/10 15:49:12, dcheng wrote:
> I don't quite follow this comment. base::Bind() does handle binding to
> refcounted objects. What sort of error were you seeing?

If I recall correctly, I was seeing this message (in bind.h):

static_assert(!internal::HasRefCountedParamAsRawPtr<is_method, Args...>::value,
"a_parameter_is_refcounted_type_and_needs_scoped_refptr");


But I must have been doing something wrong because I don't see it anymore.

[Theresa, 2015-03-10 23:55:49]

On 2015/03/10 23:50:44, Antony Sargent wrote:
> If I recall correctly, that private API is used by the webstore during the
> install process to generate the install confirmation dialog before we download
> the .crx file. I think I initially added the base64 encoded image parameter,
but
> later added an "icon_url" one, and I think the webstore exclusively uses the
> icon_url approach and not the base64 parameter. So we may be able to just get
> rid of that. I can start a discussion thread with the webstore eng folks. 
> 
> If they do still need it though, I think it would be fine to just do the
> base64->raw bytes conversion in the browser process before handing off to
> ImageDecoder, or if security team would prefer it happen in the utility
process,
> to just add a separate method for base64 decoding and do 2 round trips to the
> utility process since this is an infrequent operation with small image sizes.

It would be great if you could talk with webstore eng folks :) If we still need
base64, we can talk to security about which approach they prefer.

[dcheng, 2015-03-10 23:55:52]

For the crash, you could try using ASAN:
http://www.chromium.org/developers/testing/addresssanitizer#TOC-Building-on-A...

The recounted assert is to make sure that you bind a refcounted object as a
scoped_refptr<T>, rather than a T*, so the object is guaranteed to still be
alive when accessed by the Callback.

On Tue, Mar 10, 2015, 19:50 null <twellington@chromium.org> wrote:

> ptal
>
> tl;dr: Now using IDMap and RepeaterTimer; also moved all of the work in
> image_decoder to the IO thread and got rid of unnecessary indirection. I
> still
> need to do something with the webstore_install_helper, debug a crash I'm
> seeing
> somewhat frequently, and possibly add a "cancel" mechanism (see comment in
> image_decoder.cc).
>
>
> details: I'm a little stumped on the crash because I can't reliably
> reproduce it
> and the cpp stack trace doesn't include any of the classes I've been
> mucking
> with. I think the crash most likely has something to do with how the
> SkBitmap is
> getting passed back to the delegates, will investigate more tomorrow. The
> salient part of the cpp stack trace is below - if anything jumps out at
> you, let
> me know:
>
>
>
> signal 6 (SIGABRT), code -6 in tid 31471 (Thread-16710)
> pid: 31403, tid: 31471, name: Thread-16710  >>>
> com.google.android.apps.chrome
> <<<
>
>
> Stack Trace:
>    RELADDR   FUNCTION
>
>
>
>                        FILE:LINE
> .....
>    0025561f  base::subtle::RefCountedBase::Release() const
>
>
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../
> base/memory/ref_counted.h:62
> (discriminator 10)
>    v------>  base::RefCounted<gfx::internal::ImageStorage>::Release()
> const
>
>
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../
> base/memory/ref_counted.h:132
>    v------>
> scoped_refptr<gfx::internal::ImageStorage>::Release(gfx::
> internal::ImageStorage*)
>
>
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../
> base/memory/ref_counted.h:383
>    v------>  _ZN13scoped_refptrIN3gfx8internal12ImageStorageEED4Ev
>
>
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../
> base/memory/ref_counted.h:291
>    0031f0c1  gfx::Image::~Image()
>
>
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../
> ui/gfx/image/image.cc:449
>    00f4174d  enhanced_bookmarks::ImageRecord::~ImageRecord()
>
>
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../
> components/enhanced_bookmarks/image_record.h:15
> (discriminator 1)
>    v------>  _ZN9TupleLeafILj2EN18enhanced_bookmarks11ImageRecordEED4Ev
>
>
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../base/tuple.h:163
>    v------>
> _ZN13TupleBaseImplI13IndexSequenceIJLj0ELj1ELj2EEEJN4base8int
> ernal17UnretainedWrapperI10ImageStoreEE4GURLN18enhanced_
> bookmarks11ImageRecordEEED4Ev
>
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../base/tuple.h:156
>    v------>
> _ZN5TupleIJN4base8internal17UnretainedWrapperI10ImageStoreEE4
> GURLN18enhanced_bookmarks11ImageRecordEEED4Ev
>
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../base/tuple.h:145
>    01cff7d9  base::internal::BindState<base::internal::
> RunnableAdapter<void
> (ImageStore::*)(GURL const&, enhanced_bookmarks::ImageRecord const&)>,
> void
> (ImageStore*, GURL const&, enhanced_bookmarks::ImageRecord const&),
> base::internal::TypeList<base::internal::UnretainedWrapper<ImageStore>,
> GURL,
> enhanced_bookmarks::ImageRecord> >::~BindState()
> /usr/local/google/code/clankium/src/out/Debug/../../
> base/bind_internal.h:408
>    01cff7f5  base::internal::BindState<base::internal::
> RunnableAdapter<void
> (ImageStore::*)(GURL const&, enhanced_bookmarks::ImageRecord const&)>,
> void
> (ImageStore*, GURL const&, enhanced_bookmarks::ImageRecord const&),
> base::internal::TypeList<base::internal::UnretainedWrapper<ImageStore>,
> GURL,
> enhanced_bookmarks::ImageRecord> >::~BindState()
> /usr/local/google/code/clankium/src/out/Debug/../../
> base/bind_internal.h:408
>    v------>
> base::DefaultRefCountedThreadSafeTraits<base::internal::
> BindStateBase>::Destruct(base::internal::BindStateBase
> const*)
>
>
> /usr/local/google/code/clankium/src/out/Debug/../../
> base/memory/ref_counted.h:155
> ...
>
>
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.cc
> File chrome/browser/image_decoder.cc (right):
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.cc#newcode68
> chrome/browser/image_decoder.cc:68
>
<https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...>:
> if (!batch_mode_started_) {
> On 2015/03/10 15:49:12, dcheng wrote:
> > Would it make the code simpler if we did something like this:
> > if (!batch_mode_started_) {
> >    /* start task */
> > }
>
> > /* unconditionally post the task to DecodeImageInSandbox here */
>
>
> > That way, I think you can move the ID allocation and map mutation
> entirely to
> > the IO thread (in DecodeImageInSandbox), so we can just use IDMap.
> This means we
> > post two tasks on the initial startup, but I think that's OK?
>
> Yes, that would be simpler. I shuffled things around so that
> StartBatchMode is called from DecodeImageInSandbox; all this method does
> now is post a task to DecodeImageInSandbox. I think the new approach is
> much cleaner and easier to follow. The latest patchset also moves all of
> the work (IDMap manipulation, checks for !batch_mode_started_, etc) to
> the IO thread so that we don't have to worry about objects being
> accessed from multiple threads.
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.cc#newcode124
> chrome/browser/image_decoder.cc:124
>
<https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...>:
> if (utility_process_host_.get()) {
> On 2015/03/10 15:49:12, dcheng wrote:
> > Nit: .get() should be unnecessary to check the null-ity of a smart
> pointer.
>
> Done.
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.cc#newcode148
> chrome/browser/image_decoder.cc:148
>
<https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...>:
> RequestInfo* request_info =
> request_info_id_map_.find(id)->second;
> On 2015/03/10 15:49:12, dcheng wrote:
> > Maybe:
> > auto& entry = request_info_id_map_.find(id);
> > if (entry != request_info_map_.end()) {
> >    RequestInfo* request_info = entry->second;
> >    /* stuff */
> >    request_info_id_map_.erase(entry);
> > }
>
> Acknowledged.
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.cc#newcode153
> chrome/browser/image_decoder.cc:153
>
<https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...>:
> this, decoded_image,
> request_info->delegate));
> On 2015/03/10 15:49:12, dcheng wrote:
> > You can eliminate an indirection here, I think. Just do:
>
> > base::Bind(&Delegate::OnImageDecoded, request_info->delegate).
>
> > This brings to mind another question though:
> > How do we guarantee delegate is valid by the time we call it? The old
> > ImageDecoder implementation allowed delegates to "cancel" by letting
> them set
> > the delegate to null. This might be covered elsewhere and I'm just not
> seeing it
> > since I'm skimming through the patch atm.
>
> wrt to the indirection, ref_counted.h complains about
> ImageDecoder::Delegate not having AddRef()/Release().
>
> This is a snippet of the code I was trying to compile:
>      request_info->task_runner->PostTask(
>          FROM_HERE,
>          base::Bind(&Delegate::OnImageDecoded,
>                     request_info->delegate,
>                     decoded_image));
>
> Eliminated the indirection.
>
> With the current patchset, there is no "cancel" mechanism built in to
> image_decoder. The one class I saw explicitly ignoring results was
> change_picture_options_handler.cc. The way I read the code in that
> class, ignoring the results is for convenience/to avoid unnecessary
> work, but not setting the delegate to NULL (and therefore not ignoring
> the result) shouldn't break things.
>
> If that's incorrect (or cancel is desired for other reasons), I can look
> at ways to add back a cancel mechanism.
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.cc#newcode157
> chrome/browser/image_decoder.cc:157
>
<https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...>:
> request_info = NULL;
> On 2015/03/10 15:49:12, dcheng wrote:
> > Note: we prefer nullptr to NULL in new code.
>
> Done.
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.cc#newcode163
> chrome/browser/image_decoder.cc:163
>
<https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...>:
> RequestInfo* request_info =
> request_info_id_map_.find(id)->second;
> On 2015/03/10 15:49:12, dcheng wrote:
> > I think it's OK to just store this directly in the std::map rather
> than using an
> > indirection. The struct is pretty small, and strictly speaking, we
> would leak
> > any RequestInfo entries if ImageDecoderImpl is destroyed.
>
> > (This comment is obsolete if we transition to IDMap)
>
> Acknowledged.
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.h
> File chrome/browser/image_decoder.h (right):
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.h#newcode81
> chrome/browser/image_decoder.h:81
>
<https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...>:
> typedef std::map<int, RequestInfo*>
> RequestMap;
> On 2015/03/10 15:49:12, dcheng wrote:
> > I have the vague impression that using foo = bar; is preferred over
> typedefs
> > now, but I don't really care (based on
> https://chromium-cpp.appspot.com).
>
> Acknowledged.
>
> https://codereview.chromium.org/931993002/diff/520001/
> chrome/browser/image_decoder.h#newcode124
> chrome/browser/image_decoder.h:124
>
<https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...>:
> // value must be saved instead.
> On 2015/03/10 15:49:12, dcheng wrote:
> > I don't quite follow this comment. base::Bind() does handle binding to
> > refcounted objects. What sort of error were you seeing?
>
> If I recall correctly, I was seeing this message (in bind.h):
>
> static_assert(!internal::HasRefCountedParamAsRawPtr<is_method,
> Args...>::value,
> "a_parameter_is_refcounted_type_and_needs_scoped_refptr");
>
>
> But I must have been doing something wrong because I don't see it
> anymore.
>
> https://codereview.chromium.org/931993002/
>

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

[asargent_no_longer_on_chrome, 2015-03-11 23:48:01]

On 2015/03/10 23:55:49, twellington wrote:
> On 2015/03/10 23:50:44, Antony Sargent wrote:
> > If I recall correctly, that private API is used by the webstore during the
> > install process to generate the install confirmation dialog before we
download
> > the .crx file. I think I initially added the base64 encoded image parameter,
> but
> > later added an "icon_url" one, and I think the webstore exclusively uses the
> > icon_url approach and not the base64 parameter. So we may be able to just
get
> > rid of that. I can start a discussion thread with the webstore eng folks. 
> > 
> > If they do still need it though, I think it would be fine to just do the
> > base64->raw bytes conversion in the browser process before handing off to
> > ImageDecoder, or if security team would prefer it happen in the utility
> process,
> > to just add a separate method for base64 decoding and do 2 round trips to
the
> > utility process since this is an infrequent operation with small image
sizes.
> 
> It would be great if you could talk with webstore eng folks :) If we still
need
> base64, we can talk to security about which approach they prefer.

I got confirmation from a webstore engineer that we can remove the iconData
parameter which requires the base64 decoding. I can try and work up a CL to
remove this in the extensions API code. If you don't want to wait for that to
land first, feel free to do whatever is most convenient in this CL to sever the
dependencies (maybe removing the decode image stuff from
chrome_content_utility_client.{h,cc}?)

[Theresa, 2015-03-12 00:33:55]

On 2015/03/11 23:48:01, Antony Sargent wrote:
> On 2015/03/10 23:55:49, twellington wrote:
> > On 2015/03/10 23:50:44, Antony Sargent wrote:
> > > If I recall correctly, that private API is used by the webstore during the
> > > install process to generate the install confirmation dialog before we
> download
> > > the .crx file. I think I initially added the base64 encoded image
parameter,
> > but
> > > later added an "icon_url" one, and I think the webstore exclusively uses
the
> > > icon_url approach and not the base64 parameter. So we may be able to just
> get
> > > rid of that. I can start a discussion thread with the webstore eng folks. 
> > > 
> > > If they do still need it though, I think it would be fine to just do the
> > > base64->raw bytes conversion in the browser process before handing off to
> > > ImageDecoder, or if security team would prefer it happen in the utility
> > process,
> > > to just add a separate method for base64 decoding and do 2 round trips to
> the
> > > utility process since this is an infrequent operation with small image
> sizes.
> > 
> > It would be great if you could talk with webstore eng folks :) If we still
> need
> > base64, we can talk to security about which approach they prefer.
> 
> I got confirmation from a webstore engineer that we can remove the iconData
> parameter which requires the base64 decoding. I can try and work up a CL to
> remove this in the extensions API code. If you don't want to wait for that to
> land first, feel free to do whatever is most convenient in this CL to sever
the
> dependencies (maybe removing the decode image stuff from
> chrome_content_utility_client.{h,cc}?)

My latest patchset actually removes the base64 support and refactors
webstore_install_helper to use the image_decoder. I was looking at some tryjob
failures before posting a ptal for you.. it looks like they test failures are
flaky (other patchsets have been failing in the same way), if you want to take a
look now. I'll rerun the tryjobs in the morning if you want to wait :)

[Theresa, 2015-03-12 00:36:32]

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/520001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:153: this, decoded_image,
request_info->delegate));
On 2015/03/10 23:50:54, twellington wrote:
> On 2015/03/10 15:49:12, dcheng wrote:
> > You can eliminate an indirection here, I think. Just do:
> > 
> > base::Bind(&Delegate::OnImageDecoded, request_info->delegate).
> > 
> > This brings to mind another question though:
> > How do we guarantee delegate is valid by the time we call it? The old
> > ImageDecoder implementation allowed delegates to "cancel" by letting them
set
> > the delegate to null. This might be covered elsewhere and I'm just not
seeing
> it
> > since I'm skimming through the patch atm.
> 
> wrt to the indirection, ref_counted.h complains about ImageDecoder::Delegate
not
> having AddRef()/Release(). 
> 
> This is a snippet of the code I was trying to compile:
>     request_info->task_runner->PostTask(
>         FROM_HERE,
>         base::Bind(&Delegate::OnImageDecoded,
>                    request_info->delegate,
>                    decoded_image));
> 
> Eliminated the indirection.
> 
> With the current patchset, there is no "cancel" mechanism built in to
> image_decoder. The one class I saw explicitly ignoring results was
> change_picture_options_handler.cc. The way I read the code in that class,
> ignoring the results is for convenience/to avoid unnecessary work, but not
> setting the delegate to NULL (and therefore not ignoring the result) shouldn't
> break things.
> 
> If that's incorrect (or cancel is desired for other reasons), I can look at
ways
> to add back a cancel mechanism.

The first part of this comment, above "Eliminated the indirection" should have
been deleted. I was just missing base::Unretained() around
request_info->delegate.

[Theresa, 2015-03-12 18:39:28]

On 2015/03/12 00:33:55, twellington wrote:
> My latest patchset actually removes the base64 support and refactors
> webstore_install_helper to use the image_decoder. I was looking at some tryjob
> failures before posting a ptal for you.. it looks like they test failures are
> flaky (other patchsets have been failing in the same way), if you want to take
a
> look now. I'll rerun the tryjobs in the morning if you want to wait :)


Antony - ptal at webstore_private_api.cc, bundle_installer.cc,
webstore_install_helper.h, webstore_install_helper.cc,
webstore_standalone_installer.cc, kiosk_app_data.cc,
chrome_utility_extensions_messages.h, extensions_handler.h, and
extensions_handler.cc (set of files that changed between patchset 28 & 29).

The mac_chromium_dbg_ng is having device problems;
linux_chromium_chromeos_asan_rel_ng has been having problems with the
FileManagerBrowserTests with & without patches for more than just this CL, so I
don't think it's going to give us a clean run :-/

[asargent_no_longer_on_chrome, 2015-03-12 21:40:08]

extensions parts lgtm w/ one suggested improvement to save a copy or two and one
tiny bit of pedantic pedantry

https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:109: this,
std::string(fetched_icon_data_.begin(), fetched_icon_data_.end()),
I think we should just change fetched_icon_data_ to be a std::string instead of
std::vector<unsigned char>, and up in ::OnURLFetchComplete around line 94 we can
grab the response from the URLFetch and put it directly into fetched_icon_data_
instead of the response_data temporary. 

That might also save a copy of the bytes into the temporary std::string here on
line 109 (I'm not sure off the top of my head whether the compiler would be
smart enough to optimize that away or not).

https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.h (right):

https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.h:70: // |icon_url| can be
empty.
pedantic nit: We might want this to be something like "It is legal for
|icon_url| to be empty.", or at a minimum change the word "can" to "may" to make
it clear that we are giving permission to consumers of the API to not provide an
icon URL here.

[Theresa, 2015-03-12 22:19:10]

Antony - ptal; I diverted a little bit from the suggested improvement

https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:109: this,
std::string(fetched_icon_data_.begin(), fetched_icon_data_.end()),
On 2015/03/12 21:40:08, Antony Sargent wrote:
> I think we should just change fetched_icon_data_ to be a std::string instead
of
> std::vector<unsigned char>, and up in ::OnURLFetchComplete around line 94 we
can
> grab the response from the URLFetch and put it directly into
fetched_icon_data_
> instead of the response_data temporary. 
> 
> That might also save a copy of the bytes into the temporary std::string here
on
> line 109 (I'm not sure off the top of my head whether the compiler would be
> smart enough to optimize that away or not). 

Good catch :) Instead of doing that, I removed this method
(StartFetchedImageDecode) and call ImageDecoder::GetInstance()->Start directly
from OnURLFetchComplete. StartFetchedImageDecode used to be needed because
interaction with the utility process had to happen on the IO thread, but calling
ImageDecoder::GetInstance()->Start can happen from any thread. This change also
removed the need for fetched_icon_data_ so I delete that variable as well.

https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.h (right):

https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.h:70: // |icon_url| can be
empty.
On 2015/03/12 21:40:08, Antony Sargent wrote:
> pedantic nit: We might want this to be something like "It is legal for
> |icon_url| to be empty.", or at a minimum change the word "can" to "may" to
make
> it clear that we are giving permission to consumers of the API to not provide
an
> icon URL here. 
> 
> 

Done.

[asargent_no_longer_on_chrome, 2015-03-12 22:40:44]

On 2015/03/12 22:19:10, twellington wrote:
> Antony - ptal; I diverted a little bit from the suggested improvement
> 
>
https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
> File chrome/browser/extensions/webstore_install_helper.cc (right):
> 
>
https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
> chrome/browser/extensions/webstore_install_helper.cc:109: this,
> std::string(fetched_icon_data_.begin(), fetched_icon_data_.end()),
> On 2015/03/12 21:40:08, Antony Sargent wrote:
> > I think we should just change fetched_icon_data_ to be a std::string instead
> of
> > std::vector<unsigned char>, and up in ::OnURLFetchComplete around line 94 we
> can
> > grab the response from the URLFetch and put it directly into
> fetched_icon_data_
> > instead of the response_data temporary. 
> > 
> > That might also save a copy of the bytes into the temporary std::string here
> on
> > line 109 (I'm not sure off the top of my head whether the compiler would be
> > smart enough to optimize that away or not). 
> 
> Good catch :) Instead of doing that, I removed this method
> (StartFetchedImageDecode) and call ImageDecoder::GetInstance()->Start directly
> from OnURLFetchComplete. StartFetchedImageDecode used to be needed because
> interaction with the utility process had to happen on the IO thread, but
calling
> ImageDecoder::GetInstance()->Start can happen from any thread. This change
also
> removed the need for fetched_icon_data_ so I delete that variable as well.
> 
>
https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
> File chrome/browser/extensions/webstore_install_helper.h (right):
> 
>
https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
> chrome/browser/extensions/webstore_install_helper.h:70: // |icon_url| can be
> empty.
> On 2015/03/12 21:40:08, Antony Sargent wrote:
> > pedantic nit: We might want this to be something like "It is legal for
> > |icon_url| to be empty.", or at a minimum change the word "can" to "may" to
> make
> > it clear that we are giving permission to consumers of the API to not
provide
> an
> > icon URL here. 
> > 
> > 
> 
> Done.

This new version lgtm. I was briefly worried that passing a reference to the
|response_data| temporary variable might be problematic since it goes out of
scope and is deallocated before the callback runs, but I chatted with an
officemate and we think the way base::Bind is written is smart enough to make a
copy in such cases. 

BTW - there might be an opportunity to avoid even this copy by having a version
of ImageDecoder::Start which takes a scoped_ptr<std::string> instead of const
std::string&, and then make response_data a scoped_ptr<std::string> which you
then Pass to the new version of ImageDecoder::Start. That's probably only worth
doing if the are other consumers of ImageDecoder (aside from
WebstoreInstallHelper) that would make use of it.

[dcheng, 2015-03-13 14:20:41]

On 2015/03/12 at 22:40:44, asargent wrote:
> On 2015/03/12 22:19:10, twellington wrote:
> > Antony - ptal; I diverted a little bit from the suggested improvement
> > 
> >
https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
> > File chrome/browser/extensions/webstore_install_helper.cc (right):
> > 
> >
https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
> > chrome/browser/extensions/webstore_install_helper.cc:109: this,
> > std::string(fetched_icon_data_.begin(), fetched_icon_data_.end()),
> > On 2015/03/12 21:40:08, Antony Sargent wrote:
> > > I think we should just change fetched_icon_data_ to be a std::string
instead
> > of
> > > std::vector<unsigned char>, and up in ::OnURLFetchComplete around line 94
we
> > can
> > > grab the response from the URLFetch and put it directly into
> > fetched_icon_data_
> > > instead of the response_data temporary. 
> > > 
> > > That might also save a copy of the bytes into the temporary std::string
here
> > on
> > > line 109 (I'm not sure off the top of my head whether the compiler would
be
> > > smart enough to optimize that away or not). 
> > 
> > Good catch :) Instead of doing that, I removed this method
> > (StartFetchedImageDecode) and call ImageDecoder::GetInstance()->Start
directly
> > from OnURLFetchComplete. StartFetchedImageDecode used to be needed because
> > interaction with the utility process had to happen on the IO thread, but
calling
> > ImageDecoder::GetInstance()->Start can happen from any thread. This change
also
> > removed the need for fetched_icon_data_ so I delete that variable as well.
> > 
> >
https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
> > File chrome/browser/extensions/webstore_install_helper.h (right):
> > 
> >
https://codereview.chromium.org/931993002/diff/560001/chrome/browser/extensio...
> > chrome/browser/extensions/webstore_install_helper.h:70: // |icon_url| can be
> > empty.
> > On 2015/03/12 21:40:08, Antony Sargent wrote:
> > > pedantic nit: We might want this to be something like "It is legal for
> > > |icon_url| to be empty.", or at a minimum change the word "can" to "may"
to
> > make
> > > it clear that we are giving permission to consumers of the API to not
provide
> > an
> > > icon URL here. 
> > > 
> > > 
> > 
> > Done.
> 
> This new version lgtm. I was briefly worried that passing a reference to the
|response_data| temporary variable might be problematic since it goes out of
scope and is deallocated before the callback runs, but I chatted with an
officemate and we think the way base::Bind is written is smart enough to make a
copy in such cases. 
> 
> BTW - there might be an opportunity to avoid even this copy by having a
version of ImageDecoder::Start which takes a scoped_ptr<std::string> instead of
const std::string&, and then make response_data a scoped_ptr<std::string> which
you then Pass to the new version of ImageDecoder::Start. That's probably only
worth doing if the are other consumers of ImageDecoder (aside from
WebstoreInstallHelper) that would make use of it.

base::Bind() explicitly copies data that is bound to a const T&. You have to use
a helper to force it bind as a const ref (which is potentially dangerous for the
reasons you stated).
http://www.chromium.org/developers/coding-style/important-abstractions-and-da...

Sorry I haven't really gotten around to this, the jet lag has been pretty bad.

[dcheng, 2015-03-13 14:55:11]

I haven't reviewed the patch in its entirety, but we definitely need to solve
the cancellation problem.

https://codereview.chromium.org/931993002/diff/580001/chrome/browser/android/...
File chrome/browser/android/logo_service.cc (left):

https://codereview.chromium.org/931993002/diff/580001/chrome/browser/android/...
chrome/browser/android/logo_service.cc:58: ~LogoDecoderDelegate() override {
image_decoder_->set_delegate(NULL); }
This is an example of an instance where cancellation semantics are required.
Calling set_delegate(NULL) here prevented the old implementation of ImageDecoder
from calling a deleted delegate. However, there doesn't appear to be an
equivalent for the new code. So at the very least, we'll need to figure out a
solution for that.

[Theresa, 2015-03-13 15:22:16]

I'll work on a cancellation mechanism and ping again when it's ready for review.
I have one question (below) on the general approach.

https://codereview.chromium.org/931993002/diff/580001/chrome/browser/android/...
File chrome/browser/android/logo_service.cc (left):

https://codereview.chromium.org/931993002/diff/580001/chrome/browser/android/...
chrome/browser/android/logo_service.cc:58: ~LogoDecoderDelegate() override {
image_decoder_->set_delegate(NULL); }
On 2015/03/13 14:55:11, dcheng wrote:
> This is an example of an instance where cancellation semantics are required.
> Calling set_delegate(NULL) here prevented the old implementation of
ImageDecoder
> from calling a deleted delegate. However, there doesn't appear to be an
> equivalent for the new code. So at the very least, we'll need to figure out a
> solution for that.

Could delegates be stored as weak ptrs in image_decoder, then checked to make
sure they still exist? Or do we want a more explicit cancellation mechanism e.g.
a method in image_decoder that removes a delegate from the IDMap?

[dcheng, 2015-03-13 15:30:31]

We probably shouldn't use WeakPtrs as the cancelation mechanism, since they
aren't thread safe, and it would require all the delegates to add a weak
pointer factory.

On Fri, Mar 13, 2015, 23:22 null <twellington@chromium.org> wrote:

> I'll work on a cancellation mechanism and ping again when it's ready for
> review.
> I have one question (below) on the general approach.
>
>
> https://codereview.chromium.org/931993002/diff/580001/
> chrome/browser/android/logo_service.cc
> File chrome/browser/android/logo_service.cc (left):
>
> https://codereview.chromium.org/931993002/diff/580001/
> chrome/browser/android/logo_service.cc#oldcode58
> chrome/browser/android/logo_service.cc:58
>
<https://codereview.chromium.org/931993002/diff/580001/chrome/browser/android/...>:
> ~LogoDecoderDelegate()
> override { image_decoder_->set_delegate(NULL); }
> On 2015/03/13 14:55:11, dcheng wrote:
> > This is an example of an instance where cancellation semantics are
> required.
> > Calling set_delegate(NULL) here prevented the old implementation of
> ImageDecoder
> > from calling a deleted delegate. However, there doesn't appear to be
> an
> > equivalent for the new code. So at the very least, we'll need to
> figure out a
> > solution for that.
>
> Could delegates be stored as weak ptrs in image_decoder, then checked to
> make sure they still exist? Or do we want a more explicit cancellation
> mechanism e.g. a method in image_decoder that removes a delegate from
> the IDMap?
>
> https://codereview.chromium.org/931993002/
>

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

[Theresa, 2015-03-16 19:51:51]

On 2015/03/13 15:30:31, dcheng wrote:
> We probably shouldn't use WeakPtrs as the cancelation mechanism, since they
> aren't thread safe, and it would require all the delegates to add a weak
> pointer factory.


ptal - added ImageDecoder::RemoveDelegate as a cancellation mechanism.

[Theresa, 2015-03-19 13:44:30]

ping

[dcheng, 2015-03-19 14:25:46]

Sorry for suggesting IDMap. Given the nature of this class and the way it has to
potentially hop between a lot of threads, I think it's just too problematic to
make it work =(

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/android/...
File chrome/browser/android/logo_service.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/android/...
chrome/browser/android/logo_service.cc:55:
ImageDecoder::GetInstance()->RemoveDelegate(this);
I don't know that having GetInstance() everywhere is particularly helpful.

To my knowledge, ImageDecoder isn't mocked out in tests. So we could just make
Start()/RemoveDelegate() static methods and save a bunch of typing. WDYT?

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:105:
image_data_queue_.pop();
After thinking about this, I feel uncomfortable with depending on a FIFO
guarantee by Start() to guarantee correctness of the multiplexed requests.

I think it's better to either:
1) Use base::Bind() and bundle all the required state with the bound callback.
2) Disallow the use of a single ImageDispatcher for multiple requests.

Of these options, I would prefer #2.

#1 requires each call to Start() to bind two callbacks, and instantiates a bunch
of templates that probably won't be folded together, resulting in bloat.

#2 is similar to the existing model, except that places like this probably
wouldn't subclass ImageDelegate directly and would have a helper class subclass
it instead. We could also rename ImageDelegate to ImageRequest too (which is
probably a more appropriate name, since delegate typically implies that there's
only one of them). So we'd end up with something like:

class ImageRequest {
 public:
  virtual ~ImageRequest();
  virtual void OnImageDecoded(...) = 0;
  virtual void OnDecodeImageFailed(...) = 0;
  TaskRunner* task_runner() const { return t.get(); }

 protected:
  explicit ImageRequest(const scoped_refptr<TaskRunner>& t);

 private:
  const scoped_refptr<T> t;
};

and then starting/stopping it would look like:

ImageDecoder::Start(request);

and we could make it automatically cancel on destruction too:
ImageRequest::~ImageRequest() {
  ImageDecoder::Cancel(this);
}

(though of course this comes with its own bag of worms WRT thread-safety issues,
see below)

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:48: BrowserThread::PostTask(
So this is tricky.

Due to IDMap's non-thread safe nature, we have to do this on the IO thread.

But the object being destroyed is probably not on the IO thread. Another task
might already be on the IO thread, ready to send a callback to the object we're
removing.

I guess we're better off rolling our own solution here =/

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:88: request_info_id_map_.AddWithID(request_info,
delegate_id_counter_);
For future reference, I think you can just use plain old Add(), which will
automatically create an ID.

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:90: if (image_codec == ROBUST_JPEG_CODEC) {
Consider writing this as a switch, so if someone ever adds new values, we don't
miss it.

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:116: delete request_info;
For future reference, IDMap has a setting that indicates it should own the
pointer
(https://code.google.com/p/chromium/codesearch#chromium/src/base/id_map.h&l=19),
so you don't have to do manual memory management yourself.

[Theresa, 2015-03-19 21:54:57]

Addressed most of the issues from dcheng@'s last review. I still need to do
something about IDMap (see comments in image_decoder.cc)

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/android/...
File chrome/browser/android/logo_service.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/android/...
chrome/browser/android/logo_service.cc:55:
ImageDecoder::GetInstance()->RemoveDelegate(this);
On 2015/03/19 14:25:46, dcheng wrote:
> I don't know that having GetInstance() everywhere is particularly helpful.
> 
> To my knowledge, ImageDecoder isn't mocked out in tests. So we could just make
> Start()/RemoveDelegate() static methods and save a bunch of typing. WDYT?

Done. I agree, it's not particularly helpful to expose GetInstance().

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:105:
image_data_queue_.pop();
On 2015/03/19 14:25:46, dcheng wrote:
> After thinking about this, I feel uncomfortable with depending on a FIFO
> guarantee by Start() to guarantee correctness of the multiplexed requests.
> 
> I think it's better to either:
> 1) Use base::Bind() and bundle all the required state with the bound callback.
> 2) Disallow the use of a single ImageDispatcher for multiple requests.
> 
> Of these options, I would prefer #2.
> 
> #1 requires each call to Start() to bind two callbacks, and instantiates a
bunch
> of templates that probably won't be folded together, resulting in bloat.
> 
> #2 is similar to the existing model, except that places like this probably
> wouldn't subclass ImageDelegate directly and would have a helper class
subclass
> it instead. We could also rename ImageDelegate to ImageRequest too (which is
> probably a more appropriate name, since delegate typically implies that
there's
> only one of them). So we'd end up with something like:
> 
> class ImageRequest {
>  public:
>   virtual ~ImageRequest();
>   virtual void OnImageDecoded(...) = 0;
>   virtual void OnDecodeImageFailed(...) = 0;
>   TaskRunner* task_runner() const { return t.get(); }
> 
>  protected:
>   explicit ImageRequest(const scoped_refptr<TaskRunner>& t);
> 
>  private:
>   const scoped_refptr<T> t;
> };
> 
> and then starting/stopping it would look like:
> 
> ImageDecoder::Start(request);
> 
> and we could make it automatically cancel on destruction too:
> ImageRequest::~ImageRequest() {
>   ImageDecoder::Cancel(this);
> }
> 
> (though of course this comes with its own bag of worms WRT thread-safety
issues,
> see below)

I created a UserImageLoader::ImageRequest subclass that extends
ImageDecoder::Delegate; it keeps track of all the info needed in OnImageDecoded
and OnDecodeImageFailed. The queue is gone :)

I also changed the ImageDecoder::Delegate class to keep track of the task
runner; I really like this, because it's one less arg that has to get passed
around.

I haven't renamed Delegate -> ImageRequest yet. For cases like this, where a new
ImageRequest object will get created for each image to decode, "request" makes
sense to me. For most other cases, however, the classes using ImageDecoder
directly inherit from ImageDecoder::Delegate; they don't have or create multiple
delegates, they just are the "delegate". I don't particularly care about the
name, though, so if ImageRequest makes more sense with how we generally use the
terms in Chromium, I'll change it.

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:48: BrowserThread::PostTask(
On 2015/03/19 14:25:46, dcheng wrote:
> So this is tricky.
> 
> Due to IDMap's non-thread safe nature, we have to do this on the IO thread.
> 
> But the object being destroyed is probably not on the IO thread. Another task
> might already be on the IO thread, ready to send a callback to the object
we're
> removing.
> 
> I guess we're better off rolling our own solution here =/

I could go back to using a std::map and use base::AutoLock
lock(request_info_id_map_) in the methods that manipulate the map. Rather than
having this method post a task to the IO thread, it could acquire the lock and
remove the appropriate entry(ies) from the map. This would at least avoid the
issue of the RemoveDelegate request getting tacked to the back of the IO
thread's queue of tasks behind an OnImageDecoded callback to the object we're
removing.

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:88: request_info_id_map_.AddWithID(request_info,
delegate_id_counter_);
On 2015/03/19 14:25:46, dcheng wrote:
> For future reference, I think you can just use plain old Add(), which will
> automatically create an ID.

Acknowledged. I didn't read the API closely enough to notice that plain old
Add() returns an ID that could be used instead of keeping a
delegate_id_counter_.

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:90: if (image_codec == ROBUST_JPEG_CODEC) {
On 2015/03/19 14:25:46, dcheng wrote:
> Consider writing this as a switch, so if someone ever adds new values, we
don't
> miss it.

Done.

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:116: delete request_info;
On 2015/03/19 14:25:46, dcheng wrote:
> For future reference, IDMap has a setting that indicates it should own the
> pointer
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/id_map.h&l=19),
> so you don't have to do manual memory management yourself.

Acknowledged.

[dcheng, 2015-03-20 00:14:46]

Overall, looking good! I didn't have time to review the whole patch, but a few
quick comments to keep things moving along.

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:105:
image_data_queue_.pop();
On 2015/03/19 21:54:56, twellington wrote:
> On 2015/03/19 14:25:46, dcheng wrote:
> > After thinking about this, I feel uncomfortable with depending on a FIFO
> > guarantee by Start() to guarantee correctness of the multiplexed requests.
> > 
> > I think it's better to either:
> > 1) Use base::Bind() and bundle all the required state with the bound
callback.
> > 2) Disallow the use of a single ImageDispatcher for multiple requests.
> > 
> > Of these options, I would prefer #2.
> > 
> > #1 requires each call to Start() to bind two callbacks, and instantiates a
> bunch
> > of templates that probably won't be folded together, resulting in bloat.
> > 
> > #2 is similar to the existing model, except that places like this probably
> > wouldn't subclass ImageDelegate directly and would have a helper class
> subclass
> > it instead. We could also rename ImageDelegate to ImageRequest too (which is
> > probably a more appropriate name, since delegate typically implies that
> there's
> > only one of them). So we'd end up with something like:
> > 
> > class ImageRequest {
> >  public:
> >   virtual ~ImageRequest();
> >   virtual void OnImageDecoded(...) = 0;
> >   virtual void OnDecodeImageFailed(...) = 0;
> >   TaskRunner* task_runner() const { return t.get(); }
> > 
> >  protected:
> >   explicit ImageRequest(const scoped_refptr<TaskRunner>& t);
> > 
> >  private:
> >   const scoped_refptr<T> t;
> > };
> > 
> > and then starting/stopping it would look like:
> > 
> > ImageDecoder::Start(request);
> > 
> > and we could make it automatically cancel on destruction too:
> > ImageRequest::~ImageRequest() {
> >   ImageDecoder::Cancel(this);
> > }
> > 
> > (though of course this comes with its own bag of worms WRT thread-safety
> issues,
> > see below)
> 
> I created a UserImageLoader::ImageRequest subclass that extends
> ImageDecoder::Delegate; it keeps track of all the info needed in
OnImageDecoded
> and OnDecodeImageFailed. The queue is gone :)
> 
> I also changed the ImageDecoder::Delegate class to keep track of the task
> runner; I really like this, because it's one less arg that has to get passed
> around.
> 
> I haven't renamed Delegate -> ImageRequest yet. For cases like this, where a
new
> ImageRequest object will get created for each image to decode, "request" makes
> sense to me. For most other cases, however, the classes using ImageDecoder
> directly inherit from ImageDecoder::Delegate; they don't have or create
multiple
> delegates, they just are the "delegate". I don't particularly care about the
> name, though, so if ImageRequest makes more sense with how we generally use
the
> terms in Chromium, I'll change it.

I think it's OK for those single-shot instances to also subclass from
ImageRequest. Since they are single-shot requests, it makes sense that the class
itself subclasses ImageRequest directly (LogoDecoderDelegate is an instance of
this).

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:48: BrowserThread::PostTask(
On 2015/03/19 21:54:56, twellington wrote:
> On 2015/03/19 14:25:46, dcheng wrote:
> > So this is tricky.
> > 
> > Due to IDMap's non-thread safe nature, we have to do this on the IO thread.
> > 
> > But the object being destroyed is probably not on the IO thread. Another
task
> > might already be on the IO thread, ready to send a callback to the object
> we're
> > removing.
> > 
> > I guess we're better off rolling our own solution here =/
> 
> I could go back to using a std::map and use base::AutoLock
> lock(request_info_id_map_) in the methods that manipulate the map. Rather than
> having this method post a task to the IO thread, it could acquire the lock and
> remove the appropriate entry(ies) from the map. This would at least avoid the
> issue of the RemoveDelegate request getting tacked to the back of the IO
> thread's queue of tasks behind an OnImageDecoded callback to the object we're
> removing.

Yeah, I think that's the best idea.
It might also be nice to have an O(1) cancel, depending on how deeply queued
this can get.

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:94:
ImageRequest* image_request = new ImageRequest(image_info, *data, this);
Where is this freed?

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:36: ImageDecoder::Delegate::~Delegate() {
Call RemoveDelegate(this) here. Deleting this should automatically cancel. This
removes the need for subclasses like LogoDecoderDelegate to manually call it.

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/image_de...
chrome/browser/image_decoder.h:64: static void RemoveDelegate(Delegate*
delegate);
Cancel() is probably a clearer name for this.

[Theresa, 2015-03-20 19:09:01]

dcheng@ - I have one question in image_decoder.cc about O(1) Cancel, but
otherwise addressed all comments in the latest patchset. I'm running memory
tests right now to check for leaks and other bugs (will ping the CL when they
pass).

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:105:
image_data_queue_.pop();
On 2015/03/20 00:14:45, dcheng wrote:
> On 2015/03/19 21:54:56, twellington wrote:
> > On 2015/03/19 14:25:46, dcheng wrote:
> > > After thinking about this, I feel uncomfortable with depending on a FIFO
> > > guarantee by Start() to guarantee correctness of the multiplexed requests.
> > > 
> > > I think it's better to either:
> > > 1) Use base::Bind() and bundle all the required state with the bound
> callback.
> > > 2) Disallow the use of a single ImageDispatcher for multiple requests.
> > > 
> > > Of these options, I would prefer #2.
> > > 
> > > #1 requires each call to Start() to bind two callbacks, and instantiates a
> > bunch
> > > of templates that probably won't be folded together, resulting in bloat.
> > > 
> > > #2 is similar to the existing model, except that places like this probably
> > > wouldn't subclass ImageDelegate directly and would have a helper class
> > subclass
> > > it instead. We could also rename ImageDelegate to ImageRequest too (which
is
> > > probably a more appropriate name, since delegate typically implies that
> > there's
> > > only one of them). So we'd end up with something like:
> > > 
> > > class ImageRequest {
> > >  public:
> > >   virtual ~ImageRequest();
> > >   virtual void OnImageDecoded(...) = 0;
> > >   virtual void OnDecodeImageFailed(...) = 0;
> > >   TaskRunner* task_runner() const { return t.get(); }
> > > 
> > >  protected:
> > >   explicit ImageRequest(const scoped_refptr<TaskRunner>& t);
> > > 
> > >  private:
> > >   const scoped_refptr<T> t;
> > > };
> > > 
> > > and then starting/stopping it would look like:
> > > 
> > > ImageDecoder::Start(request);
> > > 
> > > and we could make it automatically cancel on destruction too:
> > > ImageRequest::~ImageRequest() {
> > >   ImageDecoder::Cancel(this);
> > > }
> > > 
> > > (though of course this comes with its own bag of worms WRT thread-safety
> > issues,
> > > see below)
> > 
> > I created a UserImageLoader::ImageRequest subclass that extends
> > ImageDecoder::Delegate; it keeps track of all the info needed in
> OnImageDecoded
> > and OnDecodeImageFailed. The queue is gone :)
> > 
> > I also changed the ImageDecoder::Delegate class to keep track of the task
> > runner; I really like this, because it's one less arg that has to get passed
> > around.
> > 
> > I haven't renamed Delegate -> ImageRequest yet. For cases like this, where a
> new
> > ImageRequest object will get created for each image to decode, "request"
makes
> > sense to me. For most other cases, however, the classes using ImageDecoder
> > directly inherit from ImageDecoder::Delegate; they don't have or create
> multiple
> > delegates, they just are the "delegate". I don't particularly care about the
> > name, though, so if ImageRequest makes more sense with how we generally use
> the
> > terms in Chromium, I'll change it.
> 
> I think it's OK for those single-shot instances to also subclass from
> ImageRequest. Since they are single-shot requests, it makes sense that the
class
> itself subclasses ImageRequest directly (LogoDecoderDelegate is an instance of
> this).

Done.

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/600001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:48: BrowserThread::PostTask(
On 2015/03/20 00:14:45, dcheng wrote:
> On 2015/03/19 21:54:56, twellington wrote:
> > On 2015/03/19 14:25:46, dcheng wrote:
> > > So this is tricky.
> > > 
> > > Due to IDMap's non-thread safe nature, we have to do this on the IO
thread.
> > > 
> > > But the object being destroyed is probably not on the IO thread. Another
> task
> > > might already be on the IO thread, ready to send a callback to the object
> > we're
> > > removing.
> > > 
> > > I guess we're better off rolling our own solution here =/
> > 
> > I could go back to using a std::map and use base::AutoLock
> > lock(request_info_id_map_) in the methods that manipulate the map. Rather
than
> > having this method post a task to the IO thread, it could acquire the lock
and
> > remove the appropriate entry(ies) from the map. This would at least avoid
the
> > issue of the RemoveDelegate request getting tacked to the back of the IO
> > thread's queue of tasks behind an OnImageDecoded callback to the object
we're
> > removing.
> 
> Yeah, I think that's the best idea.
> It might also be nice to have an O(1) cancel, depending on how deeply queued
> this can get.

Changed to a lock protected std::map.

atm, I think how deeply queued this gets depends on what the user is doing. For
most uses of ImageDecoder, we're really only decoding 1 or 2 images. If the user
is in the enhanced bookmark manager scrolling really quickly past hundreds of
bookmarks, this map could have a couple dozen items in it. When I was doing some
performance profiling, the image decode requests for bookmarks only took 2-3
milliseconds. While the user is scrolling the requests queue up, some get
processed and taken out of the map, more requests queue up, etc. (so even if
you're scrolling past hundreds of bookmarks, hundreds of requests don't get
queued because there's this queue-process-queue-process sort of thing
happening). I can do some more tests around how big this map gets if that would
be useful.

Going forward, though, I don't think we can make strong guarantees about how big
this map gets. I could keep a second std::unordered_map of ImageRequest* -> list
of id's to make Cancel closer to O(1). Maybe the image_request_id_map_ should be
a std::unordered_map for O(1) lookups as well?

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:94:
ImageRequest* image_request = new ImageRequest(image_info, *data, this);
On 2015/03/20 00:14:45, dcheng wrote:
> Where is this freed?

It wasn't. I fixed it to free in OnDecodeImageFailed and OnImageDecoded.

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:36: ImageDecoder::Delegate::~Delegate() {
On 2015/03/20 00:14:45, dcheng wrote:
> Call RemoveDelegate(this) here. Deleting this should automatically cancel.
This
> removes the need for subclasses like LogoDecoderDelegate to manually call it.

Done.

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/640001/chrome/browser/image_de...
chrome/browser/image_decoder.h:64: static void RemoveDelegate(Delegate*
delegate);
On 2015/03/20 00:14:45, dcheng wrote:
> Cancel() is probably a clearer name for this.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:63: // Removes all instances of image_request
from the IDMap, ensuring callbacks
Note for self: update this comment to no longer refer to IDMap

[Theresa, 2015-03-20 22:49:58]

On 2015/03/20 19:09:01, twellington wrote:
> dcheng@ - I have one question in image_decoder.cc about O(1) Cancel, but
> otherwise addressed all comments in the latest patchset. I'm running memory
> tests right now to check for leaks and other bugs (will ping the CL when they
> pass).

ptal, tryjobs all passed (except linux_chromium_chromeos_asan_rel, but the
failures I'm seeing have been happening for everyone w/ and w/out patchset)

[dcheng, 2015-03-23 11:52:10]

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_data.cc:226: IconImageRequest(const
scoped_refptr<base::SequencedTaskRunner>& t,
FWIW, some reviewers might prefer this be named a bit more explicitly (e.g.
task_runner vs t).

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc
(right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc:103:
ImageDecoder::ImageRequest(
I think you should be able to just say ImageRequest here.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:8: #include
<queue>
I think you can remove this #include.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:76: // invoked
This comment needs to be updated / rewrapped.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:81: const
ImageInfo image_info_;
Convention is to make these private and use accessors.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:82:
std::vector<unsigned char> image_data_;
Having to copy this is pretty sad. But I don't see a good way around it, given
the new interface...

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/extensio...
File chrome/browser/extensions/bundle_installer.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/extensio...
chrome/browser/extensions/bundle_installer.cc:223: this, i->first,
i->second.manifest, GURL(), NULL);
Since we're changing this line anyway, mind updating this to nullptr?

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:100:
ImageDecoder::Start(this, response_data, ImageDecoder::DEFAULT_CODEC,
If I understand this correctly, this means we probably start another utility
host that's not shared with the install helper. This is OK with the extensions
folks, right?

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:17: base::LazyInstance<ImageDecoder>::Leaky
image_decoder_instance_ =
You /could/ make this a base::LazyInstance<scoped_refptr<ImageDecoder>>.

It's a little horrible, but it gets rid of the boilerplate class and is not
without precedent.

Yes.
Ick.

However, the downside is this results requires two checks per call to Start() /
Cancel(): the implicit one by LazyInstance<T>, and then the check by Start() /
Cancel() to set the ImageDecoder in the scoped_refptr if it doesn't already
exist. In general, it looks like people only use this pattern when they need to
pass the constructor some arguments.


... instead of that, maybe we could do this instead:
base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance = ...

And then in ImageDecoder()'s constructor, we can leak a ref so we guarantee it's
never deleted.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:77: if (!batch_mode_started_) {
Can we just check that utility_process_host_ is non null here? This should also
handle the case if the utility process crashes for some reason.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:84: std::pair<int,
ImageRequest*>(image_request_id_counter_, image_request));
Use std::make_pair so you don't explicitly need to name the types.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:95: default:
You should be OK omitting this; adding a new enum that's not handled will
generate a compile warning/error that way.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:106: for (RequestMap::iterator it =
image_request_id_map_.begin();
Use a foreach here to save some typing as well.

Alternatively, you can remove the need for two loops by doing this:

for (auto it = image_request_id_map_.begin(); it != image_request_id_map_.end();
) {
  if (it->second == image_request) {
    image_request_id_map_.erase(it++);
  } else {
    ++it;
  }
}

Also, ++it is generally preferred to it++ if the behavior of the post-increment
version is not required. It can be slightly more efficient, since it++ requires
a copy by nature (though I think that can be often optimized away).

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:161: if (image_request_id_map_.find(id) !=
image_request_id_map_.end()) {
It's a pretty common idiom to do this:
auto it = image_request_id_map_.find(id);
if (it != image_request_id_map_.end()) {
  ImageRequest* image_request = it->second;
  ...
  image_request_id_map_.erase(it);
}

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:48: const
scoped_refptr<base::SequencedTaskRunner> t;
Members should definitely have a descriptive name (task_runner_). Sorry that my
example snippet implied otherwise. =(

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:69: static ImageDecoder* GetInstance();
This should be unnecessary to expose in the header now.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:112: base::Lock map_lock_;
This needs to protect the counter as well, since Start() is callable on any
thread (this appears to be the case already, but let's explicitly mention that
here)

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:125: const base::TimeDelta kBatchModeTimeout =
base::TimeDelta::FromSeconds(5);
Just move this constant into the .cc, since it's an implementation detail.

Also, doesn't this introduce a new static initializer? I'm surprised the bots
are OK with this.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/profiles...
File chrome/browser/profiles/profile_downloader.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/profiles...
chrome/browser/profiles/profile_downloader.cc:196: : ImageDecoder::ImageRequest(
ImageRequest should be sufficient here.

https://codereview.chromium.org/931993002/diff/680001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:232: const std::vector<unsigned
char>& encoded_data, bool shrink_to_fit, int id){
Nit: space between ) and {.

https://codereview.chromium.org/931993002/diff/680001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:244: const std::vector<unsigned
char>& encoded_data, bool shrink_to_fit, int id){
Ditto.

[Theresa, 2015-03-23 17:33:11]

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_data.cc:226: IconImageRequest(const
scoped_refptr<base::SequencedTaskRunner>& t,
On 2015/03/23 11:52:08, dcheng wrote:
> FWIW, some reviewers might prefer this be named a bit more explicitly (e.g.
> task_runner vs t).

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc
(right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc:103:
ImageDecoder::ImageRequest(
On 2015/03/23 11:52:08, dcheng wrote:
> I think you should be able to just say ImageRequest here.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:8: #include
<queue>
On 2015/03/23 11:52:09, dcheng wrote:
> I think you can remove this #include.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:76: // invoked
On 2015/03/23 11:52:08, dcheng wrote:
> This comment needs to be updated / rewrapped.

I rewrapped. I think the comment is still accurate, though. What did you think
should be updated?

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:81: const
ImageInfo image_info_;
On 2015/03/23 11:52:08, dcheng wrote:
> Convention is to make these private and use accessors.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:82:
std::vector<unsigned char> image_data_;
On 2015/03/23 11:52:08, dcheng wrote:
> Having to copy this is pretty sad. But I don't see a good way around it, given
> the new interface...

Acknowledged.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/extensio...
File chrome/browser/extensions/bundle_installer.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/extensio...
chrome/browser/extensions/bundle_installer.cc:223: this, i->first,
i->second.manifest, GURL(), NULL);
On 2015/03/23 11:52:09, dcheng wrote:
> Since we're changing this line anyway, mind updating this to nullptr?

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:100:
ImageDecoder::Start(this, response_data, ImageDecoder::DEFAULT_CODEC,
On 2015/03/23 11:52:09, dcheng wrote:
> If I understand this correctly, this means we probably start another utility
> host that's not shared with the install helper. This is OK with the extensions
> folks, right?

The webstore install helper has a utility process it sends ParseJSON messages
to; that utility process is separate from the ImageDecoder utility process. This
file was on the list of ones for Antony to review, so yes, I think it's OK with
the extensions folks, but I can explicitly ask if needed.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:17: base::LazyInstance<ImageDecoder>::Leaky
image_decoder_instance_ =
On 2015/03/23 11:52:09, dcheng wrote:
> You /could/ make this a base::LazyInstance<scoped_refptr<ImageDecoder>>.
> 
> It's a little horrible, but it gets rid of the boilerplate class and is not
> without precedent.
> 
> Yes.
> Ick.
> 
> However, the downside is this results requires two checks per call to Start()
/
> Cancel(): the implicit one by LazyInstance<T>, and then the check by Start() /
> Cancel() to set the ImageDecoder in the scoped_refptr if it doesn't already
> exist. In general, it looks like people only use this pattern when they need
to
> pass the constructor some arguments.
> 
> 
> ... instead of that, maybe we could do this instead:
> base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance = ...
> 
> And then in ImageDecoder()'s constructor, we can leak a ref so we guarantee
it's
> never deleted.

I'm not sure that a reference needs to be leaked. The LazyInstance documentation
says "When the instance is constructed it is registered with AtExitManager. The
destructor will be called on program exit." ImageDecoder isn't refcounted atm,
and its destructor is private, so its destructor shouldn't be getting called any
time except at program exit.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:77: if (!batch_mode_started_) {
On 2015/03/23 11:52:09, dcheng wrote:
> Can we just check that utility_process_host_ is non null here? This should
also
> handle the case if the utility process crashes for some reason.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:84: std::pair<int,
ImageRequest*>(image_request_id_counter_, image_request));
On 2015/03/23 11:52:09, dcheng wrote:
> Use std::make_pair so you don't explicitly need to name the types.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:95: default:
On 2015/03/23 11:52:09, dcheng wrote:
> You should be OK omitting this; adding a new enum that's not handled will
> generate a compile warning/error that way.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:106: for (RequestMap::iterator it =
image_request_id_map_.begin();
On 2015/03/23 11:52:09, dcheng wrote:
> Use a foreach here to save some typing as well.
> 
> Alternatively, you can remove the need for two loops by doing this:
> 
> for (auto it = image_request_id_map_.begin(); it !=
image_request_id_map_.end();
> ) {
>   if (it->second == image_request) {
>     image_request_id_map_.erase(it++);
>   } else {
>     ++it;
>   }
> }
> 
> Also, ++it is generally preferred to it++ if the behavior of the
post-increment
> version is not required. It can be slightly more efficient, since it++
requires
> a copy by nature (though I think that can be often optimized away).

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:161: if (image_request_id_map_.find(id) !=
image_request_id_map_.end()) {
On 2015/03/23 11:52:09, dcheng wrote:
> It's a pretty common idiom to do this:
> auto it = image_request_id_map_.find(id);
> if (it != image_request_id_map_.end()) {
>   ImageRequest* image_request = it->second;
>   ...
>   image_request_id_map_.erase(it);
> }

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:48: const
scoped_refptr<base::SequencedTaskRunner> t;
On 2015/03/23 11:52:09, dcheng wrote:
> Members should definitely have a descriptive name (task_runner_). Sorry that
my
> example snippet implied otherwise. =(

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:63: // Removes all instances of image_request
from the IDMap, ensuring callbacks
On 2015/03/20 19:09:01, twellington wrote:
> Note for self: update this comment to no longer refer to IDMap

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:69: static ImageDecoder* GetInstance();
On 2015/03/23 11:52:09, dcheng wrote:
> This should be unnecessary to expose in the header now.

I think it still needs to be declared here before it's defined in the c++ file.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:112: base::Lock map_lock_;
On 2015/03/23 11:52:09, dcheng wrote:
> This needs to protect the counter as well, since Start() is callable on any
> thread (this appears to be the case already, but let's explicitly mention that
> here)

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:125: const base::TimeDelta kBatchModeTimeout =
base::TimeDelta::FromSeconds(5);
On 2015/03/23 11:52:09, dcheng wrote:
> Just move this constant into the .cc, since it's an implementation detail.
> 
> Also, doesn't this introduce a new static initializer? I'm surprised the bots
> are OK with this.

Moved. I don't think it introduced a static initializer (nothing in the
ImageDecoderImpl is static), but I'm honestly not sure.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/profiles...
File chrome/browser/profiles/profile_downloader.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/profiles...
chrome/browser/profiles/profile_downloader.cc:196: : ImageDecoder::ImageRequest(
On 2015/03/23 11:52:09, dcheng wrote:
> ImageRequest should be sufficient here.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:232: const std::vector<unsigned
char>& encoded_data, bool shrink_to_fit, int id){
On 2015/03/23 11:52:09, dcheng wrote:
> Nit: space between ) and {.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:244: const std::vector<unsigned
char>& encoded_data, bool shrink_to_fit, int id){
On 2015/03/23 11:52:09, dcheng wrote:
> Ditto.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:244: const std::vector<unsigned
char>& encoded_data, bool shrink_to_fit, int id){
On 2015/03/23 11:52:09, dcheng wrote:
> Ditto.

Done.

[Theresa, 2015-03-23 18:46:37]

ptal

[dcheng, 2015-03-24 05:18:19]

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:76: // invoked
On 2015/03/23 17:33:09, twellington wrote:
> On 2015/03/23 11:52:08, dcheng wrote:
> > This comment needs to be updated / rewrapped.
> 
> I rewrapped. I think the comment is still accurate, though. What did you think
> should be updated?

This class has no background_task_runner_ member itself, so it might be a bit
confusing to reference it.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:17: base::LazyInstance<ImageDecoder>::Leaky
image_decoder_instance_ =
On 2015/03/23 17:33:10, twellington wrote:
> On 2015/03/23 11:52:09, dcheng wrote:
> > You /could/ make this a base::LazyInstance<scoped_refptr<ImageDecoder>>.
> > 
> > It's a little horrible, but it gets rid of the boilerplate class and is not
> > without precedent.
> > 
> > Yes.
> > Ick.
> > 
> > However, the downside is this results requires two checks per call to
Start()
> /
> > Cancel(): the implicit one by LazyInstance<T>, and then the check by Start()
/
> > Cancel() to set the ImageDecoder in the scoped_refptr if it doesn't already
> > exist. In general, it looks like people only use this pattern when they need
> to
> > pass the constructor some arguments.
> > 
> > 
> > ... instead of that, maybe we could do this instead:
> > base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance = ...
> > 
> > And then in ImageDecoder()'s constructor, we can leak a ref so we guarantee
> it's
> > never deleted.
> 
> I'm not sure that a reference needs to be leaked. The LazyInstance
documentation
> says "When the instance is constructed it is registered with AtExitManager.
The
> destructor will be called on program exit." ImageDecoder isn't refcounted atm,
> and its destructor is private, so its destructor shouldn't be getting called
any
> time except at program exit.

To clarify, I'm proposing that we eliminate the ImageDecoder / ImageDecoderImpl
wrapping and just lift the ImageDecoderImpl implementation into the parent
class.

Then, to make sure that the ImageDecoder isn't accidentally destroyed, we need
to leak a ref in ImageDecoder's ctor, since refcounted objects start with a
refcount of 0.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:125: const base::TimeDelta kBatchModeTimeout =
base::TimeDelta::FromSeconds(5);
On 2015/03/23 17:33:10, twellington wrote:
> On 2015/03/23 11:52:09, dcheng wrote:
> > Just move this constant into the .cc, since it's an implementation detail.
> > 
> > Also, doesn't this introduce a new static initializer? I'm surprised the
bots
> > are OK with this.
> 
> Moved. I don't think it introduced a static initializer (nothing in the
> ImageDecoderImpl is static), but I'm honestly not sure.

I've confirmed locally that this does introduce a static initializer.

In general, any global variable of non-POD type will introduce a static
initializer (LazyInstance and Singleton are very carefully written to avoid
generating static initializers).

The usual idiom is:
const int kBatchModeTimeoutInSeconds = 5;
if ((x - y) > base::TimeDelta::FromSeconds(kBatchModeTimeoutInSeconds)) {
  // foo
}

https://codereview.chromium.org/931993002/diff/720001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/931993002/diff/720001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:37: :
ImageDecoder::ImageRequest(
Just ImageRequest.

[Theresa, 2015-03-24 23:51:43]

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.h:76: // invoked
On 2015/03/24 05:18:18, dcheng wrote:
> On 2015/03/23 17:33:09, twellington wrote:
> > On 2015/03/23 11:52:08, dcheng wrote:
> > > This comment needs to be updated / rewrapped.
> > 
> > I rewrapped. I think the comment is still accurate, though. What did you
think
> > should be updated?
> 
> This class has no background_task_runner_ member itself, so it might be a bit
> confusing to reference it.

Done.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:17: base::LazyInstance<ImageDecoder>::Leaky
image_decoder_instance_ =
On 2015/03/24 05:18:18, dcheng wrote:
> On 2015/03/23 17:33:10, twellington wrote:
> > On 2015/03/23 11:52:09, dcheng wrote:
> > > You /could/ make this a base::LazyInstance<scoped_refptr<ImageDecoder>>.
> > > 
> > > It's a little horrible, but it gets rid of the boilerplate class and is
not
> > > without precedent.
> > > 
> > > Yes.
> > > Ick.
> > > 
> > > However, the downside is this results requires two checks per call to
> Start()
> > /
> > > Cancel(): the implicit one by LazyInstance<T>, and then the check by
Start()
> /
> > > Cancel() to set the ImageDecoder in the scoped_refptr if it doesn't
already
> > > exist. In general, it looks like people only use this pattern when they
need
> > to
> > > pass the constructor some arguments.
> > > 
> > > 
> > > ... instead of that, maybe we could do this instead:
> > > base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance = ...
> > > 
> > > And then in ImageDecoder()'s constructor, we can leak a ref so we
guarantee
> > it's
> > > never deleted.
> > 
> > I'm not sure that a reference needs to be leaked. The LazyInstance
> documentation
> > says "When the instance is constructed it is registered with AtExitManager.
> The
> > destructor will be called on program exit." ImageDecoder isn't refcounted
atm,
> > and its destructor is private, so its destructor shouldn't be getting called
> any
> > time except at program exit.
> 
> To clarify, I'm proposing that we eliminate the ImageDecoder /
ImageDecoderImpl
> wrapping and just lift the ImageDecoderImpl implementation into the parent
> class.
> 
> Then, to make sure that the ImageDecoder isn't accidentally destroyed, we need
> to leak a ref in ImageDecoder's ctor, since refcounted objects start with a
> refcount of 0.

Done. Thank you for the clarification :) I should have picked up on that.

To "leak" a reference, I just called AddRef() in the constructor. Is that the
appropriate way to make sure it's not accidentally destroyed or is there a
different pattern we use?

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:125: const base::TimeDelta kBatchModeTimeout =
base::TimeDelta::FromSeconds(5);
On 2015/03/24 05:18:18, dcheng wrote:
> On 2015/03/23 17:33:10, twellington wrote:
> > On 2015/03/23 11:52:09, dcheng wrote:
> > > Just move this constant into the .cc, since it's an implementation detail.
> > > 
> > > Also, doesn't this introduce a new static initializer? I'm surprised the
> bots
> > > are OK with this.
> > 
> > Moved. I don't think it introduced a static initializer (nothing in the
> > ImageDecoderImpl is static), but I'm honestly not sure.
> 
> I've confirmed locally that this does introduce a static initializer.
> 
> In general, any global variable of non-POD type will introduce a static
> initializer (LazyInstance and Singleton are very carefully written to avoid
> generating static initializers).
> 
> The usual idiom is:
> const int kBatchModeTimeoutInSeconds = 5;
> if ((x - y) > base::TimeDelta::FromSeconds(kBatchModeTimeoutInSeconds)) {
>   // foo
> }

If it's in image_decoder.cc, do the static intializers still get created? I
could also change the const back to an int and use it to create the TimeDelta in
 image_decoder.cc.

https://codereview.chromium.org/931993002/diff/720001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/931993002/diff/720001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:37: :
ImageDecoder::ImageRequest(
On 2015/03/24 05:18:18, dcheng wrote:
> Just ImageRequest.

Done.

[dcheng, 2015-03-25 14:10:10]

Overall, LGTM with the comments addressed (but you'll need owners approval since
I'm not an owner for anything except the IPC files).

My only real concern is the O(n) scaling of Cancel(). Making this O(1) is
probably doesn't do much most of the time, but I'd probably still lean towards
doing it, since ImageRequest auto-cancels on deletion.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:17: base::LazyInstance<ImageDecoder>::Leaky
image_decoder_instance_ =
On 2015/03/24 23:51:42, twellington wrote:
> On 2015/03/24 05:18:18, dcheng wrote:
> > On 2015/03/23 17:33:10, twellington wrote:
> > > On 2015/03/23 11:52:09, dcheng wrote:
> > > > You /could/ make this a base::LazyInstance<scoped_refptr<ImageDecoder>>.
> > > > 
> > > > It's a little horrible, but it gets rid of the boilerplate class and is
> not
> > > > without precedent.
> > > > 
> > > > Yes.
> > > > Ick.
> > > > 
> > > > However, the downside is this results requires two checks per call to
> > Start()
> > > /
> > > > Cancel(): the implicit one by LazyInstance<T>, and then the check by
> Start()
> > /
> > > > Cancel() to set the ImageDecoder in the scoped_refptr if it doesn't
> already
> > > > exist. In general, it looks like people only use this pattern when they
> need
> > > to
> > > > pass the constructor some arguments.
> > > > 
> > > > 
> > > > ... instead of that, maybe we could do this instead:
> > > > base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance = ...
> > > > 
> > > > And then in ImageDecoder()'s constructor, we can leak a ref so we
> guarantee
> > > it's
> > > > never deleted.
> > > 
> > > I'm not sure that a reference needs to be leaked. The LazyInstance
> > documentation
> > > says "When the instance is constructed it is registered with
AtExitManager.
> > The
> > > destructor will be called on program exit." ImageDecoder isn't refcounted
> atm,
> > > and its destructor is private, so its destructor shouldn't be getting
called
> > any
> > > time except at program exit.
> > 
> > To clarify, I'm proposing that we eliminate the ImageDecoder /
> ImageDecoderImpl
> > wrapping and just lift the ImageDecoderImpl implementation into the parent
> > class.
> > 
> > Then, to make sure that the ImageDecoder isn't accidentally destroyed, we
need
> > to leak a ref in ImageDecoder's ctor, since refcounted objects start with a
> > refcount of 0.
> 
> Done. Thank you for the clarification :) I should have picked up on that.
> 
> To "leak" a reference, I just called AddRef() in the constructor. Is that the
> appropriate way to make sure it's not accidentally destroyed or is there a
> different pattern we use?

Yep, that's the best way.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:125: const base::TimeDelta kBatchModeTimeout =
base::TimeDelta::FromSeconds(5);
On 2015/03/24 23:51:42, twellington wrote:
> On 2015/03/24 05:18:18, dcheng wrote:
> > On 2015/03/23 17:33:10, twellington wrote:
> > > On 2015/03/23 11:52:09, dcheng wrote:
> > > > Just move this constant into the .cc, since it's an implementation
detail.
> > > > 
> > > > Also, doesn't this introduce a new static initializer? I'm surprised the
> > bots
> > > > are OK with this.
> > > 
> > > Moved. I don't think it introduced a static initializer (nothing in the
> > > ImageDecoderImpl is static), but I'm honestly not sure.
> > 
> > I've confirmed locally that this does introduce a static initializer.
> > 
> > In general, any global variable of non-POD type will introduce a static
> > initializer (LazyInstance and Singleton are very carefully written to avoid
> > generating static initializers).
> > 
> > The usual idiom is:
> > const int kBatchModeTimeoutInSeconds = 5;
> > if ((x - y) > base::TimeDelta::FromSeconds(kBatchModeTimeoutInSeconds)) {
> >   // foo
> > }
> 
> If it's in image_decoder.cc, do the static intializers still get created? I
> could also change the const back to an int and use it to create the TimeDelta
in
>  image_decoder.cc.

Yeah, it will still create a static initializer =/

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:17: base::LazyInstance<ImageDecoder>::Leaky
image_decoder_instance_ =
Put this in an unnamed namespace. Also you can probably call it something
shorter, like g_decoder or decoder, since it's not exposed outside this file
anyway.

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:154: image_request_id_map_.erase(id);
erase(it)

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:168: image_request_id_map_.erase(id);
erase(it)

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
chrome/browser/image_decoder.h:60: // posted back to |task_runner|.
Update this comment.

[Theresa, 2015-03-25 17:13:45]

Addressed comments from dcheng@ review. I didn't change Cancel() to be O(1). I'm
leaning toward leaving it as is and optimizing in the future if its needed for
performance reasons. Right now, I don't think the RequestMap will be  large
enough to warrant the additional code complexity of maintaining two hashmaps.

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/680001/chrome/browser/image_de...
chrome/browser/image_decoder.h:125: const base::TimeDelta kBatchModeTimeout =
base::TimeDelta::FromSeconds(5);
On 2015/03/25 14:10:09, dcheng wrote:
> On 2015/03/24 23:51:42, twellington wrote:
> > On 2015/03/24 05:18:18, dcheng wrote:
> > > On 2015/03/23 17:33:10, twellington wrote:
> > > > On 2015/03/23 11:52:09, dcheng wrote:
> > > > > Just move this constant into the .cc, since it's an implementation
> detail.
> > > > > 
> > > > > Also, doesn't this introduce a new static initializer? I'm surprised
the
> > > bots
> > > > > are OK with this.
> > > > 
> > > > Moved. I don't think it introduced a static initializer (nothing in the
> > > > ImageDecoderImpl is static), but I'm honestly not sure.
> > > 
> > > I've confirmed locally that this does introduce a static initializer.
> > > 
> > > In general, any global variable of non-POD type will introduce a static
> > > initializer (LazyInstance and Singleton are very carefully written to
avoid
> > > generating static initializers).
> > > 
> > > The usual idiom is:
> > > const int kBatchModeTimeoutInSeconds = 5;
> > > if ((x - y) > base::TimeDelta::FromSeconds(kBatchModeTimeoutInSeconds)) {
> > >   // foo
> > > }
> > 
> > If it's in image_decoder.cc, do the static intializers still get created? I
> > could also change the const back to an int and use it to create the
TimeDelta
> in
> >  image_decoder.cc.
> 
> Yeah, it will still create a static initializer =/

Done. Changed back to a const int.

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:17: base::LazyInstance<ImageDecoder>::Leaky
image_decoder_instance_ =
On 2015/03/25 14:10:09, dcheng wrote:
> Put this in an unnamed namespace. Also you can probably call it something
> shorter, like g_decoder or decoder, since it's not exposed outside this file
> anyway.

Done.

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:154: image_request_id_map_.erase(id);
On 2015/03/25 14:10:09, dcheng wrote:
> erase(it)

Done.

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:168: image_request_id_map_.erase(id);
On 2015/03/25 14:10:09, dcheng wrote:
> erase(it)

Done.

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/740001/chrome/browser/image_de...
chrome/browser/image_decoder.h:60: // posted back to |task_runner|.
On 2015/03/25 14:10:09, dcheng wrote:
> Update this comment.

Done.

[Theresa, 2015-03-25 17:20:09]

twellington@chromium.org changed reviewers:
+ erg@chromium.org, tedchoc@chromium.org, thestig@chromium.org,
xiyuan@chromium.org

[Theresa, 2015-03-25 17:20:10]

Adding owners for classes using the refactored ImageDecoder.

ptal - ImageDecoder has been significantly refactored, requiring updates to the
classes that use it. It's a large CL, but nobody is on the hook for reviewing
the whole thing. Please the see list of packages per reviewer below:

xiyuan@ - browser/apps/, browser/chromeos/app_mode/ browser/chromeos/app_login/,
browser/ui/

erg@ - browser/profiles/

tedchoc@ - browser/android/

thestig@ - browser/media_galleries/, bitmap_fetcher classes, and image_decoder
(which has already been extensively reviewed and reworked, but needs approval by
an owner)

already lgtm'ed - utility/, common/, browser/extensions/,
browser/chromeos/extensions

[Elliot Glaysher, 2015-03-25 17:27:20]

profiles lgtm

[xiyuan, 2015-03-25 18:07:28]

browser/apps/,
browser/chromeos/app_mode/
browser/chromeos/login/,
browser/ui/

LGTM

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:56: }
nit: move UserImageLoader::UserImageRequest methods together, either move this
down or move OnImageDecoded etc up.

[Ted C, 2015-03-25 18:42:22]

On 2015/03/25 18:07:28, xiyuan wrote:
> browser/apps/,
> browser/chromeos/app_mode/
> browser/chromeos/login/,
> browser/ui/
> 
> LGTM
> 
>
https://codereview.chromium.org/931993002/diff/760001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):
> 
>
https://codereview.chromium.org/931993002/diff/760001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:56: }
> nit: move UserImageLoader::UserImageRequest methods together, either move this
> down or move OnImageDecoded etc up.

browser/android/ - lgtm

[Lei Zhang, 2015-03-25 21:44:47]

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:16: namespace {
nit: add a blank line after this and before the end of the anonymous namespace.
There should be 2 spaces in the ending comment.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:18: base::LazyInstance<ImageDecoder>::Leaky
g_decoder_ = LAZY_INSTANCE_INITIALIZER;
nit: just |g_decoder| since it's not a member variable.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:67:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
DCHECK_CURRENTLY_ON()

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:72: last_request_ = base::TimeTicks::Now();
If |batch_mode_timer_| is running, can't you use base::Timer::Reset() here,
instead of tracking a separate |last_request_| ?

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:26: // Internally, all of the work happens on the
IO thread, and then
From just looking at the header file, it's not obvious why we need |map_lock_|
when everything should be single-threaded internally.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:27: // the result (ImageRequest::OnImageDecoded
or
This part is a bit confusing - OnImageDecoded is not the result.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:41: base::SequencedTaskRunner* task_runner()
const {
Does this need to be public?

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:51: const
scoped_refptr<base::SequencedTaskRunner> task_runner_;
Document this, mention it's the thread to post back to.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:60: // posted back to image_request's
task_runner_.
nit: |task_runner|, ditto below.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:61: static void Start(ImageRequest*
image_request,
Since (ImageDecoder::DEFAULT_CODEC, false) are the most common arguments to
(image_codec, shrink_to_fit), you can rename this to StartWithOptions() and have
Start() omit those two parameters.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:71: friend struct
base::DefaultLazyInstanceTraits<ImageDecoder>;
nit: blank line after this

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:85: using RequestMap = std::map<int,
ImageRequest*>;
Is this the new preferred method vs. using a typedef?

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:97: void OnDecodeImageSucceeded(const SkBitmap&
decoded_image, int id);
nit: id -> request_id?

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:120: const int kBatchModeTimeoutSeconds = 5;
Can this just go into the .cc file?

[Theresa, 2015-03-25 23:13:16]

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:56: }
On 2015/03/25 18:07:28, xiyuan wrote:
> nit: move UserImageLoader::UserImageRequest methods together, either move this
> down or move OnImageDecoded etc up.

Done.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:16: namespace {
On 2015/03/25 21:44:46, Lei Zhang wrote:
> nit: add a blank line after this and before the end of the anonymous
namespace.
> There should be 2 spaces in the ending comment.

Done.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:18: base::LazyInstance<ImageDecoder>::Leaky
g_decoder_ = LAZY_INSTANCE_INITIALIZER;
On 2015/03/25 21:44:46, Lei Zhang wrote:
> nit: just |g_decoder| since it's not a member variable.

Done.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:67:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2015/03/25 21:44:46, Lei Zhang wrote:
> DCHECK_CURRENTLY_ON()

Done.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:72: last_request_ = base::TimeTicks::Now();
On 2015/03/25 21:44:46, Lei Zhang wrote:
> If |batch_mode_timer_| is running, can't you use base::Timer::Reset() here,
> instead of tracking a separate |last_request_| ?

Yes, Reset() would effectively do the same thing. I chose not to use it, because
for some use cases (e.g. scrolling through bookmarks), hundreds of image
decoding requests may come in at once and calling Reset on the timer each time
would cause more churn than checking if batch mode should be stopped every 5
seconds.

The code for Timer::Reset() doesn't look like it has super high overhead if
there is an existing scheduled task that can be reused, though, so I'm not
opposed to changing this to a OneShotTimer w/ calls to Reset().

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:26: // Internally, all of the work happens on the
IO thread, and then
On 2015/03/25 21:44:46, Lei Zhang wrote:
> From just looking at the header file, it's not obvious why we need |map_lock_|
> when everything should be single-threaded internally.

Updated the comment. Cancel() doesn't run on the IO thread. Cancel() is
automatically called when classes extending ImageRequest are destroyed.
Previously Cancel() did run on the IO thread, but there was concern that the
Cancel() request would be tacked to the back of the IO thread's processing
queue, behind callbacks to the object that was trying to delete itself.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:27: // the result (ImageRequest::OnImageDecoded
or
On 2015/03/25 21:44:46, Lei Zhang wrote:
> This part is a bit confusing - OnImageDecoded is not the result.

I changed "result" to "callback"

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:41: base::SequencedTaskRunner* task_runner()
const {
On 2015/03/25 21:44:46, Lei Zhang wrote:
> Does this need to be public?

ImageDecoder needs to be able to call it, and it's used in user_image_loader.cc
for a couple of DCHECKs.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:51: const
scoped_refptr<base::SequencedTaskRunner> task_runner_;
On 2015/03/25 21:44:46, Lei Zhang wrote:
> Document this, mention it's the thread to post back to.

Done.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:60: // posted back to image_request's
task_runner_.
On 2015/03/25 21:44:46, Lei Zhang wrote:
> nit: |task_runner|, ditto below.

Is the nit to rename task_runner_ to task_runner (without the trailing '_') or
to encase references to member variables in |'s?

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:61: static void Start(ImageRequest*
image_request,
On 2015/03/25 21:44:46, Lei Zhang wrote:
> Since (ImageDecoder::DEFAULT_CODEC, false) are the most common arguments to
> (image_codec, shrink_to_fit), you can rename this to StartWithOptions() and
have
> Start() omit those two parameters.

Done.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:71: friend struct
base::DefaultLazyInstanceTraits<ImageDecoder>;
On 2015/03/25 21:44:46, Lei Zhang wrote:
> nit: blank line after this

Done.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:85: using RequestMap = std::map<int,
ImageRequest*>;
On 2015/03/25 21:44:46, Lei Zhang wrote:
> Is this the new preferred method vs. using a typedef?

Yes, unless the header needs to be compatible with C
(https://chromium-cpp.appspot.com/)

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:97: void OnDecodeImageSucceeded(const SkBitmap&
decoded_image, int id);
On 2015/03/25 21:44:46, Lei Zhang wrote:
> nit: id -> request_id?

Done.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:120: const int kBatchModeTimeoutSeconds = 5;
On 2015/03/25 21:44:46, Lei Zhang wrote:
> Can this just go into the .cc file?

Done.

[Lei Zhang, 2015-03-25 23:28:38]

lgtm

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:60: // posted back to image_request's
task_runner_.
On 2015/03/25 23:13:15, twellington wrote:
> On 2015/03/25 21:44:46, Lei Zhang wrote:
> > nit: |task_runner|, ditto below.
> 
> Is the nit to rename task_runner_ to task_runner (without the trailing '_') or
> to encase references to member variables in |'s?

Refer to variables as |variable|.

https://codereview.chromium.org/931993002/diff/780001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/780001/chrome/browser/image_de...
chrome/browser/image_decoder.h:31: // The Cancel() method does not run on the IO
thread. |map_lock_| is used to
Can you document what thread Cancel() runs on?

[Theresa, 2015-03-25 23:54:09]

Will land in the morning if there are no objections.

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/760001/chrome/browser/image_de...
chrome/browser/image_decoder.h:60: // posted back to image_request's
task_runner_.
On 2015/03/25 23:28:38, Lei Zhang wrote:
> On 2015/03/25 23:13:15, twellington wrote:
> > On 2015/03/25 21:44:46, Lei Zhang wrote:
> > > nit: |task_runner|, ditto below.
> > 
> > Is the nit to rename task_runner_ to task_runner (without the trailing '_')
or
> > to encase references to member variables in |'s?
> 
> Refer to variables as |variable|.

Done.

https://codereview.chromium.org/931993002/diff/780001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/931993002/diff/780001/chrome/browser/image_de...
chrome/browser/image_decoder.h:31: // The Cancel() method does not run on the IO
thread. |map_lock_| is used to
On 2015/03/25 23:28:38, Lei Zhang wrote:
> Can you document what thread Cancel() runs on?

Done.

[Theresa, 2015-03-26 14:06:28]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2015-03-26 14:06:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, dcheng@chromium.org, xiyuan@chromium.org,
tedchoc@chromium.org, erg@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/931993002/#ps800001
(title: "Fix a few comments")

[commit-bot: I haz the power, 2015-03-26 14:06:43]

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

[commit-bot: I haz the power, 2015-03-26 15:10:13]

Committed patchset #41 (id:800001)

[commit-bot: I haz the power, 2015-03-26 15:10:46]

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