Add possibility to define data callback to BitmapFetcher

Add possibility to define data callback to BitmapFetcher

BUG=479715
Committed: https://crrev.com/58c4b6fc734361e9de25959c7f92690e8f5ba372
Cr-Commit-Position: refs/heads/master@{#333962}

[wmaslowski, 2015-04-22 13:32:30]

wmaslowski@opera.com changed reviewers:
+ dschuyler@chromium.org, twellington@chromium.org

[Theresa, 2015-04-22 23:07:06]

lgtm

https://codereview.chromium.org/1097383005/diff/1/chrome/browser/bitmap_fetch...
File chrome/browser/bitmap_fetcher/bitmap_fetcher.h (right):

https://codereview.chromium.org/1097383005/diff/1/chrome/browser/bitmap_fetch...
chrome/browser/bitmap_fetcher/bitmap_fetcher.h:45: // and create data callback
for URLFetcher it creates.
The phrasing here is a little unclear, maybe something like:

Like BitmapFetcher::Start above except it sets the |user_data_key| and
|create_data_callback| that are used when setting the user data on any URL
objects the fetcher creates.

[Peter Kasting, 2015-04-22 23:16:19]

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

[Peter Kasting, 2015-04-22 23:16:20]

Any chance you can motivate why you need to be able to set this user data?

https://codereview.chromium.org/1097383005/diff/1/chrome/browser/bitmap_fetch...
File chrome/browser/bitmap_fetcher/bitmap_fetcher.cc (right):

https://codereview.chromium.org/1097383005/diff/1/chrome/browser/bitmap_fetch...
chrome/browser/bitmap_fetcher/bitmap_fetcher.cc:38: void BitmapFetcher::Start(
I'm not a fan of having these two methods which have almost-identical args and
code.  And what happens in the future when someone wants to set some other piece
of info on the underlying URL fetcher?

Instead, I would split things into BitmapFetcher::CreateURLFetcher(), an inlined
BitmapFetcher::url_fetcher() non-const accessor that callers can use to do
things like call SetURLRequestUserData(), and a BitmapFetcher::Start() function
that just calls through to the underlying Start().  (This last is optional;
callers could just as easily call url_fetcher()->Start().)

[dschuyler, 2015-04-22 23:44:32]

On 2015/04/22 23:16:20, Peter Kasting wrote:
> Any chance you can motivate why you need to be able to set this user data?
> 
>
https://codereview.chromium.org/1097383005/diff/1/chrome/browser/bitmap_fetch...
> File chrome/browser/bitmap_fetcher/bitmap_fetcher.cc (right):
> 
>
https://codereview.chromium.org/1097383005/diff/1/chrome/browser/bitmap_fetch...
> chrome/browser/bitmap_fetcher/bitmap_fetcher.cc:38: void BitmapFetcher::Start(
> I'm not a fan of having these two methods which have almost-identical args and
> code.  And what happens in the future when someone wants to set some other
piece
> of info on the underlying URL fetcher?
> 
> Instead, I would split things into BitmapFetcher::CreateURLFetcher(), an
inlined
> BitmapFetcher::url_fetcher() non-const accessor that callers can use to do
> things like call SetURLRequestUserData(), and a BitmapFetcher::Start()
function
> that just calls through to the underlying Start().  (This last is optional;
> callers could just as easily call url_fetcher()->Start().)

Hi,

While I'm not 100% sure that my review/lgtm would be needed here.  I decided to
really look at it for the practice of reviewing :)
I need to re-read Peters suggestion above a couple times to really let it soak
in; imo he's right.
Hey, I wonder though - rather than having BitmapFetcher::CreateURLFetcher(),
would it make sense to have more member functions on BitmapFetcher (to set setup
and init options separate from Start) and continue to keep the url_fetcher as
part of the internal workings?
I guess I'm just saying a variant of the same idea though: avoid growing a
longer parameter list of Start().

[wmaslowski, 2015-04-23 11:12:56]

I thought about it and it spliting this into Init and Start but TBH I think it
only complicated the API usage and required to change all the code that uses it.
I didn't really like it.

My new approach is to remove second Start function and just make
CreateURLFetcher virtual so if anyone needs to customize URLFetcher then they
can just override it.

PS: As for the usage AFAIR we attach some user data to all the requests for
extensions WebstoreInstaller and handle it specially in the network delegate.

[Theresa, 2015-04-23 22:13:23]

On 2015/04/23 11:12:56, wmaslowski wrote:
> I thought about it and it spliting this into Init and Start but TBH I think it
> only complicated the API usage and required to change all the code that uses
it.
> I didn't really like it.
> 
> My new approach is to remove second Start function and just make
> CreateURLFetcher virtual so if anyone needs to customize URLFetcher then they
> can just override it.
> 
> PS: As for the usage AFAIR we attach some user data to all the requests for
> extensions WebstoreInstaller and handle it specially in the network delegate.

Can you provide an example of how you envision things working with patch set 2?
I think I'm missing something... let's say CustomBitmapFetcher extends
BitmapFetcher and overrides CreateURLFetcher.
CustomBitmapFetcher#CreateURLFetcher won't accept parameters for the user_data
or create_data_callback. I guess those could be set in the constructor for
CustomBitmapFetcher or something like that?

I like Peter's suggestion better, because it's easier for clients to use since
they wouldn't have to create a new class and override CreateURLFetcher.

Another alternative is adding a method to BitmapFetcher like
setURLRequestUserData that gets called before Start(). In Start(), you could
check if the data is available and set it on the url_fetcher_ if it is. This is
less versatile than what Peter proposed, but it's another idea.

[Peter Kasting, 2015-04-23 22:18:59]

On 2015/04/23 22:13:23, twellington wrote:
> On 2015/04/23 11:12:56, wmaslowski wrote:
> > I thought about it and it spliting this into Init and Start but TBH I think
it
> > only complicated the API usage and required to change all the code that uses
> it.

Complicated API usage is a valid reason to object.  "Changes all the code that
uses it" isn't; we should never avoid making an API change because we have to
touch lots of places in the code.

[wmaslowski, 2015-04-24 10:41:56]

Went with suggestion to split Start into 2 parts

[Peter Kasting, 2015-04-24 23:18:22]

Just some comment requests, then LGTM after that.

https://codereview.chromium.org/1097383005/diff/40001/chrome/browser/bitmap_f...
File chrome/browser/bitmap_fetcher/bitmap_fetcher.h (right):

https://codereview.chromium.org/1097383005/diff/40001/chrome/browser/bitmap_f...
chrome/browser/bitmap_fetcher/bitmap_fetcher.h:34: // Start fetching the URL
with the fetcher. The delegate is notified
This comment needs updating.  In particular you should motivate why Init()
doesn't automatically Start().

https://codereview.chromium.org/1097383005/diff/40001/chrome/browser/bitmap_f...
chrome/browser/bitmap_fetcher/bitmap_fetcher.h:45: void Start();
You should add a comment here.

[Theresa, 2015-04-24 23:23:57]

lgtm too, thanks

[wmaslowski, 2015-05-04 13:41:40]

I updated the docs - can you confirm that it is still lgtm?

[Theresa, 2015-05-04 16:57:39]

one nit, but otherwise still lgtm

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

https://codereview.chromium.org/1097383005/diff/60001/chrome/browser/bitmap_f...
chrome/browser/bitmap_fetcher/bitmap_fetcher.h:35: // can be accessed to
configure it further(eg. add user data to request).
nit, missing space: "further (e.g. add...)"

[dschuyler, 2015-05-05 20:45:51]

lgtm

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

https://codereview.chromium.org/1097383005/diff/60001/chrome/browser/bitmap_f...
chrome/browser/bitmap_fetcher/bitmap_fetcher.h:40: const std::string& referrer,
No big deal, but I noticed that each of the calls seem to pass a blank string
here.  Maybe consider removing this parameter from Init and have callers use the
url_fether() to set the referrer if desired.  Maybe in another CL though - I
don't think this should hold up this CL.

[wmaslowski, 2015-05-06 09:40:36]

The CQ bit was checked by wmaslowski@opera.com

[wmaslowski, 2015-05-06 09:40:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, dschuyler@chromium.org, twellington@chromium.org
Link to the patchset: https://codereview.chromium.org/1097383005/#ps80001
(title: "Added missing space")

[commit-bot: I haz the power, 2015-05-06 09:40:42]

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

[commit-bot: I haz the power, 2015-05-06 09:47:17]

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

[commit-bot: I haz the power, 2015-05-06 09:47:18]

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

[wmaslowski, 2015-05-14 09:54:26]

Rebased and modified usage in bookmark_app_helper

[wmaslowski, 2015-05-22 11:57:03]

ping - I still need lgtm on last changes

[Peter Kasting, 2015-05-22 18:28:02]

On 2015/05/22 11:57:03, wmaslowski wrote:
> ping - I still need lgtm on last changes

LGTM from whom, on what changes?  When you need something like this please
clearly restate who's supposed to take what action.

[wmaslowski, 2015-05-25 09:37:07]

wmaslowski@opera.com changed reviewers:
+ asargent@chromium.org, benwells@chromium.org, dfalcantara@chromium.org,
dubroy@chromium.org, mathp@chromium.org, mlerman@chromium.org, sky@chromium.org

[wmaslowski, 2015-05-25 09:37:08]

Added missing reviewers

[Mike Lerman, 2015-05-25 10:43:23]

lgtm

[Mathieu, 2015-05-25 12:54:11]

lgtm suggestions

[benwells, 2015-05-25 22:28:25]

On 2015/05/25 12:54:11, Mathieu Perreault wrote:
> lgtm suggestions

c/b/banners and c/b/extensions lgtm

As Peter mentions when adding for reviewers it helps if you explain what each
reviewer should look at.

[gone, 2015-05-26 17:48:12]

lgtm

[wmaslowski, 2015-06-08 11:29:03]

The CQ bit was checked by wmaslowski@opera.com

[wmaslowski, 2015-06-08 11:29:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org, twellington@chromium.org
Link to the patchset: https://codereview.chromium.org/1097383005/#ps100001
(title: "Rebased and changed one more usage of BitmapFetcher")

[commit-bot: I haz the power, 2015-06-08 11:29:43]

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

[commit-bot: I haz the power, 2015-06-08 11:37:02]

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

[commit-bot: I haz the power, 2015-06-08 11:37:04]

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

[sky, 2015-06-08 15:05:29]

https://codereview.chromium.org/1097383005/diff/100001/chrome/browser/bitmap_...
File chrome/browser/bitmap_fetcher/bitmap_fetcher.cc (right):

https://codereview.chromium.org/1097383005/diff/100001/chrome/browser/bitmap_...
chrome/browser/bitmap_fetcher/bitmap_fetcher.cc:26: if (url_fetcher_ != NULL)
In general we DCHECK people calling Init multiple times.

https://codereview.chromium.org/1097383005/diff/100001/chrome/browser/bitmap_...
chrome/browser/bitmap_fetcher/bitmap_fetcher.cc:37: url_fetcher_->Start();
The old code early outed if url_fecher_ was already set. Why is that not
necessary anymore?

[wmaslowski, 2015-06-09 16:46:12]

The CQ bit was checked by wmaslowski@opera.com to run a CQ dry run

[wmaslowski, 2015-06-09 16:46:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, benwells@chromium.org, pkasting@chromium.org,
mathp@chromium.org, mlerman@chromium.org, dschuyler@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1097383005/#ps120001
(title: "Clarified how calling Init/Start twice works + fixed android")

[commit-bot: I haz the power, 2015-06-09 16:46:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097383005/120001

[commit-bot: I haz the power, 2015-06-09 17:34:49]

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

[commit-bot: I haz the power, 2015-06-09 17:34:51]

Dry run: This issue passed the CQ dry run.

[wmaslowski, 2015-06-10 08:18:23]

wmaslowski@opera.com changed reviewers:
+ kkimlabs@chromium.org

[wmaslowski, 2015-06-10 08:18:24]

@sky - I addressed Your concerns by adding one more NULL check, but I'd rather
not add DCHECK in Init. This path replaces single Start() call with
Init()+Start() and previous version explicitly stated that calling Start
multiple times is ignored rather than an error.

@kkimlabs - Added you as reviewer for
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc

[Kibeom Kim (inactive), 2015-06-10 15:01:32]

chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc lgtm

[sky, 2015-06-10 15:57:05]

LGTM

[wmaslowski, 2015-06-11 15:01:21]

The CQ bit was checked by wmaslowski@opera.com

[commit-bot: I haz the power, 2015-06-11 15:02:19]

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

[commit-bot: I haz the power, 2015-06-11 16:04:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-11 16:05:21]

Patchset 7 (id:??) landed as
https://crrev.com/58c4b6fc734361e9de25959c7f92690e8f5ba372
Cr-Commit-Position: refs/heads/master@{#333962}