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

Implement "Save image as" for canvas.

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.

Chromium side:
  https://codereview.chromium.org/263453007/

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

[zino, 2014-04-24 12:24:51]

@junov,

It seems that you are already assigned to the bugs.
I will remove this CL immediately if you are already working.
If not so, Could you please review this?

Thank you :)

[Justin Novosad, 2014-04-24 13:50:15]

On 2014/04/24 12:24:51, zino wrote:
> @junov,
> 
> It seems that you are already assigned to the bugs.
> I will remove this CL immediately if you are already working.
> If not so, Could you please review this?
> 
> Thank you :)

I had not started yet.  But to avoid duplicated effort in the future, please
assign bugs to yourself before starting to work on them.

[Justin Novosad, 2014-04-24 14:19:38]

This UI feature needs to have a test. The test will have to be in a separate
change because it belongs in chromium. See
src/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc

I would like to review the chromium and the blink changes at the same time.

https://codereview.chromium.org/255563004/diff/1/Source/core/html/HTMLCanvasE...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/255563004/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.cpp:408: String
HTMLCanvasElement::toDataURL(const String& mimeType, const double* quality,
ExceptionState& exceptionState)
I know it was like this before, but this method should be const.

https://codereview.chromium.org/255563004/diff/1/Source/core/rendering/HitTes...
File Source/core/rendering/HitTestResult.cpp (right):

https://codereview.chromium.org/255563004/diff/1/Source/core/rendering/HitTes...
Source/core/rendering/HitTestResult.cpp:286: if
(isHTMLCanvasElement(*m_innerNonSharedNode)
This may work, but it is way too cumbersome. It implies generating a potentially
massive data URL just for querying the hit test result.  This can cause a delay
in displaying the contextual menu.  Also, there is no requirement to offer all
the same menu options as we do for image media.  The only options that make
sense in my opinion are "Save Image As" and "Copy Image".  We should not be
offering to copy the image location or to open in a new tab.

https://codereview.chromium.org/255563004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderHTMLCanvas.h (right):

https://codereview.chromium.org/255563004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderHTMLCanvas.h:45: virtual bool isImage() const
OVERRIDE { return true; }
This is not a good idea. It implies that the object is an HTML image, which it
is not.  Instead you should change the relevant call sites to (isImage() ||
isCanvas())

[zino, 2014-04-25 12:04:19]

On 2014/04/24 13:50:15, junov wrote:
> I had not started yet.  But to avoid duplicated effort in the future, please
> assign bugs to yourself before starting to work on them.

Okay, Thank you.

[zino, 2014-04-25 13:21:02]

On 2014/04/24 14:19:38, junov wrote:
> This UI feature needs to have a test. The test will have to be in a separate
> change because it belongs in chromium. See
>
src/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
> 
> I would like to review the chromium and the blink changes at the same time.
> 
>
https://codereview.chromium.org/255563004/diff/1/Source/core/html/HTMLCanvasE...
> File Source/core/html/HTMLCanvasElement.cpp (right):
> 
>
https://codereview.chromium.org/255563004/diff/1/Source/core/html/HTMLCanvasE...
> Source/core/html/HTMLCanvasElement.cpp:408: String
> HTMLCanvasElement::toDataURL(const String& mimeType, const double* quality,
> ExceptionState& exceptionState)
> I know it was like this before, but this method should be const.
> 
>
https://codereview.chromium.org/255563004/diff/1/Source/core/rendering/HitTes...
> File Source/core/rendering/HitTestResult.cpp (right):
> 
>
https://codereview.chromium.org/255563004/diff/1/Source/core/rendering/HitTes...
> Source/core/rendering/HitTestResult.cpp:286: if
> (isHTMLCanvasElement(*m_innerNonSharedNode)
> This may work, but it is way too cumbersome. It implies generating a
potentially
> massive data URL just for querying the hit test result.  This can cause a
delay
> in displaying the contextual menu.  Also, there is no requirement to offer all
> the same menu options as we do for image media.  The only options that make
> sense in my opinion are "Save Image As" and "Copy Image".  We should not be
> offering to copy the image location or to open in a new tab.
> 
>
https://codereview.chromium.org/255563004/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderHTMLCanvas.h (right):
> 
>
https://codereview.chromium.org/255563004/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderHTMLCanvas.h:45: virtual bool isImage() const
> OVERRIDE { return true; }
> This is not a good idea. It implies that the object is an HTML image, which it
> is not.  Instead you should change the relevant call sites to (isImage() ||
> isCanvas())

Thank you for detailed explanation!!
I fully agree with your opinion.
However, I think we need some discussion before re-implementing.

I think that you would like to create data url once when users actually click
"save image" menu or "copy image" menu.
Is my understanding correct?

If so, it seems that we should add a new content API for getting data url on
browser process. (in chromium side)
To save some image, the current chrome(or chromium) uses DownloadManager and url
of the image.
In other words, the browser process have to know url to save the image.
However, creating data url is happen in blink side and then the url is just
passed through IPC from renderer to browser in chromium side.
(RenderFrameImpl::showContextMenu)

To prevent duplicated creating data url, I think that render process should be
just pass empty url to browser process and then we should make a new content API
such as "getDataURL" to defer getting real url.
So, when users click "save image", then the browser process can get a image data
url as calling the new API.

(If you can't understand my opinion because my poor english, I will just share
prototype codes next week. T_T)

I would like to know your opinion.
Please share it if you have a better idea.

Thank you :)

[Justin Novosad, 2014-04-25 15:01:46]

On 2014/04/25 13:21:02, zino wrote:
> 
> Thank you for detailed explanation!!
> I fully agree with your opinion.
> However, I think we need some discussion before re-implementing.
> 
> I think that you would like to create data url once when users actually click
> "save image" menu or "copy image" menu.
> Is my understanding correct?

It would be even better to avoid going through the unnecessary dataURL encoding,
and just encode directly to png. If that requires too much additional work, then
going through a data URL is fine.
> 
> If so, it seems that we should add a new content API for getting data url on
> browser process. (in chromium side)

I think you are right. You could add a new media type. Something like "local
image", which would imply that the image is not a network resource (no loacation
URL, etc.)  Before starting to do that, you should probably discuss it with the
people in chrome/browser/renderer_context_menu/OWNERS

> To save some image, the current chrome(or chromium) uses DownloadManager and
url
> of the image.
> In other words, the browser process have to know url to save the image.
> However, creating data url is happen in blink side and then the url is just
> passed through IPC from renderer to browser in chromium side.
> (RenderFrameImpl::showContextMenu)
> 
> To prevent duplicated creating data url, I think that render process should be
> just pass empty url to browser process and then we should make a new content
API
> such as "getDataURL" to defer getting real url.
> So, when users click "save image", then the browser process can get a image
data
> url as calling the new API.

I think that makes sense but we will need other reviewers' opinions because I am
less familiar with this area.
If you are going to create a new IPC interface for passing the image, then you
should use that opportunity to create an interface that pass the raw png binary
data, rather than a DataURL, which is an intermediate base-64 encoding of the
data.
> 
> (If you can't understand my opinion because my poor english, I will just share
> prototype codes next week. T_T)
> 
> I would like to know your opinion.
> Please share it if you have a better idea.
> 
> Thank you :)

[zino, 2014-04-30 10:54:43]

@junov

I uploaded the patch set 2. (in chromium side)
Please take a look.

Thank you :)

[zino, 2014-04-30 10:55:25]

On 2014/04/30 10:54:43, zino wrote:
> @junov
> 
> I uploaded the patch set 2. (in chromium side)
> Please take a look.
> 
> Thank you :)

Sorry, in blink side!

[zino, 2014-04-30 10:59:12]

+ @dglazkov for */web/*

Please take a look.

Thank you :)

[Justin Novosad, 2014-04-30 18:11:43]

This look great.  The only thing I would like some clarification about is why a
new navigation policy is required. I am less familiar with that part of Blink.

[zino, 2014-05-01 16:01:03]

On 2014/04/30 18:11:43, junov wrote:
> This look great.  The only thing I would like some clarification about is why
a
> new navigation policy is required. I am less familiar with that part of Blink.

Thank you for review.

The policy is required in order to change save location.
(https://codereview.chromium.org/263453007/patch/80001/90012)
If there wasn't the policy, the image would be always saved to the same download
directory.

Thank you :)

[Justin Novosad, 2014-05-01 19:42:59]

On 2014/05/01 16:01:03, zino wrote:
> On 2014/04/30 18:11:43, junov wrote:
> > This look great.  The only thing I would like some clarification about is
why
> a
> > new navigation policy is required. I am less familiar with that part of
Blink.
> 
> Thank you for review.
> 
> The policy is required in order to change save location.
> (https://codereview.chromium.org/263453007/patch/80001/90012)
> If there wasn't the policy, the image would be always saved to the same
download
> directory.
> 
> Thank you :)

lgtm for changes in Source/core

[zino, 2014-05-02 07:08:55]

Ping @dglazkov

Could you please review this?

Thank you.

[dglazkov, 2014-05-06 17:27:19]

lgtm

https://codereview.chromium.org/255563004/diff/30001/public/web/WebContextMen...
File public/web/WebContextMenuData.h (right):

https://codereview.chromium.org/255563004/diff/30001/public/web/WebContextMen...
public/web/WebContextMenuData.h:58: // An canvas node is selected.
"A canvas node"

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

On 2014/05/06 17:27:19, dglazkov wrote:
> lgtm
> 
>
https://codereview.chromium.org/255563004/diff/30001/public/web/WebContextMen...
> File public/web/WebContextMenuData.h (right):
> 
>
https://codereview.chromium.org/255563004/diff/30001/public/web/WebContextMen...
> public/web/WebContextMenuData.h:58: // An canvas node is selected.
> "A canvas node"

Oh.. Thank you!

[zino, 2014-05-07 08:14:03]

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

[commit-bot: I haz the power, 2014-05-07 08:15:28]

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

[commit-bot: I haz the power, 2014-05-07 09:06:20]

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:
  linux_blink_dbg on tryserver.blink

[commit-bot: I haz the power, 2014-05-07 10:16:19]

Change committed as 173505