CL for readability review

CL for readability review


BUG=NONE
TEST=NONE

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91808

[brettw, 2011-06-27 18:14:48]

(Sending mail so this appears in my inbox.)

[brettw, 2011-06-29 20:31:56]

Here's my first pass of comments.
                                                                                
Overall, this code looks good.  I know there are a number of comments here, but
don't let that disturb you - it's our job as readability reviewers to point out
every style concern we can find, no matter what.
                                                                                
Please remember that the purpose of this review is not to be a commentary on
your ability to write code, but to ensure that you understand Google's style
guidelines, which aren't necessarily better or worse than what you may be used
to, but are almost certainly different and can take some getting used to.
                                                                                
Note that this isn't a code review: I didn't thoroughly inspect your code for
correctness, that's the job of the engineers who work in the area it applies to.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc
File views/ime/character_composer.cc (right):

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:19: ComposeChecker(const uint16* data, int
max_sequence_length, int n_sequences);
Can you clarify the ownership of "data" in a comment here? It looks like you
require this to live as long as the class, which is something that should be
documented clearly.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:26: const uint16* data_;
I also like to make the ownership of member pointers clear. In this case, just a
"// Lifetime managed by class creator." or something would be sufficient (so
people don't think you need to delete it in the destructor).

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:77: typedef std::vector<unsigned int> KeyType;
This typedef appears 3 times, does it make sense to just make it a member of
ComposeChecker and use it in all 3 places? This may be unnecessary depending on
the answer to my bsearch question below.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:145: bsearch(&sequence, table, n_sequences,
This bsearch stuff is kind of awkward because you don't have the typesafety and
need to do casts, etc. Did you consider std::lower_bound, which will operate on
the containers directly in a typesafe manner?

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:186: #include
"third_party/gtk+/gtk/gtkimcontextsimpleseqs.h"
It's not clear to me why this include is here in the middle of the file. Can it
be at the top with the regular ones? Does it need the typedef you provide?

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:239: cedilla_compose_seqs, 4,
arraysize(cedilla_compose_seqs)/(4 + 2));
Wrapped lines should be indented 4 spaces.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:252: }  // anonymous namespace
You don't need the "anonymous" here, just "// namespace"
http://www/eng/doc/cppguide.xml?external=y&showone=Namespace_Formatting#Names...

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:286: compose_buffer_.pop_back(); // remove the
keypress added this time
Please be sure all your comments begin with a capital letter and end with a
period:
http://www/eng/doc/cppguide.xml?external=y&showone=Punctuation,_Spelling_and_...

Also, in this case, end-of-line comments should have two spaces before the "//"

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:295: //
Be sure to delete this once you've made other changes to this file.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h
File views/ime/character_composer.h (right):

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h#...
views/ime/character_composer.h:9: #include <vector>
Put a blank line after this (separating the "sections" of the headers like you
did in the .cc file):
http://www/eng/doc/cppguide.xml?external=y&showone=Names_and_Order_of_Include...

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h#...
views/ime/character_composer.h:27: // Filters keypress.  Returns true if the
keypress is handled.
Can you clarify in this comment what "handled" means? Does that mean it was a
dead key and it was added to the buffer, or that it wasn't relevant to the
character composer, etc.?

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h#...
views/ime/character_composer.h:32: const string16& GetComposedCharacter() const
{
This is valid according to the style guide, but I think in Chrome it's normal to
use the composed_character() style of naming inline getters (unless the function
is virtual or something).

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h#...
views/ime/character_composer.h:37: std::vector<unsigned int> compose_buffer_;
Can you document these two members? They aren't blindingly obvious to me just
looking at this header.

[hashimoto, 2011-06-30 07:19:21]

Thank you for taking time for my readability review request.
This is a new patch set with fixes.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc
File views/ime/character_composer.cc (right):

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:19: ComposeChecker(const uint16* data, int
max_sequence_length, int n_sequences);
On 2011/06/29 20:31:56, brettw wrote:
> Can you clarify the ownership of "data" in a comment here? It looks like you
> require this to live as long as the class, which is something that should be
> documented clearly.

Fixed

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:26: const uint16* data_;
On 2011/06/29 20:31:56, brettw wrote:
> I also like to make the ownership of member pointers clear. In this case, just
a
> "// Lifetime managed by class creator." or something would be sufficient (so
> people don't think you need to delete it in the destructor).

Fixed

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:77: typedef std::vector<unsigned int> KeyType;
On 2011/06/29 20:31:56, brettw wrote:
> This typedef appears 3 times, does it make sense to just make it a member of
> ComposeChecker and use it in all 3 places? This may be unnecessary depending
on
> the answer to my bsearch question below.

typedef std::vector<unsigned int> ComposeBufferType;
is added to the top of the anonymous namespace

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:145: bsearch(&sequence, table, n_sequences,
On 2011/06/29 20:31:56, brettw wrote:
> This bsearch stuff is kind of awkward because you don't have the typesafety
and
> need to do casts, etc. Did you consider std::lower_bound, which will operate
on
> the containers directly in a typesafe manner?

I use ugly bsearch instead of typesafe STL algorithms here because I have to
search on a two-dimensional array and coding style prohibits operator
overloading.
Using STL algorithms on two-dimensional array requires implementing customized
iterator, but I cannot implement iterators without operator overloading.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:186: #include
"third_party/gtk+/gtk/gtkimcontextsimpleseqs.h"
On 2011/06/29 20:31:56, brettw wrote:
> It's not clear to me why this include is here in the middle of the file. Can
it
> be at the top with the regular ones? Does it need the typedef you provide?
Added #include "ui/base/gtk/gtk_integers.h" instead of typedef.
Added comment to describe why this file is included here.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:239: cedilla_compose_seqs, 4,
arraysize(cedilla_compose_seqs)/(4 + 2));
On 2011/06/29 20:31:56, brettw wrote:
> Wrapped lines should be indented 4 spaces.

Fixed

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:252: }  // anonymous namespace
On 2011/06/29 20:31:56, brettw wrote:
> You don't need the "anonymous" here, just "// namespace"
>
http://www/eng/doc/cppguide.xml?external=y&showone=Namespace_Formatting#Names...

Fixed all two

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:286: compose_buffer_.pop_back(); // remove the
keypress added this time
On 2011/06/29 20:31:56, brettw wrote:
> Please be sure all your comments begin with a capital letter and end with a
> period:
>
http://www/eng/doc/cppguide.xml?external=y&showone=Punctuation%2C_Spelling_an...
> 
> Also, in this case, end-of-line comments should have two spaces before the
"//"

Fixed

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:295: //
On 2011/06/29 20:31:56, brettw wrote:
> Be sure to delete this once you've made other changes to this file.

Deleted all three.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h
File views/ime/character_composer.h (right):

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h#...
views/ime/character_composer.h:9: #include <vector>
On 2011/06/29 20:31:56, brettw wrote:
> Put a blank line after this (separating the "sections" of the headers like you
> did in the .cc file):
>
http://www/eng/doc/cppguide.xml?external=y&showone=Names_and_Order_of_Include...

Fixed

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h#...
views/ime/character_composer.h:27: // Filters keypress.  Returns true if the
keypress is handled.
On 2011/06/29 20:31:56, brettw wrote:
> Can you clarify in this comment what "handled" means? Does that mean it was a
> dead key and it was added to the buffer, or that it wasn't relevant to the
> character composer, etc.?

Fixed

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h#...
views/ime/character_composer.h:32: const string16& GetComposedCharacter() const
{
On 2011/06/29 20:31:56, brettw wrote:
> This is valid according to the style guide, but I think in Chrome it's normal
to
> use the composed_character() style of naming inline getters (unless the
function
> is virtual or something).

Fixed

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.h#...
views/ime/character_composer.h:37: std::vector<unsigned int> compose_buffer_;
On 2011/06/29 20:31:56, brettw wrote:
> Can you document these two members? They aren't blindingly obvious to me just
> looking at this header.

Fixed

[brettw, 2011-07-01 22:21:18]

Thanks for the fixes, here's my second pass.

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc
File views/ime/character_composer.cc (right):

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:145: bsearch(&sequence, table, n_sequences,
A raw pointer is actually a valid iterator for STL, we do this in several
places:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/ui/base/keycodes/keyb...
This code uses an operator< which, as you point out, it shouldn't do. But you
can pass a comparator functor as the 4th argument to lower_bound like this code
does:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/histor...

[hashimoto, 2011-07-04 04:45:20]

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc
File views/ime/character_composer.cc (right):

http://codereview.chromium.org/7264015/diff/1/views/ime/character_composer.cc...
views/ime/character_composer.cc:145: bsearch(&sequence, table, n_sequences,
On 2011/07/01 22:21:18, brettw wrote:
> A raw pointer is actually a valid iterator for STL, we do this in several
> places:
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/ui/base/keycodes/keyb...
> This code uses an operator< which, as you point out, it shouldn't do. But you
> can pass a comparator functor as the 4th argument to lower_bound like this
code
> does:
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/histor...

Yes, that's true about operator<, but I could not find a way to handle the
stride without operator++.
If I was allowed to use operator overloading, I would do something like below to
implement customized iterator (please forget about miscellaneous typedefs and
methods).

class MyIterator {
 public:
  MyIterator(uint16* ptr, int stride) : ptr(ptr), stride(stride) {}
  MyIterator& operator++() {
    ptr += stride;
    return *this;
  }
  uint16* operator*() {return ptr;}
 private:
  uint16* ptr;
  int stride;
};

[brettw, 2011-07-04 05:36:04]

I missed that part about the stride. Can you clarify that? It seems like the
ComposeChecker class could use a comment explaining what it does and how it
works. The parameters you pass to the constructor when you actually use it look
pretty obscure to me just looking at it now.

Since the data we're searching isn't our data and we can't change the format to
be something more compatible with pointer-based interation, I think we should
actually write an iterator. The goal of the style guide is to increase
readability, and trying to avoid a simple operator++ here seems like it's
actually hurting readability quite a bit. It also helps that this will be
internal to a .cc file so won't confuse people working in other files who aren't
expecting operators. What do you think?

[hashimoto, 2011-07-04 06:18:36]

Thank you very much for your reply in Sunday night.

On 2011/07/04 05:36:04, brettw wrote:
> I missed that part about the stride. Can you clarify that? It seems like the
> ComposeChecker class could use a comment explaining what it does and how it
> works. The parameters you pass to the constructor when you actually use it
look
> pretty obscure to me just looking at it now.
Thank you for pointing it out.
It seeems that the part really needs a detailed description.

> Since the data we're searching isn't our data and we can't change the format
to
> be something more compatible with pointer-based interation, I think we should
> actually write an iterator. The goal of the style guide is to increase
> readability, and trying to avoid a simple operator++ here seems like it's
> actually hurting readability quite a bit. It also helps that this will be
> internal to a .cc file so won't confuse people working in other files who
aren't
> expecting operators. What do you think?
Pretty make sense.
I will try that.

[hashimoto, 2011-07-06 07:10:57]

Third patch set, added comment to describe the format of composition table data
and implemented SequenceIterator to use std::lower_bound instead of bsearch

[brettw, 2011-07-06 16:51:24]

http://codereview.chromium.org/7264015/diff/13003/views/ime/character_compose...
File views/ime/character_composer.cc (right):

http://codereview.chromium.org/7264015/diff/13003/views/ime/character_compose...
views/ime/character_composer.cc:87: inline bool operator>=(const
SequenceIterator& l, const SequenceIterator& r) {
Are all these operators really necessary to make lower_bound work? Somehow I
hoped it would be simpler but maybe I was being over-optimistic.

[hashimoto, 2011-07-07 07:05:32]

Fourth patch set, removed unnecessary operators.

http://codereview.chromium.org/7264015/diff/13003/views/ime/character_compose...
File views/ime/character_composer.cc (right):

http://codereview.chromium.org/7264015/diff/13003/views/ime/character_compose...
views/ime/character_composer.cc:87: inline bool operator>=(const
SequenceIterator& l, const SequenceIterator& r) {
On 2011/07/06 16:51:24, brettw wrote:
> Are all these operators really necessary to make lower_bound work? Somehow I
> hoped it would be simpler but maybe I was being over-optimistic.

I thought having all comparing operators is rational, but I agree with you that
having too many operators makes the code hard to read.

[brettw, 2011-07-07 15:36:46]

LGTM with these two minor comments. Please let me know when you check this in
and I'll mark the readability bug fixed.

http://codereview.chromium.org/7264015/diff/11006/views/ime/character_compose...
File views/ime/character_composer.cc (right):

http://codereview.chromium.org/7264015/diff/11006/views/ime/character_compose...
views/ime/character_composer.cc:17: // An iterator class to apply
std::lower_bound for composition table
Be sure to get a period at the end of this new comment.

http://codereview.chromium.org/7264015/diff/11006/views/ime/character_compose...
views/ime/character_composer.cc:18: class SequenceIterator :
I'd put the colon on this line indented 4 spaces (sort of like how initializer
lists would be formatted).

[hashimoto, 2011-07-08 03:22:44]

http://codereview.chromium.org/7264015/diff/11006/views/ime/character_compose...
File views/ime/character_composer.cc (right):

http://codereview.chromium.org/7264015/diff/11006/views/ime/character_compose...
views/ime/character_composer.cc:17: // An iterator class to apply
std::lower_bound for composition table
On 2011/07/07 15:36:46, brettw wrote:
> Be sure to get a period at the end of this new comment.

Fixed

http://codereview.chromium.org/7264015/diff/11006/views/ime/character_compose...
views/ime/character_composer.cc:18: class SequenceIterator :
On 2011/07/07 15:36:46, brettw wrote:
> I'd put the colon on this line indented 4 spaces (sort of like how initializer
> lists would be formatted).

Fixed

[hashimoto, 2011-07-08 04:35:08]

On 2011/07/07 15:36:46, brettw wrote:
> LGTM with these two minor comments. Please let me know when you check this in
> and I'll mark the readability bug fixed.

I committed this as
http://src.chromium.org/viewvc/chrome?view=rev&revision=91808
I am very honored to have been readability reviewed by you, one of the names in
src/base/OWNERS.
Thank you very much!