Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
270613004:
Implement "Copy image" for canvas (blink side). (Closed)
|
Created:
6 years, 7 months ago by zino Modified:
6 years, 5 months ago CC:
blink-reviews, groby+blinkspell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionImplement "Copy image" for canvas (blink side).
Many users want to copy the image of <canvas> to clipboard. (like <img>)
As well as being useful, The feature is already supported in Firefox and IE.
Chromium side:
https://codereview.chromium.org/275833002/
BUG=369092
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177547
Patch Set 1 : #Patch Set 2 : compile error: isNULL->isNull #
Total comments: 1
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 7
Patch Set 5 : #Patch Set 6 : add test #
Total comments: 8
Patch Set 7 : nits #
Total comments: 6
Patch Set 8 : addressed nits #
Messages
Total messages: 31 (0 generated)
Please take a look. (I'll upload the chromium side patch as well) Thank you :)
Ping Owners! I've just upload the chromium side patch as well. https://codereview.chromium.org/275833002/ Please review this CL (in blink side) Thank you :)
https://codereview.chromium.org/270613004/diff/60001/Source/core/editing/Edit... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/270613004/diff/60001/Source/core/editing/Edit... Source/core/editing/Editor.cpp:431: if (renderer) How does this even work? Shouldn't this be 'if (!renderer)'
On 2014/05/09 14:57:33, junov wrote: > https://codereview.chromium.org/270613004/diff/60001/Source/core/editing/Edit... > File Source/core/editing/Editor.cpp (right): > > https://codereview.chromium.org/270613004/diff/60001/Source/core/editing/Edit... > Source/core/editing/Editor.cpp:431: if (renderer) > How does this even work? Shouldn't this be 'if (!renderer)' Oh, really sorry! It is a critical mistake :( I addressed it and I modified to not create a data URL. Please review again?
https://codereview.chromium.org/270613004/diff/80001/Source/core/editing/Edit... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/270613004/diff/80001/Source/core/editing/Edit... Source/core/editing/Editor.cpp:457: urlString = AtomicString("data:image/png"); This URL does not seem useful. Why not leave urlString empty?
copyImage() copies a binary data of image, url, and title to clipboard. The binary data of image(real image) is pasted in other applications through ctrl + v or paste menu. Then the url can be used as fallback if the application (like gedit, notepad) doesn't support a image. In case of canvas unlike case of image, the copied url is always a data url and it is usually too large and not useful. So, I think we don't need to send a data url because copy image is *not* copy url of image. NOTE: the url is just fallback url and in that case, data URL is not useful. On 2014/05/09 19:56:37, junov wrote: > https://codereview.chromium.org/270613004/diff/80001/Source/core/editing/Edit... > File Source/core/editing/Editor.cpp (right): > > https://codereview.chromium.org/270613004/diff/80001/Source/core/editing/Edit... > Source/core/editing/Editor.cpp:457: urlString = AtomicString("data:image/png"); > This URL does not seem useful. Why not leave urlString empty? The reason is that copy image is failed if the url is empty or not valid. Thank you :)
On 2014/05/10 00:23:48, zino wrote: > copyImage() copies a binary data of image, url, and title to clipboard. > The binary data of image(real image) is pasted in other applications through > ctrl + v or paste menu. > Then the url can be used as fallback if the application (like gedit, notepad) > doesn't support a image. > In case of canvas unlike case of image, the copied url is always a data url and > it is usually too large and not useful. > So, I think we don't need to send a data url because copy image is *not* copy > url of image. > > NOTE: the url is just fallback url and in that case, data URL is not useful. > > On 2014/05/09 19:56:37, junov wrote: > > > https://codereview.chromium.org/270613004/diff/80001/Source/core/editing/Edit... > > File Source/core/editing/Editor.cpp (right): > > > > > https://codereview.chromium.org/270613004/diff/80001/Source/core/editing/Edit... > > Source/core/editing/Editor.cpp:457: urlString = > AtomicString("data:image/png"); > > This URL does not seem useful. Why not leave urlString empty? > > The reason is that copy image is failed if the url is empty or not valid. > > Thank you :) I see, but this is not a valid data URL since it is missing a data field (see http://tools.ietf.org/html/rfc2397 ). If it ends up being used as a fallback, it may cause a parsing error in the application that tries to use it. Perhaps the fallback should be something like a plain text data URL that says "[image]" or something like that. I don't know if there is a precedent for this kind of situation. Dimitri, what do you think? Do we need an additional reviewer here?
Ping Reviewers. I mentioned "the url can be used as fallback if the application doesn't support a image." But I'm not sure that. So, I added @yosi for editing review. Nevertheless, IMHO the url doesn't still seem to be useful. So, I agree with @junov's comment and I've already modified the URL string. Could you please review again?
On 2014/06/19 10:51:04, zino wrote: > Ping Reviewers. > > I mentioned "the url can be used as fallback if the application doesn't support > a image." > But I'm not sure that. So, I added @yosi for editing review. > > Nevertheless, IMHO the url doesn't still seem to be useful. > So, I agree with @junov's comment and I've already modified the URL string. > > Could you please review again? I have no further comments, but I would like another reviewer to chime-in. In particular regarding the fallback URL.
Could you add layout test for this patch? Thanks in advance. https://codereview.chromium.org/270613004/diff/100001/Source/core/editing/Edi... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/270613004/diff/100001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:430: RenderObject* renderer = node->renderer(); It seems it is better to call |node->document().updateLayoutIgnorePendingStylesheets()| to make sure |node| to have |RenderObject|. https://codereview.chromium.org/270613004/diff/100001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:434: Image* image = 0; nit: How about having helper function to return |Image| from |node|? Image* image = imageFromNode(node); if (!image) return; static Image* imageFromNode(const Node* node) { node->document().updateLayoutIgnorePendingStylesheets(); RenderObject* renderer = node->renderer(); if (!renderer || !renderer->isImage()) return nullptr; if (RenderImage* renderImage = toRenderImage(renderer)) { ImageResource* cachedImage = renderImage->cachedImage(); if (!cachedImage || cachedImage->errorOccurred()) return nullptr; return cachedImage->imageForRenderer(renderImage); } if (renderer->isCanvas()) return toHTMLCanvasElement(node)->copiedImage(); return nullptr; } https://codereview.chromium.org/270613004/diff/100001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:457: urlString = AtomicString("data:,[canvas]"); Just curiosity, what "data:,[canvas]" is? Where do we expand them real URL? https://codereview.chromium.org/270613004/diff/100001/Source/web/ContextMenuC... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/270613004/diff/100001/Source/web/ContextMenuC... Source/web/ContextMenuClientImpl.cpp:235: data.hasImageContents = r.image() && !(r.image()->isNull()); nit: We don't need to have parenthesis, |!r.image()->isNull()| is fine.
Dear all, I mentioned that sending a full data url is not needed but it isn't right. Many native apps is okay but some JS app or service has some troubles. (i.e. : http://snag.gy) Sorry for the confusion. And, I updated a new patch set. Could you please review again? https://codereview.chromium.org/270613004/diff/100001/Source/core/editing/Edi... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/270613004/diff/100001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:430: RenderObject* renderer = node->renderer(); On 2014/06/20 04:24:10, yosi wrote: > It seems it is better to call > |node->document().updateLayoutIgnorePendingStylesheets()| to make sure |node| to > have |RenderObject|. Done. https://codereview.chromium.org/270613004/diff/100001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:434: Image* image = 0; On 2014/06/20 04:24:10, yosi wrote: > nit: How about having helper function to return |Image| from |node|? > > Image* image = imageFromNode(node); > if (!image) > return; > > static Image* imageFromNode(const Node* node) > { > node->document().updateLayoutIgnorePendingStylesheets(); > RenderObject* renderer = node->renderer(); > if (!renderer || !renderer->isImage()) > return nullptr; > if (RenderImage* renderImage = toRenderImage(renderer)) { > ImageResource* cachedImage = renderImage->cachedImage(); > if (!cachedImage || cachedImage->errorOccurred()) > return nullptr; > return cachedImage->imageForRenderer(renderImage); > } > if (renderer->isCanvas()) > return toHTMLCanvasElement(node)->copiedImage(); > return nullptr; > } Done. https://codereview.chromium.org/270613004/diff/100001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:457: urlString = AtomicString("data:,[canvas]"); On 2014/06/20 04:24:10, yosi wrote: > Just curiosity, what "data:,[canvas]" is? Where do we expand them real URL? I thought that the URL is not needed in the case of canvas because it is always large dataURL. Many native app uses binary image data usually but some JS app or service seems to use dataURL instead of binary data. So, I updated related codes.
And Can we create layout tests about this CL? I think this CL is dependent on https://codereview.chromium.org/275833002/ (chromium side patch). So, after landing two CLs, it is possible. (maybe manual tests instead of layout tests) Thank you :)
On 2014/06/20 08:42:22, zino wrote: > And > > Can we create layout tests about this CL? > I think this CL is dependent on https://codereview.chromium.org/275833002/ > (chromium side patch). > So, after landing two CLs, it is possible. (maybe manual tests instead of layout > tests) > > Thank you :) You can write layout test with CANVAS element + document.execCommand("copy") rather than invoking from context menu. It seems context menu part is handled by browser test.
On 2014/06/20 08:57:13, yosi wrote:
> You can write layout test with CANVAS element + document.execCommand("copy")
> rather than invoking from context menu.
> It seems context menu part is handled by browser test.
Thank you for review.
I'm not sure but |copy| seems to be different with |copy image|.
In general, |copy| means |selection copy| which just copies HTML contents.
However, |copy image| means that real binary image is copied.
For example.. If we would try to copy dynamic GIF image to google docs..
|copy| - the image is still dynamic.
|copy image| - the image will be fixed because the binary is JPEG or PNG.
Exceptionally, if we would use |copy| on image document,
it will work exactly the same as |copy image|.
So, in the case of <canvas> tag,
|copy| is unnecessary because it can't have alt or src attributes to be able to
represent the image.
As a result, I think we can make a manual tests after landing patches.
On 2014/06/20 13:49:56, zino wrote:
> On 2014/06/20 08:57:13, yosi wrote:
> > You can write layout test with CANVAS element + document.execCommand("copy")
> > rather than invoking from context menu.
> > It seems context menu part is handled by browser test.
>
> Thank you for review.
>
> I'm not sure but |copy| seems to be different with |copy image|.
> In general, |copy| means |selection copy| which just copies HTML contents.
> However, |copy image| means that real binary image is copied.
>
> For example.. If we would try to copy dynamic GIF image to google docs..
> |copy| - the image is still dynamic.
> |copy image| - the image will be fixed because the binary is JPEG or PNG.
>
> Exceptionally, if we would use |copy| on image document,
> it will work exactly the same as |copy image|.
>
> So, in the case of <canvas> tag,
> |copy| is unnecessary because it can't have alt or src attributes to be able
to
> represent the image.
>
> As a result, I think we can make a manual tests after landing patches.
I see. Editor::copyImage() is called by WebViewImpl::copyImageAt. So, we can
write gtest for it.
There are two options, one create core/editing/EditorTest.cpp, another is
web/tests/WebViewTest.cpp.
I recommend EditorTest.cpp, since it needs fewer initialization and localized.
We have some gtest, core/editing/*Test.cpp.
@Yosi, I've just uploaded a test. Could you please review again?
The CQ bit was checked by yosin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/270613004/160001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
LGTM w/ tiny nits Please wait for OWNERS review for committing. https://codereview.chromium.org/270613004/diff/160001/Source/core/editing/Edi... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/270613004/diff/160001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:431: if (renderer->isCanvas()) { nit: We don't need to have "{}" for single then clause. https://codereview.chromium.org/270613004/diff/160001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:437: return 0; optional nit: It is better to use |nullptr|. https://codereview.chromium.org/270613004/diff/160001/Source/web/tests/data/c... File Source/web/tests/data/canvas-copy-image.html (right): https://codereview.chromium.org/270613004/diff/160001/Source/web/tests/data/c... Source/web/tests/data/canvas-copy-image.html:3: <head> nit: We don't need to have empty HEAD element. https://codereview.chromium.org/270613004/diff/160001/Source/web/tests/data/c... Source/web/tests/data/canvas-copy-image.html:6: <canvas width="200" height="200"></canvas> nit: We don't need to indent HTML.
Thank you for review. https://codereview.chromium.org/270613004/diff/160001/Source/core/editing/Edi... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/270613004/diff/160001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:431: if (renderer->isCanvas()) { On 2014/06/30 09:17:08, Yosi_UTC9 wrote: > nit: We don't need to have "{}" for single then clause. Done. https://codereview.chromium.org/270613004/diff/160001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:437: return 0; On 2014/06/30 09:17:08, Yosi_UTC9 wrote: > optional nit: It is better to use |nullptr|. Done. I also think it is better. BTW, can we use it? (Blink coding guideline says to have to use 0 in C++ yet) https://codereview.chromium.org/270613004/diff/160001/Source/web/tests/data/c... File Source/web/tests/data/canvas-copy-image.html (right): https://codereview.chromium.org/270613004/diff/160001/Source/web/tests/data/c... Source/web/tests/data/canvas-copy-image.html:3: <head> On 2014/06/30 09:17:08, Yosi_UTC9 wrote: > nit: We don't need to have empty HEAD element. Done. https://codereview.chromium.org/270613004/diff/160001/Source/web/tests/data/c... Source/web/tests/data/canvas-copy-image.html:6: <canvas width="200" height="200"></canvas> On 2014/06/30 09:17:08, Yosi_UTC9 wrote: > nit: We don't need to indent HTML. Done.
@tkent, Could you take a look as the owner of web/* or core/*?
lgtm https://codereview.chromium.org/270613004/diff/180001/Source/core/editing/Edi... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/270613004/diff/180001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:424: static Image* imageFromNode(const Node* node) The argument type should be |const Node&| because it can't be null. https://codereview.chromium.org/270613004/diff/180001/Source/web/ContextMenuC... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/270613004/diff/180001/Source/web/ContextMenuC... Source/web/ContextMenuClientImpl.cpp:234: // An image can to be null for many reasons, like being blocked, no image can to be -> can be? https://codereview.chromium.org/270613004/diff/180001/Source/web/tests/data/c... File Source/web/tests/data/canvas-copy-image.html (right): https://codereview.chromium.org/270613004/diff/180001/Source/web/tests/data/c... Source/web/tests/data/canvas-copy-image.html:6: var canvas = document.querySelector("canvas"); Please do not indent the whole content of <script>. It should be: <script> var canvas = ... var context = ...
Thank you for review. https://codereview.chromium.org/270613004/diff/180001/Source/core/editing/Edi... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/270613004/diff/180001/Source/core/editing/Edi... Source/core/editing/Editor.cpp:424: static Image* imageFromNode(const Node* node) On 2014/06/30 22:59:46, tkent wrote: > The argument type should be |const Node&| because it can't be null. Done. https://codereview.chromium.org/270613004/diff/180001/Source/web/ContextMenuC... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/270613004/diff/180001/Source/web/ContextMenuC... Source/web/ContextMenuClientImpl.cpp:234: // An image can to be null for many reasons, like being blocked, no image On 2014/06/30 22:59:46, tkent wrote: > can to be -> can be? Done. https://codereview.chromium.org/270613004/diff/180001/Source/web/tests/data/c... File Source/web/tests/data/canvas-copy-image.html (right): https://codereview.chromium.org/270613004/diff/180001/Source/web/tests/data/c... Source/web/tests/data/canvas-copy-image.html:6: var canvas = document.querySelector("canvas"); On 2014/06/30 22:59:46, tkent wrote: > Please do not indent the whole content of <script>. > It should be: > > <script> > var canvas = ... > var context = ... Done.
@junov Would you mind if I commit this?
On 2014/07/03 10:33:35, zino wrote: > @junov > > Would you mind if I commit this? Ping Justin, This is a kind of canvas feature. Could you take a look finall? Thanks :)
lgtm
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/270613004/200001
Message was sent while issue was closed.
Change committed as 177547 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
