Add UMA to HTMLCanvasElement to track image formats usage of toDataURL/toBlob

Add UMA to HTMLCanvasElement to track image formats usage of toDataURL/toBlob

BUG=596117
Committed: https://crrev.com/7b4d6fe2b7070127175b4086fc100e7c83dfbf10
Cr-Commit-Position: refs/heads/master@{#383176}

[xlai (Olivia), 2016-03-21 17:44:58]

Description was changed from

==========
Add UMA to HTMLCanvasElement to track image formats usage of toDataURL/toBlob

BUG=596117
==========

to

==========
Add UMA to HTMLCanvasElement to track image formats usage of toDataURL/toBlob

BUG=596117
==========

[xlai (Olivia), 2016-03-21 17:44:58]

xlai@chromium.org changed reviewers:
+ junov@chromium.org

[xlai (Olivia), 2016-03-21 18:04:13]

xlai@chromium.org changed reviewers:
+ jwd@chromium.org

[xlai (Olivia), 2016-03-21 20:29:36]

Getting the usage of different image formats (e.g., png, jpeg, etc.) as returned
from canvas is a useful information to understand the trend of user demand. In
this patch I add a new UMA to collect this kind of information for both
toDataURL and toBlob (callback version) in HTMLCanvasElement. One more suffix
can be added in the future when toBlob (promise version) in OffscreenCanvas is
implemented.

[Justin Novosad, 2016-03-22 15:09:04]

https://codereview.chromium.org/1820813004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1820813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:510: } else if
(lowercaseMimeType == "image/bmp") {
There is also an alternate MIME type for bmp: image/x-windows-bmp

https://codereview.chromium.org/1820813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:512: } else if
(lowercaseMimeType == "image/ico") {
this is not a valid MIME type. should be "image/x-icon"

https://codereview.chromium.org/1820813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:514: } else if
(lowercaseMimeType == "image/tiff") {
can also be image/x-tiff

https://codereview.chromium.org/1820813004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1820813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:188: enum
CanvasReturnImageFormat {
This does not need to be in the header. Also, why "CanvasReturn"?  IMHO, a mor
appropriate name would be "RequestedImageMimeType"

https://codereview.chromium.org/1820813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:189: Png = 0,
enum members should have the enum name as a prefix. Something like
RequestedImageMimeTypeJpeg

https://codereview.chromium.org/1820813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:196:
NumberOfImageFormats
You should add an "Unknown" value

[xlai (Olivia), 2016-03-22 17:19:33]

Changes based on feedback.

[Justin Novosad, 2016-03-22 18:19:53]

https://codereview.chromium.org/1820813004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1820813004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:597: String
encodingMimeType = toEncodingMimeType(mimeType, "toDataURL");
Replace this string arg with an enum

[xlai (Olivia), 2016-03-22 19:42:37]

Changes based on second feedback

[Justin Novosad, 2016-03-22 20:23:02]

https://codereview.chromium.org/1820813004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1820813004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:101: enum
RequestImageFunctionCallName {
Naming nit, you can be more concise here: EncodeReason. Consistent with other
uses of "Reason" throughout blink.

https://codereview.chromium.org/1820813004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:102: ToDataURL = 0,
EncodeReasonToDataURL

[xlai (Olivia), 2016-03-22 20:44:48]

Nits

[jwd, 2016-03-23 19:05:13]

https://codereview.chromium.org/1820813004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1820813004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:495: enum
RequestedImageMimeType {
Can you add a comment that this enum is used for a histogram, and the values
should not be changed.

[Justin Novosad, 2016-03-23 19:35:48]

https://codereview.chromium.org/1820813004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1820813004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:533:
DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram,
toDataURLImageFormatHistogram, new
EnumerationHistogram("Canvas.RequestedImageMimeTypes_toDataURL",
NumberOfRequestedImageMimeTypes));
Are you sure it is correct to use "_" as the suffix separator? I thought we used
"."

[xlai (Olivia), 2016-03-23 19:42:06]

jwd@: Adding comments based on feedback.

juno@: Yes, in the beginning of tools/metrics/histogram.xml, there is a long
block of comments giving instructions of how to use suffix on histogram; the
example it uses indicate that the histogram suffix is separated by "_". I
double-checked with some existing histogram suffix in the code base and confirm
that.

[Justin Novosad, 2016-03-23 20:20:38]

core/ lgtm

[jwd, 2016-03-24 20:59:07]

lgtm

[xlai (Olivia), 2016-03-24 21:00:09]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2016-03-24 21:00:45]

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

[commit-bot: I haz the power, 2016-03-24 22:59:50]

Description was changed from

==========
Add UMA to HTMLCanvasElement to track image formats usage of toDataURL/toBlob

BUG=596117
==========

to

==========
Add UMA to HTMLCanvasElement to track image formats usage of toDataURL/toBlob

BUG=596117
==========

[commit-bot: I haz the power, 2016-03-24 22:59:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-24 23:01:41]

Description was changed from

==========
Add UMA to HTMLCanvasElement to track image formats usage of toDataURL/toBlob

BUG=596117
==========

to

==========
Add UMA to HTMLCanvasElement to track image formats usage of toDataURL/toBlob

BUG=596117

Committed: https://crrev.com/7b4d6fe2b7070127175b4086fc100e7c83dfbf10
Cr-Commit-Position: refs/heads/master@{#383176}
==========

[commit-bot: I haz the power, 2016-03-24 23:01:42]

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