Replace image_messages.h with Mojo service

Replace image_messages.h with Mojo service

Implement ImageDownloader service and register it into RenderFrame
ServiceRegistry. This Mojo service will do the job did by
ImageLoadingHelper before.

Re-use mojom files:
third_party/mojo_services/src/geometry/public/interfaces/
geometry.mojom
skia/public/interfaces/bitmap.mojom

TBR=rockot@chromium.org,amistry@google.com,darin@chromium.org,nasko@chromium.org
BUG=

Committed: https://crrev.com/ca2eabdc854a972db31fec01af6bb22ae643af4c
Cr-Commit-Position: refs/heads/master@{#337370}

[haltonhuo, 2015-04-14 07:42:35]

This CL is come from Leon Han <leon.han@intel.com>, he has "git cl upload" issue
inside Intel. So I submit this CL for him.

[leonhsl(Using Gerrit), 2015-04-14 07:53:18]

This CL is for the intent-to-impl discussed in
https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/hfO12Iwnxow

This CL depends on the CL https://codereview.chromium.org/1028543002/ because
I'm re-using the bitmap.mojom defined there.

[Ken Rockot(use gerrit already), 2015-04-14 16:14:21]

Holding off the LG until the dependent patch lands, but this looks pretty great
as-is!

https://codereview.chromium.org/1085783002/diff/1/content/browser/image_downl...
File content/browser/image_downloader/image_downloader_dispatcher.h (right):

https://codereview.chromium.org/1085783002/diff/1/content/browser/image_downl...
content/browser/image_downloader/image_downloader_dispatcher.h:18: class
ImageDownloaderDispatcher : public mojo::ErrorHandler {
nit: Please add a brief comment documenting the purpose of the class

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
File content/common/image_downloader/image_downloader_types.mojom (right):

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader_types.mojom:10: struct
DownloadReq {
nit: I think we shouldn't abbreviate this. DownloadRequest would be fine.

Alternatively since these live in image_downloader, they could also just be
Request and Result respectively.

https://codereview.chromium.org/1085783002/diff/1/third_party/mojo_services/R...
File third_party/mojo_services/README.chromium (right):

https://codereview.chromium.org/1085783002/diff/1/third_party/mojo_services/R...
third_party/mojo_services/README.chromium:14: Add
third_party/mojo_services/mojo_services_public.gyp.
nit: This comment isn't necessary. Everything we roll is one level down in src/.

[leonhsl(Using Gerrit), 2015-04-15 02:50:31]

Thank you very much for kindly review and encouragement words:)
I'll fix the nit issues soon.
 
>
https://codereview.chromium.org/1085783002/diff/1/third_party/mojo_services/R...
> File third_party/mojo_services/README.chromium (right):
> 
>
https://codereview.chromium.org/1085783002/diff/1/third_party/mojo_services/R...
> third_party/mojo_services/README.chromium:14: Add
> third_party/mojo_services/mojo_services_public.gyp.
> nit: This comment isn't necessary. Everything we roll is one level down in
src/.

I updated README.chromium because the following "** Presubmit Warnings **" when
doing 'git cl upload':
"
When updating or adding third party code the appropriate
'README.chromium' file should also be updated with the correct
version and package information.
  third_party/mojo_services/mojo_services_public.gyp
"
Should I just ignore the warning to continue uploading or something else should
I do for it?
Thanks a lot.

[Ken Rockot(use gerrit already), 2015-04-15 03:22:32]

In this case I am inclined to ignore the presubmit warning, but I could be
wrong. I will let someone else support or oppose my suggestion. :)

On Tue, Apr 14, 2015 at 7:50 PM, <leon.han@intel.com> wrote:

> Thank you very much for kindly review and encouragement words:)
> I'll fix the nit issues soon.
>
>
> https://codereview.chromium.org/1085783002/diff/1/third_
> party/mojo_services/README.chromium
>
>> File third_party/mojo_services/README.chromium (right):
>>
>
>
> https://codereview.chromium.org/1085783002/diff/1/third_
> party/mojo_services/README.chromium#newcode14
>
>> third_party/mojo_services/README.chromium:14: Add
>> third_party/mojo_services/mojo_services_public.gyp.
>> nit: This comment isn't necessary. Everything we roll is one level down in
>>
> src/.
>
> I updated README.chromium because the following "** Presubmit Warnings **"
> when
> doing 'git cl upload':
> "
> When updating or adding third party code the appropriate
> 'README.chromium' file should also be updated with the correct
> version and package information.
>   third_party/mojo_services/mojo_services_public.gyp
> "
> Should I just ignore the warning to continue uploading or something else
> should
> I do for it?
> Thanks a lot.
>
> https://codereview.chromium.org/1085783002/
>

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

[Anand Mistry (off Chromium), 2015-04-15 06:30:38]

amistry@chromium.org changed reviewers:
+ amistry@chromium.org
- amistry@google.com

[Anand Mistry (off Chromium), 2015-04-15 07:20:45]

I haven't gotten around to reviewing this yet (but I will), but I should say
that my change (the one this depends on) won't be getting any traction for the
next 2 week since I'll be away. Feel free to split off the skia/bitmap parts and
have that submitted separately.

[nasko, 2015-04-15 20:57:22]

Overall it looks good, some minor nits on formatting and comments. I would defer
to amistry@ for the Mojo pieces.

https://codereview.chromium.org/1085783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1754: // Disconnect with
ImageDownloader Mojo service in render frame.
nit: s/render frame/RenderFrame/

https://codereview.chromium.org/1085783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1085783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:707: // Dispatcher to do IPC
with ImageDownloader Mojo service in Renderer side.
nit: "Renderer" isn't very clear. Is it the renderer process? The RenderFrame
object? Or something else altogether.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
File content/common/image_downloader/image_downloader.mojom (right):

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader.mojom:12: 
nit: No need for extra new line at end of file.

https://codereview.chromium.org/1085783002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4791: // only main frame have
ImageDownloader service
nit: Capitalize "only" and end with a period.

https://codereview.chromium.org/1085783002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4792: if (!frame_->parent())
If statements with body longer than one line require {}.

[Anand Mistry (off Chromium), 2015-04-16 01:07:16]

First pass comments. Haven't looked too carefully and I'm about to catch a
flight.

At a high level, me likey! But still need to look over the details.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
File content/common/image_downloader/image_downloader.mojom (right):

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader.mojom:9: interface
ImageDownloader {
Please add a short comment on what this is.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
File content/common/image_downloader/image_downloader_types.mojom (right):

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader_types.mojom:10: struct
DownloadReq {
On 2015/04/14 16:14:20, Ken Rockot wrote:
> nit: I think we shouldn't abbreviate this. DownloadRequest would be fine.
> 
> Alternatively since these live in image_downloader, they could also just be
> Request and Result respectively.

I too think these should live in image_downloader.mojom. They're not generally
useful by themselves, so no point in splitting them out.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader_types.mojom:18: int32
http_status_code;
uint32. HTTP status codes are defined to be 3-digit integers
(http://tools.ietf.org/html/rfc7231#page-47), and hence are always non-negative.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader_types.mojom:19: string url;
Why is this needed? As far as I can tell, it never changes and is just passed
from request to response.

https://codereview.chromium.org/1085783002/diff/1/content/renderer/image_down...
File content/renderer/image_downloader/image_downloader_impl.h (right):

https://codereview.chromium.org/1085783002/diff/1/content/renderer/image_down...
content/renderer/image_downloader/image_downloader_impl.h:35: typedef
mojo::Callback<void(image_downloader::DownloadResultPtr)>
This should already be defined in the generated code as DownloadImageCallback.

https://codereview.chromium.org/1085783002/diff/1/third_party/mojo_services/m...
File third_party/mojo_services/mojo_services_public.gyp (right):

https://codereview.chromium.org/1085783002/diff/1/third_party/mojo_services/m...
third_party/mojo_services/mojo_services_public.gyp:1: # Copyright 2015 The
Chromium Authors. All rights reserved.
I don't know much about how third_party works, but won't this get reverted on
the next roll?

[Ken Rockot(use gerrit already), 2015-04-16 01:09:10]

>
https://codereview.chromium.org/1085783002/diff/1/third_party/mojo_services/m...
> third_party/mojo_services/mojo_services_public.gyp:1: # Copyright 2015 The
> Chromium Authors. All rights reserved.
> I don't know much about how third_party works, but won't this get reverted on
> the next roll?

Just on this point: Nope. We don't roll anything directly into toplevel
third_party/mojo or third_party/mojo_services, so the immediate contents of
these paths are safe.

[leonhsl(Using Gerrit), 2015-04-16 02:58:17]

leon.han@intel.com changed reviewers:
+ leon.han@intel.com

[leonhsl(Using Gerrit), 2015-04-16 02:58:17]

Thank you all for kindly review and I'll revise code according comments now.
Thanks again~

[leonhsl(Using Gerrit), 2015-04-17 06:14:35]

1. Updated code according review comments, PTAL.
2. Split out another CL https://codereview.chromium.org/1098643002/ from
Amistry's CL https://codereview.chromium.org/1028543002/ to provide bitmap.mojom
definition which is needed by this CL. PTAL there.

Thanks all~

https://codereview.chromium.org/1085783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1754: // Disconnect with
ImageDownloader Mojo service in render frame.
On 2015/04/15 20:57:22, nasko wrote:
> nit: s/render frame/RenderFrame/

Done.

https://codereview.chromium.org/1085783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1085783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:707: // Dispatcher to do IPC
with ImageDownloader Mojo service in Renderer side.
On 2015/04/15 20:57:22, nasko wrote:
> nit: "Renderer" isn't very clear. Is it the renderer process? The RenderFrame
> object? Or something else altogether.

Done. "Renderer side" ---> "RenderFrame"

https://codereview.chromium.org/1085783002/diff/1/content/browser/image_downl...
File content/browser/image_downloader/image_downloader_dispatcher.h (right):

https://codereview.chromium.org/1085783002/diff/1/content/browser/image_downl...
content/browser/image_downloader/image_downloader_dispatcher.h:18: class
ImageDownloaderDispatcher : public mojo::ErrorHandler {
On 2015/04/14 16:14:20, Ken Rockot wrote:
> nit: Please add a brief comment documenting the purpose of the class

Done.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
File content/common/image_downloader/image_downloader.mojom (right):

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader.mojom:9: interface
ImageDownloader {
On 2015/04/16 01:07:16, Anand Mistry (away till 30th) wrote:
> Please add a short comment on what this is.

Done.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader.mojom:12: 
On 2015/04/15 20:57:22, nasko wrote:
> nit: No need for extra new line at end of file.

Done.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
File content/common/image_downloader/image_downloader_types.mojom (right):

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader_types.mojom:10: struct
DownloadReq {
On 2015/04/14 16:14:20, Ken Rockot wrote:
> nit: I think we shouldn't abbreviate this. DownloadRequest would be fine.
> 
> Alternatively since these live in image_downloader, they could also just be
> Request and Result respectively.

Done. 
Combine image_downloader_types.mojom into image_downloader.mojom.
Struct name as DownloadRequest/DownloadResult.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader_types.mojom:18: int32
http_status_code;
On 2015/04/16 01:07:16, Anand Mistry (away till 30th) wrote:
> uint32. HTTP status codes are defined to be 3-digit integers
> (http://tools.ietf.org/html/rfc7231#page-47), and hence are always
non-negative.

The old IPC message ImageHostMsg_DidDownloadImage defines http status code as
int32, and codes residing in both sides of the ImageDownloader mojo connection
are treating it as int32, because int32 is also enough to hold the 3-digit
integer, can we just keep it now? Otherwise we need to align many places to
uint32.

https://codereview.chromium.org/1085783002/diff/1/content/common/image_downlo...
content/common/image_downloader/image_downloader_types.mojom:19: string url;
On 2015/04/16 01:07:16, Anand Mistry (away till 30th) wrote:
> Why is this needed? As far as I can tell, it never changes and is just passed
> from request to response.

Done. Yes we can omit it from response.

https://codereview.chromium.org/1085783002/diff/1/content/renderer/image_down...
File content/renderer/image_downloader/image_downloader_impl.h (right):

https://codereview.chromium.org/1085783002/diff/1/content/renderer/image_down...
content/renderer/image_downloader/image_downloader_impl.h:35: typedef
mojo::Callback<void(image_downloader::DownloadResultPtr)>
On 2015/04/16 01:07:16, Anand Mistry (away till 30th) wrote:
> This should already be defined in the generated code as DownloadImageCallback.

Done.

https://codereview.chromium.org/1085783002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4791: // only main frame have
ImageDownloader service
On 2015/04/15 20:57:22, nasko wrote:
> nit: Capitalize "only" and end with a period.

Done.

https://codereview.chromium.org/1085783002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4792: if (!frame_->parent())
On 2015/04/15 20:57:22, nasko wrote:
> If statements with body longer than one line require {}.

Done.

https://codereview.chromium.org/1085783002/diff/1/third_party/mojo_services/R...
File third_party/mojo_services/README.chromium (right):

https://codereview.chromium.org/1085783002/diff/1/third_party/mojo_services/R...
third_party/mojo_services/README.chromium:14: Add
third_party/mojo_services/mojo_services_public.gyp.
On 2015/04/14 16:14:20, Ken Rockot wrote:
> nit: This comment isn't necessary. Everything we roll is one level down in
src/.

Reverted my change.

[leonhsl(Using Gerrit), 2015-06-11 05:23:17]

As the dependency /skia part has been submitted, did the rebase and re-uploaded.
PTAL, Thanks.

[Anand Mistry (off Chromium), 2015-06-12 08:19:38]

https://codereview.chromium.org/1085783002/diff/40001/content/browser/image_d...
File content/browser/image_downloader/image_downloader_dispatcher.h (right):

https://codereview.chromium.org/1085783002/diff/40001/content/browser/image_d...
content/browser/image_downloader/image_downloader_dispatcher.h:24: class
ImageDownloaderDispatcher : public mojo::ErrorHandler {
I'm strongly inclined to say delete this class and just use the Mojo interface
directly from RenderFrameHostImpl. The error handler is unnecessary since it's
not doing anything useful, and the remaining functionality is just
boxing/unboxing.

https://codereview.chromium.org/1085783002/diff/40001/content/common/image_do...
File content/common/image_downloader/image_downloader.mojom (right):

https://codereview.chromium.org/1085783002/diff/40001/content/common/image_do...
content/common/image_downloader/image_downloader.mojom:24: // Called by
ImageDownloaderDispatcher, sends download request
This is an interface. Describe the intended functionality, not the
implementation. i.e.
Fetch and decode an image from a given URL. Returns the decoded image.

But add a bit more detail than that.

https://codereview.chromium.org/1085783002/diff/40001/content/common/image_me...
File content/common/image_messages.h (left):

https://codereview.chromium.org/1085783002/diff/40001/content/common/image_me...
content/common/image_messages.h:14: #define IPC_MESSAGE_START ImageMsgStart
Delete this constant from ipc/ipc_message_start.h

https://codereview.chromium.org/1085783002/diff/40001/content/renderer/image_...
File content/renderer/image_downloader/image_downloader_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/40001/content/renderer/image_...
content/renderer/image_downloader/image_downloader_impl.cc:151: SkBitmap
ImageDownloaderImpl::ImageFromDataUrl(const GURL& url) const {
Push this out of this class and into the anonymous namespace. It doesn't access
any class members, and the functionality is logically separate from the rest of
this class.

https://codereview.chromium.org/1085783002/diff/40001/content/renderer/image_...
content/renderer/image_downloader/image_downloader_impl.cc:156: const unsigned
char* src_data =
Not sure why this is like this, but std::string::data() does the same thing, but
nicer.

[leonhsl(Using Gerrit), 2015-06-15 08:09:43]

Thanks for review.
Uploaded new patch, PTAL.

https://codereview.chromium.org/1085783002/diff/40001/content/browser/image_d...
File content/browser/image_downloader/image_downloader_dispatcher.h (right):

https://codereview.chromium.org/1085783002/diff/40001/content/browser/image_d...
content/browser/image_downloader/image_downloader_dispatcher.h:24: class
ImageDownloaderDispatcher : public mojo::ErrorHandler {
On 2015/06/12 08:19:38, Anand Mistry wrote:
> I'm strongly inclined to say delete this class and just use the Mojo interface
> directly from RenderFrameHostImpl. The error handler is unnecessary since it's
> not doing anything useful, and the remaining functionality is just
> boxing/unboxing.

I think that ErrorHandler can reset the ImageDownloaderPtr once Mojo connection
error happened, so that if StartDownload() is called next time, we can
re-connect ImageDownloaderPtr to remote service. Otherwise, the
ImageDownloaderPtr will keep non-null and unbound, then later calls to
StartDownload() will just fail silently. 
I'm not sure why GeolocationDispatcher and BatteryStatusDispatcher have no
ErrorHandler, maybe it's functional OK for them not to re-connect even Mojo
connection broke.
And, I thought render_frame_host_impl.cc(h) is huge so I tried to avoid adding
more code into it.. :)

https://codereview.chromium.org/1085783002/diff/40001/content/common/image_do...
File content/common/image_downloader/image_downloader.mojom (right):

https://codereview.chromium.org/1085783002/diff/40001/content/common/image_do...
content/common/image_downloader/image_downloader.mojom:24: // Called by
ImageDownloaderDispatcher, sends download request
On 2015/06/12 08:19:38, Anand Mistry wrote:
> This is an interface. Describe the intended functionality, not the
> implementation. i.e.
> Fetch and decode an image from a given URL. Returns the decoded image.
> 
> But add a bit more detail than that.

Done.

https://codereview.chromium.org/1085783002/diff/40001/content/common/image_me...
File content/common/image_messages.h (left):

https://codereview.chromium.org/1085783002/diff/40001/content/common/image_me...
content/common/image_messages.h:14: #define IPC_MESSAGE_START ImageMsgStart
On 2015/06/12 08:19:38, Anand Mistry wrote:
> Delete this constant from ipc/ipc_message_start.h

Done.

https://codereview.chromium.org/1085783002/diff/40001/content/renderer/image_...
File content/renderer/image_downloader/image_downloader_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/40001/content/renderer/image_...
content/renderer/image_downloader/image_downloader_impl.cc:151: SkBitmap
ImageDownloaderImpl::ImageFromDataUrl(const GURL& url) const {
On 2015/06/12 08:19:38, Anand Mistry wrote:
> Push this out of this class and into the anonymous namespace. It doesn't
access
> any class members, and the functionality is logically separate from the rest
of
> this class.

Done.

https://codereview.chromium.org/1085783002/diff/40001/content/renderer/image_...
content/renderer/image_downloader/image_downloader_impl.cc:156: const unsigned
char* src_data =
On 2015/06/12 08:19:38, Anand Mistry wrote:
> Not sure why this is like this, but std::string::data() does the same thing,
but
> nicer.

Yeah data.data() is better than &data[0], modified.

[Anand Mistry (off Chromium), 2015-06-16 07:51:34]

amistry@chromium.org changed reviewers:
+ sammc@chromium.org

[Anand Mistry (off Chromium), 2015-06-16 07:51:36]

Adding Sam, who probably has some things to say.

https://codereview.chromium.org/1085783002/diff/40001/content/browser/image_d...
File content/browser/image_downloader/image_downloader_dispatcher.h (right):

https://codereview.chromium.org/1085783002/diff/40001/content/browser/image_d...
content/browser/image_downloader/image_downloader_dispatcher.h:24: class
ImageDownloaderDispatcher : public mojo::ErrorHandler {
On 2015/06/15 08:09:43, leon.han wrote:
> On 2015/06/12 08:19:38, Anand Mistry wrote:
> > I'm strongly inclined to say delete this class and just use the Mojo
interface
> > directly from RenderFrameHostImpl. The error handler is unnecessary since
it's
> > not doing anything useful, and the remaining functionality is just
> > boxing/unboxing.
> 
> I think that ErrorHandler can reset the ImageDownloaderPtr once Mojo
connection
> error happened, so that if StartDownload() is called next time, we can
> re-connect ImageDownloaderPtr to remote service. Otherwise, the
> ImageDownloaderPtr will keep non-null and unbound, then later calls to
> StartDownload() will just fail silently. 
> I'm not sure why GeolocationDispatcher and BatteryStatusDispatcher have no
> ErrorHandler, maybe it's functional OK for them not to re-connect even Mojo
> connection broke.
> And, I thought render_frame_host_impl.cc(h) is huge so I tried to avoid adding
> more code into it.. :)

Yes, but the Mojo connection should never terminate since the other end is
strongly bound to the message pipe. If it does, that's a problem, not an
expected condition. Handling the error gracefully hides the issue. There are
times when a disconnection is expected, but this is not one of them.

Also, I know the implementation in the renderer is also bound to the RenderFrame
lifespan, but if that goes, the host side will also be gone, so there shouldn't
be an issue there.

https://codereview.chromium.org/1085783002/diff/60001/content/renderer/image_...
File content/renderer/image_downloader/image_downloader_impl.h (right):

https://codereview.chromium.org/1085783002/diff/60001/content/renderer/image_...
content/renderer/image_downloader/image_downloader_impl.h:35: explicit
ImageDownloaderImpl(
No 'explicit' since you have more than 2 arguments.

https://codereview.chromium.org/1085783002/diff/60001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (left):

https://codereview.chromium.org/1085783002/diff/60001/ipc/ipc_message_start.h...
ipc/ipc_message_start.h:56: ImageMsgStart,
On second thought (and after looking over some other code), add this back.
Instead, add a comment that this has been removed. My concern is that by
removing this, the message types change and could break something (I suspect app
shim will break).

[leonhsl(Using Gerrit), 2015-06-17 11:30:36]

On 2015/06/16 07:51:36, Anand Mistry wrote:
>
> Yes, but the Mojo connection should never terminate since the other end is
> strongly bound to the message pipe. If it does, that's a problem, not an
> expected condition. Handling the error gracefully hides the issue. There are
> times when a disconnection is expected, but this is not one of them.
> 
> Also, I know the implementation in the renderer is also bound to the
RenderFrame
> lifespan, but if that goes, the host side will also be gone, so there
shouldn't
> be an issue there.
> 

Understood and agree with removing the ErrorHandler.
And, I'd like to keep the ImageDownloaderDispatcher class because otherwise we 
will add more codes into render_frame_host_impl.cc to make it huger and bring 
several more header files included there. How do you think? I'm relying on your
opinion.
Thanks a lot~

[Anand Mistry (off Chromium), 2015-06-18 00:36:31]

On 2015/06/17 11:30:36, leon.han wrote:
> On 2015/06/16 07:51:36, Anand Mistry wrote:
> >
> > Yes, but the Mojo connection should never terminate since the other end is
> > strongly bound to the message pipe. If it does, that's a problem, not an
> > expected condition. Handling the error gracefully hides the issue. There are
> > times when a disconnection is expected, but this is not one of them.
> > 
> > Also, I know the implementation in the renderer is also bound to the
> RenderFrame
> > lifespan, but if that goes, the host side will also be gone, so there
> shouldn't
> > be an issue there.
> > 
> 
> Understood and agree with removing the ErrorHandler.
> And, I'd like to keep the ImageDownloaderDispatcher class because otherwise we

> will add more codes into render_frame_host_impl.cc to make it huger and bring 
> several more header files included there. How do you think? I'm relying on
your
> opinion.
> Thanks a lot~

My concern is that the amount (and complexity) of code being split off into its
own class is so small and trivial, it's not worth it. You're adding 2 files (127
lines total) to save 10-20 lines of code. In my opinion, the trade off isn't
worth it. I agree we should be slimming down RenderFrameHostImpl, but there's
probably better fish to fry in that space.

[leonhsl(Using Gerrit), 2015-06-23 02:18:02]

Sorry for late response. PTAL, Thanks~

https://codereview.chromium.org/1085783002/diff/40001/content/browser/image_d...
File content/browser/image_downloader/image_downloader_dispatcher.h (right):

https://codereview.chromium.org/1085783002/diff/40001/content/browser/image_d...
content/browser/image_downloader/image_downloader_dispatcher.h:24: class
ImageDownloaderDispatcher : public mojo::ErrorHandler {
On 2015/06/16 07:51:35, Anand Mistry wrote:
> On 2015/06/15 08:09:43, leon.han wrote:
> > On 2015/06/12 08:19:38, Anand Mistry wrote:
> > > I'm strongly inclined to say delete this class and just use the Mojo
> interface
> > > directly from RenderFrameHostImpl. The error handler is unnecessary since
> it's
> > > not doing anything useful, and the remaining functionality is just
> > > boxing/unboxing.
> > 
> > I think that ErrorHandler can reset the ImageDownloaderPtr once Mojo
> connection
> > error happened, so that if StartDownload() is called next time, we can
> > re-connect ImageDownloaderPtr to remote service. Otherwise, the
> > ImageDownloaderPtr will keep non-null and unbound, then later calls to
> > StartDownload() will just fail silently. 
> > I'm not sure why GeolocationDispatcher and BatteryStatusDispatcher have no
> > ErrorHandler, maybe it's functional OK for them not to re-connect even Mojo
> > connection broke.
> > And, I thought render_frame_host_impl.cc(h) is huge so I tried to avoid
adding
> > more code into it.. :)
> 
> Yes, but the Mojo connection should never terminate since the other end is
> strongly bound to the message pipe. If it does, that's a problem, not an
> expected condition. Handling the error gracefully hides the issue. There are
> times when a disconnection is expected, but this is not one of them.
> 
> Also, I know the implementation in the renderer is also bound to the
RenderFrame
> lifespan, but if that goes, the host side will also be gone, so there
shouldn't
> be an issue there.

Done.

https://codereview.chromium.org/1085783002/diff/60001/content/renderer/image_...
File content/renderer/image_downloader/image_downloader_impl.h (right):

https://codereview.chromium.org/1085783002/diff/60001/content/renderer/image_...
content/renderer/image_downloader/image_downloader_impl.h:35: explicit
ImageDownloaderImpl(
On 2015/06/16 07:51:36, Anand Mistry wrote:
> No 'explicit' since you have more than 2 arguments.

Done.

https://codereview.chromium.org/1085783002/diff/60001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (left):

https://codereview.chromium.org/1085783002/diff/60001/ipc/ipc_message_start.h...
ipc/ipc_message_start.h:56: ImageMsgStart,
On 2015/06/16 07:51:36, Anand Mistry wrote:
> On second thought (and after looking over some other code), add this back.
> Instead, add a comment that this has been removed. My concern is that by
> removing this, the message types change and could break something (I suspect
app
> shim will break).

Done.

[leonhsl(Using Gerrit), 2015-06-28 09:59:41]

ping amistry@

[Anand Mistry (off Chromium), 2015-07-01 04:17:12]

LGTM

Sorry this has taken so long. I'm happy with this, but you need to sync and
rebase. Once you do, I'll start a few try jobs to see if you've missed anything.

[haltonhuo, 2015-07-01 07:46:27]

The CQ bit was checked by halton.huo@intel.com

[haltonhuo, 2015-07-01 07:46:28]

The patchset sent to the CQ was uploaded after l-g-t-m from amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1085783002/#ps100001
(title: "rebase to master@{#336990}")

[commit-bot: I haz the power, 2015-07-01 07:46:47]

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

[Anand Mistry (off Chromium), 2015-07-01 07:53:52]

On 2015/07/01 07:46:47, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1085783002/100001

Unless you're an owner, this won't work. I'm not an owner of these, so you'll
need an LGTM from some other people.

[commit-bot: I haz the power, 2015-07-01 08:23:39]

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

[commit-bot: I haz the power, 2015-07-01 08:23:41]

Exceeded global retry quota

[leonhsl(Using Gerrit), 2015-07-01 08:44:28]

leon.han@intel.com changed reviewers:
+ sky@chromium.org

[leonhsl(Using Gerrit), 2015-07-01 08:44:29]

Hi,
Would you PTAL as OWNERs? Thanks~
ping rockot@, nasko@, sky@

amistry@, Thanks a lot for kindly help. I'm solving the trybot error at the same
time..

[haltonhuo, 2015-07-02 01:01:54]

The CQ bit was checked by halton.huo@intel.com

[haltonhuo, 2015-07-02 01:01:54]

The patchset sent to the CQ was uploaded after l-g-t-m from amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1085783002/#ps120001
(title: "Fix trybot unit_tests error")

[commit-bot: I haz the power, 2015-07-02 01:03:30]

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

[commit-bot: I haz the power, 2015-07-02 01:41:15]

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

[commit-bot: I haz the power, 2015-07-02 01:41:17]

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

[nasko, 2015-07-02 08:41:07]

Looks good overall, but there are some layering issues that need to be resolved.

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
File content/browser/frame_host/DEPS (right):

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
content/browser/frame_host/DEPS:25: "+content/public/browser/web_contents.h",
This seems wrong. The frame_host layer doesn't and shouldn't know about the
web_contents layer. You should define the callback type in a common location and
have WebContents and this layer both consume it.

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1890: int
RenderFrameHostImpl::DownloadImage(
To avoid all the complexity of defining a callback in a common location, why not
allow access to the image download service by making it accessible from
RenderFrameHost? With that approach, the code of this method can be fully moved
to WebContentsImpl and there will be no layering violations.

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1902:
mojo::GetProxy(&mojo_image_downloader_));
Can this fail? If not handled, this will cause a browser crash.

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:450: // Download image API
called by web_contents.
No need to add specifics as to who calls this method, it can be anyone in the
future, so this type of comment will get stale/incorrect.

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
File content/renderer/image_downloader/image_downloader_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
content/renderer/image_downloader/image_downloader_impl.cc:38: // Decode the
image using WebKit's image decoder.
s/WebKit/Blink/

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
content/renderer/image_downloader/image_downloader_impl.cc:172: if (!frame)
There shouldn't be a case where render_frame()->GetWebFrame() returns nullptr. I
think you can convert this to a DCHECK or just drop it.

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
File content/renderer/image_downloader/image_downloader_impl.h (right):

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
content/renderer/image_downloader/image_downloader_impl.h:5: #ifndef
CONTENT_IMAGE_DOWNLOADER_IMPL_H_
The include guard needs to have the full path and file name:
CONTENT_RENDERER_IMAGE_DOWNLOADER_IMAGE_DOWNLOADER_IMPL_H_

https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (right):

https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start....
ipc/ipc_message_start.h:56: // Note: ImageMsgStart has been removed, we use Mojo
instead.
Why not remove this then?

[Anand Mistry (off Chromium), 2015-07-02 08:56:51]

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1902:
mojo::GetProxy(&mojo_image_downloader_));
On 2015/07/02 08:41:07, nasko (paris) wrote:
> Can this fail? If not handled, this will cause a browser crash.

Not in a way that will cause a crash. This line will always succeed because the
connection is done asynchronously. If the connection ultimately fails (i.e. the
renderer crashes), then this will result in a hanging callback. But that problem
already exists in the current implementation.

https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (right):

https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start....
ipc/ipc_message_start.h:56: // Note: ImageMsgStart has been removed, we use Mojo
instead.
On 2015/07/02 08:41:07, nasko (paris) wrote:
> Why not remove this then?

I'll answer this. I suspected that removing it might cause issues with IPC
message mismatches. I know this has caused issues in the past, so I suggested to
comment it instead of removing.

[nasko, 2015-07-02 09:00:09]

On 2015/07/02 08:56:51, Anand Mistry wrote:
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_impl.cc:1902:
> mojo::GetProxy(&mojo_image_downloader_));
> On 2015/07/02 08:41:07, nasko (paris) wrote:
> > Can this fail? If not handled, this will cause a browser crash.
> 
> Not in a way that will cause a crash. This line will always succeed because
the
> connection is done asynchronously. If the connection ultimately fails (i.e.
the
> renderer crashes), then this will result in a hanging callback. But that
problem
> already exists in the current implementation.
> 
> https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start.h
> File ipc/ipc_message_start.h (right):
> 
>
https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start....
> ipc/ipc_message_start.h:56: // Note: ImageMsgStart has been removed, we use
Mojo
> instead.
> On 2015/07/02 08:41:07, nasko (paris) wrote:
> > Why not remove this then?
> 
> I'll answer this. I suspected that removing it might cause issues with IPC
> message mismatches. I know this has caused issues in the past, so I suggested
to
> comment it instead of removing.

Can you point me to examples of what those issues were (bugs/CLs)?

[Anand Mistry (off Chromium), 2015-07-02 09:03:53]

On 2015/07/02 09:00:09, nasko (paris) wrote:
> On 2015/07/02 08:56:51, Anand Mistry wrote:
> >
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> > File content/browser/frame_host/render_frame_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> > content/browser/frame_host/render_frame_host_impl.cc:1902:
> > mojo::GetProxy(&mojo_image_downloader_));
> > On 2015/07/02 08:41:07, nasko (paris) wrote:
> > > Can this fail? If not handled, this will cause a browser crash.
> > 
> > Not in a way that will cause a crash. This line will always succeed because
> the
> > connection is done asynchronously. If the connection ultimately fails (i.e.
> the
> > renderer crashes), then this will result in a hanging callback. But that
> problem
> > already exists in the current implementation.
> > 
> >
https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start.h
> > File ipc/ipc_message_start.h (right):
> > 
> >
>
https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start....
> > ipc/ipc_message_start.h:56: // Note: ImageMsgStart has been removed, we use
> Mojo
> > instead.
> > On 2015/07/02 08:41:07, nasko (paris) wrote:
> > > Why not remove this then?
> > 
> > I'll answer this. I suspected that removing it might cause issues with IPC
> > message mismatches. I know this has caused issues in the past, so I
suggested
> to
> > comment it instead of removing.
> 
> Can you point me to examples of what those issues were (bugs/CLs)?

https://code.google.com/p/chromium/issues/detail?id=405347#c5

[nasko, 2015-07-02 11:13:33]

On 2015/07/02 09:03:53, Anand Mistry wrote:
> On 2015/07/02 09:00:09, nasko (paris) wrote:
> > On 2015/07/02 08:56:51, Anand Mistry wrote:
> > >
> >
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> > > File content/browser/frame_host/render_frame_host_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> > > content/browser/frame_host/render_frame_host_impl.cc:1902:
> > > mojo::GetProxy(&mojo_image_downloader_));
> > > On 2015/07/02 08:41:07, nasko (paris) wrote:
> > > > Can this fail? If not handled, this will cause a browser crash.
> > > 
> > > Not in a way that will cause a crash. This line will always succeed
because
> > the
> > > connection is done asynchronously. If the connection ultimately fails
(i.e.
> > the
> > > renderer crashes), then this will result in a hanging callback. But that
> > problem
> > > already exists in the current implementation.
> > > 
> > >
> https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start.h
> > > File ipc/ipc_message_start.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start....
> > > ipc/ipc_message_start.h:56: // Note: ImageMsgStart has been removed, we
use
> > Mojo
> > > instead.
> > > On 2015/07/02 08:41:07, nasko (paris) wrote:
> > > > Why not remove this then?
> > > 
> > > I'll answer this. I suspected that removing it might cause issues with IPC
> > > message mismatches. I know this has caused issues in the past, so I
> suggested
> > to
> > > comment it instead of removing.
> > 
> > Can you point me to examples of what those issues were (bugs/CLs)?
> 
> https://code.google.com/p/chromium/issues/detail?id=405347#c5

Chromium doesn't really support processes from different versions being used.
There are lots of other things that will go bad, even without this particular
case. Unless there is a different reason, I would remove the entry in the list.

[leonhsl(Using Gerrit), 2015-07-03 03:49:55]

On 2015/07/02 08:41:07, nasko (paris) wrote:
> Looks good overall, but there are some layering issues that need to be
resolved.
> 
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> File content/browser/frame_host/DEPS (right):
> 
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> content/browser/frame_host/DEPS:25: "+content/public/browser/web_contents.h",
> This seems wrong. The frame_host layer doesn't and shouldn't know about the
> web_contents layer. You should define the callback type in a common location
and
> have WebContents and this layer both consume it.
> 
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_impl.cc:1890: int
> RenderFrameHostImpl::DownloadImage(
> To avoid all the complexity of defining a callback in a common location, why
not
> allow access to the image download service by making it accessible from
> RenderFrameHost? With that approach, the code of this method can be fully
moved
> to WebContentsImpl and there will be no layering violations.
> 
Yeah Moving the code of this method to WebContentsImpl can avoid defining a
common callback. But I'm considering to let WebContentsImpl get the image
download service directly from RenderFrameHostImpl instead of through
RenderFrameHost, as our Mojom definitions are in content/common, RenderFrameHost
in content/public can't reference them.
How about your opinion? @nasko, amistry@, Thanks~

[nasko, 2015-07-03 11:11:31]

On 2015/07/03 03:49:55, leon.han wrote:
> On 2015/07/02 08:41:07, nasko (paris) wrote:
> > Looks good overall, but there are some layering issues that need to be
> resolved.
> > 
> >
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> > File content/browser/frame_host/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> > content/browser/frame_host/DEPS:25:
"+content/public/browser/web_contents.h",
> > This seems wrong. The frame_host layer doesn't and shouldn't know about the
> > web_contents layer. You should define the callback type in a common location
> and
> > have WebContents and this layer both consume it.
> > 
> >
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> > File content/browser/frame_host/render_frame_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
> > content/browser/frame_host/render_frame_host_impl.cc:1890: int
> > RenderFrameHostImpl::DownloadImage(
> > To avoid all the complexity of defining a callback in a common location, why
> not
> > allow access to the image download service by making it accessible from
> > RenderFrameHost? With that approach, the code of this method can be fully
> moved
> > to WebContentsImpl and there will be no layering violations.
> > 
> Yeah Moving the code of this method to WebContentsImpl can avoid defining a
> common callback. But I'm considering to let WebContentsImpl get the image
> download service directly from RenderFrameHostImpl instead of through
> RenderFrameHost, as our Mojom definitions are in content/common,
RenderFrameHost
> in content/public can't reference them.
> How about your opinion? @nasko, amistry@, Thanks~

Yes, calling into RenderFrameHostImpl is what I had in mind. The convention is
that within content/, we can use the *Impl class, since we are in the same
"component". The RenderFrameHost public API is for callers outside of content/.
As such, what you are proposing is totally fine.

[leonhsl(Using Gerrit), 2015-07-06 02:15:26]

uploaded patch set #8, PTAL, Thanks~

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1890: int
RenderFrameHostImpl::DownloadImage(
On 2015/07/02 08:41:07, nasko (paris) wrote:
> To avoid all the complexity of defining a callback in a common location, why
not
> allow access to the image download service by making it accessible from
> RenderFrameHost? With that approach, the code of this method can be fully
moved
> to WebContentsImpl and there will be no layering violations.

Done.

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1085783002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:450: // Download image API
called by web_contents.
On 2015/07/02 08:41:07, nasko (paris) wrote:
> No need to add specifics as to who calls this method, it can be anyone in the
> future, so this type of comment will get stale/incorrect.

Done.

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
File content/renderer/image_downloader/image_downloader_impl.cc (right):

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
content/renderer/image_downloader/image_downloader_impl.cc:38: // Decode the
image using WebKit's image decoder.
On 2015/07/02 08:41:07, nasko (paris) wrote:
> s/WebKit/Blink/

Done.

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
content/renderer/image_downloader/image_downloader_impl.cc:172: if (!frame)
On 2015/07/02 08:41:07, nasko (paris) wrote:
> There shouldn't be a case where render_frame()->GetWebFrame() returns nullptr.
I
> think you can convert this to a DCHECK or just drop it.

Done. Changed to DCHECK.

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
File content/renderer/image_downloader/image_downloader_impl.h (right):

https://codereview.chromium.org/1085783002/diff/120001/content/renderer/image...
content/renderer/image_downloader/image_downloader_impl.h:5: #ifndef
CONTENT_IMAGE_DOWNLOADER_IMPL_H_
On 2015/07/02 08:41:07, nasko (paris) wrote:
> The include guard needs to have the full path and file name:
> CONTENT_RENDERER_IMAGE_DOWNLOADER_IMAGE_DOWNLOADER_IMPL_H_

Done.

https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (right):

https://codereview.chromium.org/1085783002/diff/120001/ipc/ipc_message_start....
ipc/ipc_message_start.h:56: // Note: ImageMsgStart has been removed, we use Mojo
instead.
On 2015/07/02 08:41:07, nasko (paris) wrote:
> Why not remove this then?

Done.

[haltonhuo, 2015-07-06 07:05:16]

The CQ bit was checked by halton.huo@intel.com

[haltonhuo, 2015-07-06 07:05:16]

The patchset sent to the CQ was uploaded after l-g-t-m from amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1085783002/#ps140001
(title: "Fix review comments on set7")

[commit-bot: I haz the power, 2015-07-06 07:05:22]

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

[nasko, 2015-07-06 07:45:23]

https://codereview.chromium.org/1085783002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1085783002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:449: // Returns the Mojo
ImageDownloader service pointer.
You are not returning a pointer, but a const reference to a type. Please drop
the "pointer" part of the comment.

https://codereview.chromium.org/1085783002/diff/140001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/1085783002/diff/140001/content/content.gyp#ne...
content/content.gyp:437: '../skia/skia.gyp:skia_mojo',
Why are the changes in this file needed? Aren't the changes in
content/content_browser.gypi covering this already?

[commit-bot: I haz the power, 2015-07-06 08:17:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-06 08:18:55]

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

[leonhsl(Using Gerrit), 2015-07-06 08:28:08]

On 2015/07/06 07:45:23, nasko (paris) wrote:
>
https://codereview.chromium.org/1085783002/diff/140001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_impl.h (right):
> 
>
https://codereview.chromium.org/1085783002/diff/140001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_impl.h:449: // Returns the Mojo
> ImageDownloader service pointer.
> You are not returning a pointer, but a const reference to a type. Please drop
> the "pointer" part of the comment.
>
Ack.
 
> https://codereview.chromium.org/1085783002/diff/140001/content/content.gyp
> File content/content.gyp (right):
> 
>
https://codereview.chromium.org/1085783002/diff/140001/content/content.gyp#ne...
> content/content.gyp:437: '../skia/skia.gyp:skia_mojo',
> Why are the changes in this file needed? Aren't the changes in
> content/content_browser.gypi covering this already?
This is to fix the compile error occurred in trybot "android_clang_dbg_recipe"
for patch set #7.
I'm not sure about the root cause but I checked around other parts for the mojo
Java compilation and think also it's correct to do like this.
Actually my local compilation for Android can succeed without this change. I
don't know the special configuration for "android_clang_dbg_recipe"....

[leonhsl(Using Gerrit), 2015-07-06 08:32:06]

Sorry I just clicked the commit checkbox to try the buildbot but seems now the
patch has been landed...
I thought this wouldn't happen because it has not gotten looks good from OWNERs.
My misunderstanding.
But now what should I do?  Thanks~

[leonhsl(Using Gerrit), 2015-07-06 08:48:33]

On 2015/07/06 08:28:08, leon.han wrote:
> On 2015/07/06 07:45:23, nasko (paris) wrote:
> >
>
https://codereview.chromium.org/1085783002/diff/140001/content/browser/frame_...
> > File content/browser/frame_host/render_frame_host_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/1085783002/diff/140001/content/browser/frame_...
> > content/browser/frame_host/render_frame_host_impl.h:449: // Returns the Mojo
> > ImageDownloader service pointer.
> > You are not returning a pointer, but a const reference to a type. Please
drop
> > the "pointer" part of the comment.
> >
> Ack.
>  
> > https://codereview.chromium.org/1085783002/diff/140001/content/content.gyp
> > File content/content.gyp (right):
> > 
> >
>
https://codereview.chromium.org/1085783002/diff/140001/content/content.gyp#ne...
> > content/content.gyp:437: '../skia/skia.gyp:skia_mojo',
> > Why are the changes in this file needed? Aren't the changes in
> > content/content_browser.gypi covering this already?
> This is to fix the compile error occurred in trybot "android_clang_dbg_recipe"
> for patch set #7.
> I'm not sure about the root cause but I checked around other parts for the
mojo
> Java compilation and think also it's correct to do like this.
> Actually my local compilation for Android can succeed without this change. I
> don't know the special configuration for "android_clang_dbg_recipe"....

The error happened when compiling 
out/Debug/java_mojo/content_common_mojo_bindings_mojom/src/org/chromium/mojom/image_downloader/DownloadResult.java
It's the java binding source code for image_downloader.mojom, actually it should
not be compiled even for Android build, because we are not using
image_downloader.mojom java binding anywhere. So I don't know why
"android_clang_dbg_recipe" compile it for content_java target.. Then I need to
add the mojom java bindings of skia and geometry, which are referenced in our
image_downloader.mojom java bindings.

[nasko, 2015-07-06 10:00:31]

On 2015/07/06 08:32:06, leon.han wrote:
> Sorry I just clicked the commit checkbox to try the buildbot but seems now the
> patch has been landed...
> I thought this wouldn't happen because it has not gotten looks good from
OWNERs.
> My misunderstanding.
> But now what should I do?  Thanks~

In the future, please don't check the CQ box until you have LGTM from all
owners. There is a CQ dry run you could use if you want to launch a bunch of try
jobs at the same time or use "git cl try" from your command line.

Please prepare a separate patch to address the outstanding issues.

[leonhsl(Using Gerrit), 2015-07-06 10:35:35]

On 2015/07/06 10:00:31, nasko (paris) wrote:
> On 2015/07/06 08:32:06, leon.han wrote:
> > Sorry I just clicked the commit checkbox to try the buildbot but seems now
the
> > patch has been landed...
> > I thought this wouldn't happen because it has not gotten looks good from
> OWNERs.
> > My misunderstanding.
> > But now what should I do?  Thanks~
> 
> In the future, please don't check the CQ box until you have LGTM from all
> owners. There is a CQ dry run you could use if you want to launch a bunch of
try
> jobs at the same time or use "git cl try" from your command line.
> 
> Please prepare a separate patch to address the outstanding issues.

Understood. Will follow the rule strictly in the future.
I'll make a new CL to revise it soon. Thanks~