Implement "Save image as" for canvas (chromium side).

Implement "Save image as" for canvas (chromium side).

Many users want to save the <canvas> as a image file. (like <img>)
As well as being useful, The feature is already supported in Firefox and IE.

Blink side:
  https://codereview.chromium.org/255563004/

BUG=19277, 129710
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269171

[zino, 2014-04-30 10:53:41]

Please take a look.

Thank you :)

[Avi (use Gerrit), 2014-04-30 14:52:28]

+jam for thoughts about the content API.

https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:855: // FIXME:
We should support 'copy image' for canvas.
Chromium style is:

// TODO(zino): We should support 'copy image' for canvas.

https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1559: // FIXME:
We can use SaveImageAt() like a case of canvas.
TODO again. And I do look forward to your fix to make this other code cleaner :)

https://codereview.chromium.org/263453007/diff/20001/content/browser/download...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/263453007/diff/20001/content/browser/download...
content/browser/download/download_manager_impl.cc:123: save_info->file =
params->GetFile();*/
? We don't commit commented-out code.

https://codereview.chromium.org/263453007/diff/20001/content/public/browser/r...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/263453007/diff/20001/content/public/browser/r...
content/public/browser/render_view_host.h:96: virtual void SaveImageAt(int x,
int y) = 0;
I'm feeling weird that we're introducing a "save image" call that's not used for
the usual "save image" mechanism. Are you going to retrofit our existing "save
image" to it?

[Justin Novosad, 2014-04-30 15:07:15]

I realize that implementing the feature this way is a lot more work that your
previous attempt.  Thanks for taking the time to do it the right way with a
dedicated media type.

This change is missing tests.  See:
src/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc

https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:855: // FIXME:
We should support 'copy image' for canvas.
On 2014/04/30 14:52:28, Avi wrote:
> Chromium style is:
> 
> // TODO(zino): We should support 'copy image' for canvas.

You should also file a bug for this so we don't forget about it, and you can
reference the bug in this comment

https://codereview.chromium.org/263453007/diff/20001/content/browser/download...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/263453007/diff/20001/content/browser/download...
content/browser/download/download_manager_impl.cc:60:
//request->SetLoadFlags(request->load_flags() | params->load_flags());
Why was this commented out?

[zino, 2014-05-01 15:39:42]

On 2014/04/30 15:07:15, junov wrote:
> I realize that implementing the feature this way is a lot more work that your
> previous attempt.  Thanks for taking the time to do it the right way with a
> dedicated media type.

Thank you for review :)

> 
> This change is missing tests.  See:
>
src/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc

I included the test as a patch set 3.

> 
>
https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:855: //
FIXME:
> We should support 'copy image' for canvas.
> On 2014/04/30 14:52:28, Avi wrote:
> > Chromium style is:
> > 
> > // TODO(zino): We should support 'copy image' for canvas.
> 
> You should also file a bug for this so we don't forget about it, and you can
> reference the bug in this comment

I added comments.

> 
>
https://codereview.chromium.org/263453007/diff/20001/content/browser/download...
> File content/browser/download/download_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/263453007/diff/20001/content/browser/download...
> content/browser/download/download_manager_impl.cc:60:
> //request->SetLoadFlags(request->load_flags() | params->load_flags());
> Why was this commented out?

Oh, sorry. It's my mistake. T_T
I recovered "download_manager_impl.cc".

Thank you :)

[zino, 2014-05-01 15:45:47]

Thank you for review!

On 2014/04/30 14:52:28, Avi wrote:
> +jam for thoughts about the content API.
> 
>
https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:855: //
FIXME:
> We should support 'copy image' for canvas.
> Chromium style is:
> 
> // TODO(zino): We should support 'copy image' for canvas.
> 
>
https://codereview.chromium.org/263453007/diff/20001/chrome/browser/renderer_...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:1559: //
FIXME:
> We can use SaveImageAt() like a case of canvas.
> TODO again. And I do look forward to your fix to make this other code cleaner
:)

I addressed it. Thank you :)

> 
>
https://codereview.chromium.org/263453007/diff/20001/content/browser/download...
> File content/browser/download/download_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/263453007/diff/20001/content/browser/download...
> content/browser/download/download_manager_impl.cc:123: save_info->file =
> params->GetFile();*/
> ? We don't commit commented-out code.

I'm sorry. It is my mistake.
I recovered the file.

> 
>
https://codereview.chromium.org/263453007/diff/20001/content/public/browser/r...
> File content/public/browser/render_view_host.h (right):
> 
>
https://codereview.chromium.org/263453007/diff/20001/content/public/browser/r...
> content/public/browser/render_view_host.h:96: virtual void SaveImageAt(int x,
> int y) = 0;
> I'm feeling weird that we're introducing a "save image" call that's not used
for
> the usual "save image" mechanism. Are you going to retrofit our existing "save
> image" to it?

The image url is required to save the image currently.
Current "save image" always sends the image url as ContextMenuParam to browser
process through IPC whenever showing context menu.
If the url is short, then it's okay. But if we use data url, the length of the
url may be too long.
(especially canvas always uses data url)
We don't need to send the too long url whenever showing the menu.
So, we've needed a new API.

Thank you.

[Justin Novosad, 2014-05-01 17:40:45]

non-owner lgtm

[Avi (use Gerrit), 2014-05-01 18:10:58]

This LGTM, though can you follow up to route the existing "save image" code
through your new "save image" call?

I'm also curious as to what jam@ says about the interface.

[jam, 2014-05-01 18:13:25]

On 2014/05/01 18:10:58, Avi wrote:
> This LGTM, though can you follow up to route the existing "save image" code
> through your new "save image" call?
> 
> I'm also curious as to what jam@ says about the interface.

fine with me

[zino, 2014-05-07 08:50:12]

On 2014/05/01 18:10:58, Avi wrote:
> This LGTM, though can you follow up to route the existing "save image" code
> through your new "save image" call?

Yes, I can.
But it may be time-consuming.

Thank you :)

[zino, 2014-05-07 10:16:44]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-05-07 10:21:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/263453007/80001

[zino, 2014-05-07 11:19:21]

The CQ bit was unchecked by jinho.bang@samsung.com

[zino, 2014-05-07 11:19:22]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-05-07 11:26:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/263453007/80001

[zino, 2014-05-07 13:34:19]

The CQ bit was unchecked by jinho.bang@samsung.com

[zino, 2014-05-07 13:50:10]

Dear @jln,

This CL seems to need your approval.
(content/common/view_messages.h)

Could you please review?

[dcheng, 2014-05-09 00:25:55]

rslgtm for view_messages.h changes.

[zino, 2014-05-09 00:38:25]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-05-09 00:43:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/263453007/80001

[commit-bot: I haz the power, 2014-05-09 04:32:25]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-09 06:57:22]

Change committed as 269171