Add the ability for keys to be drawn as an image.

Add the ability for keys to be drawn as an image.

Some keys are now drawn as images instead of as text.

R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/5177863dec6169319299a895126e04a3598d87e3

[jamesr, 2015-05-21 23:39:29]

How big are the pngs?

[APW, 2015-05-21 23:51:29]

On 2015/05/21 23:39:29, jamesr wrote:
> How big are the pngs?

64x64 pixels and anywhere from ~350 to 850 bytes

[jamesr, 2015-05-22 00:00:24]

Hmm pretty small, but git is pretty terrible at dealing with binary assets and I
imagine we'll have more/larger ones over time.  git saves space by storing delta
compressed representations of files but it can't do that usefully for binary
blobs so you end up with a copy of every binary file the repo ever had in the
history.  Can you plop the assets into google storage and refer to them by hash?
 That way when the assets do get big we don't have to do anything special.  Look
at how sky/sdk/lib/assets/material-design-icons.sha1 are handled as an example

[qsr, 2015-05-22 15:13:38]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-05-22 15:13:39]

https://codereview.chromium.org/1149293002/diff/1/services/keyboard_native/BU...
File services/keyboard_native/BUILD.gn (right):

https://codereview.chromium.org/1149293002/diff/1/services/keyboard_native/BU...
services/keyboard_native/BUILD.gn:8: embed_file("action_icon") {
maybe we need to expand this to be able to handle multiple files at once?

Or we might want to use something like chromium pak files and embed the pak
file.

[APW, 2015-05-26 22:04:05]

Updated to download images rather than embed them.

PTAL

[jamesr, 2015-05-27 00:17:04]

Sorry, I meant download the assets at sync time but still bake them into the
binary, not download them at run time.  Having them be in the binary is fine - I
just don't want them checked into the repo.  Did you look at the flow for
downloading the material design assets?

[APW, 2015-05-27 20:25:55]

Okay went back to embedding.

PTAL

[jamesr, 2015-05-27 23:30:31]

https://codereview.chromium.org/1149293002/diff/40001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1149293002/diff/40001/.gitignore#newcode72
.gitignore:72: /services/keyboard_native/res/*
if they are all .png files you can say
/services/keyboard_native_res/*.png

which is (imo) slightly more future proof than listing the dir + excluding sha1
in case we need to add READMEs or SVG files or scripts or whatever

https://codereview.chromium.org/1149293002/diff/40001/services/keyboard_nativ...
File services/keyboard_native/key_layout.cc (right):

https://codereview.chromium.org/1149293002/diff/40001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:45: const char* text,
does this use |text| ?

https://codereview.chromium.org/1149293002/diff/40001/services/keyboard_nativ...
File services/keyboard_native/key_layout.h (right):

https://codereview.chromium.org/1149293002/diff/40001/services/keyboard_nativ...
services/keyboard_native/key_layout.h:87: class ImageKey : public TextKey {
why does this inherit from TextKey? inheriting from a concrete class in C++ (as
opposed to an abstract interface) is best avoided as there are lots of corner
cases that behave in really strange ways like accessing a virtual during a
c'tor/d'tor. much better to put common functionality in a type that can be
embedded instead of inherited from

[APW, 2015-05-28 17:54:25]

Addressed comments.

PTAL

https://codereview.chromium.org/1149293002/diff/40001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1149293002/diff/40001/.gitignore#newcode72
.gitignore:72: /services/keyboard_native/res/*
On 2015/05/27 23:30:31, jamesr wrote:
> if they are all .png files you can say
> /services/keyboard_native_res/*.png
> 
> which is (imo) slightly more future proof than listing the dir + excluding
sha1
> in case we need to add READMEs or SVG files or scripts or whatever

Done.

https://codereview.chromium.org/1149293002/diff/40001/services/keyboard_nativ...
File services/keyboard_native/key_layout.cc (right):

https://codereview.chromium.org/1149293002/diff/40001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:45: const char* text,
On 2015/05/27 23:30:31, jamesr wrote:
> does this use |text| ?

|text| is used by the TextKey as a fallback in-case the bitmap is invalid. 
Also, some ImageKeys may represent text which should be emitted if pressed.

https://codereview.chromium.org/1149293002/diff/40001/services/keyboard_nativ...
File services/keyboard_native/key_layout.h (right):

https://codereview.chromium.org/1149293002/diff/40001/services/keyboard_nativ...
services/keyboard_native/key_layout.h:87: class ImageKey : public TextKey {
On 2015/05/27 23:30:31, jamesr wrote:
> why does this inherit from TextKey? inheriting from a concrete class in C++
(as
> opposed to an abstract interface) is best avoided as there are lots of corner
> cases that behave in really strange ways like accessing a virtual during a
> c'tor/d'tor. much better to put common functionality in a type that can be
> embedded instead of inherited from

Done.

[APW, 2015-05-28 17:54:29]

Addressed comments.

PTAL

[APW, 2015-06-02 21:43:30]

anwilson@chromium.org changed reviewers:
+ viettrungluu@chromium.org
- jamesr@chromium.org

[APW, 2015-06-02 21:43:46]

PTAL

[viettrungluu, 2015-06-02 22:29:36]

LGTM w/a collection of nits (and a few other optional suggestions)

https://codereview.chromium.org/1149293002/diff/60001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1149293002/diff/60001/.gitignore#newcode81
.gitignore:81: 
nit: please don't add an extra blank line to the end!

https://codereview.chromium.org/1149293002/diff/60001/DEPS
File DEPS (right):

https://codereview.chromium.org/1149293002/diff/60001/DEPS#newcode372
DEPS:372: 
nit: no blank line here either

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/BUILD.gn (right):

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/BUILD.gn:8: embed_file("action_icon") {
nit: Feel free to put these targets after the others, since they're less
important/interesting.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/BUILD.gn:67: ":action_icon",
nit: Usually, the ":foo" dependencies are listed first. (I'm surprised git cl
upload doesn't complain about requiring "gn format".)

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/key_layout.cc (right):

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:28: // Key implementation.
You can probably skip this comment in the .cc file.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:54: DCHECK(bitmap_.width() > 0);
nit: DCHECK_GT (though I suppose if it fails, you know that bitmap_.width() is
zero, so <shrug>)

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:56: bitmap_valid_ = bitmap_.width() > 0
&& bitmap_.height() > 0;
... though personally I might just do:

bitmap_valid_ = result && bitmap_.width() > 0 && bitmap_.height() > 0;
DCHECK(bitmap_valid_);

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:63: // Key implementation.
...

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/key_layout.h (right):

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.h:17: #include
"third_party/skia/include/core/SkImageDecoder.h"
I don't think you need these includes in the .h, only in the .cc, no?

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.h:93: const mojo::embed::Data& data);
Probably you can just forward declare mojo::embed::Data, rather than including
the header?

Also worth mentioning: You might consider just declaring this in an anonymous
namespace in the .cc file. (This would also avoid any include for SkBitmap.)

(Also note that you can just forward declare TextKey in this file, and put the
full declaration in the .cc file.)

[APW, 2015-06-02 23:16:55]

https://codereview.chromium.org/1149293002/diff/60001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1149293002/diff/60001/.gitignore#newcode81
.gitignore:81: 
On 2015/06/02 22:29:35, viettrungluu wrote:
> nit: please don't add an extra blank line to the end!

Done.

https://codereview.chromium.org/1149293002/diff/60001/DEPS
File DEPS (right):

https://codereview.chromium.org/1149293002/diff/60001/DEPS#newcode372
DEPS:372: 
On 2015/06/02 22:29:35, viettrungluu wrote:
> nit: no blank line here either

Done.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/BUILD.gn (right):

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/BUILD.gn:8: embed_file("action_icon") {
On 2015/06/02 22:29:35, viettrungluu wrote:
> nit: Feel free to put these targets after the others, since they're less
> important/interesting.

Done.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/BUILD.gn:67: ":action_icon",
On 2015/06/02 22:29:35, viettrungluu wrote:
> nit: Usually, the ":foo" dependencies are listed first. (I'm surprised git cl
> upload doesn't complain about requiring "gn format".)

Done.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/key_layout.cc (right):

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:28: // Key implementation.
On 2015/06/02 22:29:35, viettrungluu wrote:
> You can probably skip this comment in the .cc file.

Done.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:54: DCHECK(bitmap_.width() > 0);
On 2015/06/02 22:29:35, viettrungluu wrote:
> nit: DCHECK_GT (though I suppose if it fails, you know that bitmap_.width() is
> zero, so <shrug>)

Done.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:56: bitmap_valid_ = bitmap_.width() > 0
&& bitmap_.height() > 0;
On 2015/06/02 22:29:35, viettrungluu wrote:
> ... though personally I might just do:
> 
> bitmap_valid_ = result && bitmap_.width() > 0 && bitmap_.height() > 0;
> DCHECK(bitmap_valid_);

Done.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.cc:63: // Key implementation.
On 2015/06/02 22:29:35, viettrungluu wrote:
> ...

Done.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/key_layout.h (right):

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.h:17: #include
"third_party/skia/include/core/SkImageDecoder.h"
On 2015/06/02 22:29:35, viettrungluu wrote:
> I don't think you need these includes in the .h, only in the .cc, no?

Done.

https://codereview.chromium.org/1149293002/diff/60001/services/keyboard_nativ...
services/keyboard_native/key_layout.h:93: const mojo::embed::Data& data);
On 2015/06/02 22:29:35, viettrungluu wrote:
> Probably you can just forward declare mojo::embed::Data, rather than including
> the header?
> 
> Also worth mentioning: You might consider just declaring this in an anonymous
> namespace in the .cc file. (This would also avoid any include for SkBitmap.)
> 
> (Also note that you can just forward declare TextKey in this file, and put the
> full declaration in the .cc file.)

Done.

[APW, 2015-06-02 23:17:37]

Committed patchset #5 (id:80001) manually as
5177863dec6169319299a895126e04a3598d87e3 (presubmit successful).