For C++ readability review.

For C++ readability review.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252258

[nadav, 2014-02-06 08:23:49]

Hi Weitao,

Thank you for submitting this CL for a readability review. I made a first pass
through the code, and, overall, it's very clean and readable. One of the
questions I have to you (or others on your team) is the state of C++11 in
Chromium code. I feel like your CL has some C++11 use, but not the kind I'd
expect. Please point me at the relevant policy, as I'm not familiar with those.
Also, note that some of the comments I made in-line (and/or the documentation I
linked to) applies to more than one place in the CL. While I make each unique
comment once, I do expect you to fix the underlying issue wherever it's
relevant. As part of the readability process we'd like to get the candidate to
practice noticing some readability issues themselves, so leaving this work to
you serves a useful purpose.

Do let me know if you have any questions
Nadav

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.cc:58: bool xmpp_server_valid =
const?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of...

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.h:24: typedef
NativeMessagingChannel::SendMessageCallback SendMessageCallback;
Why do you need this typedef as part of the class's public interface? It seems
to not be needed by users of this class.

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.h:28:
scoped_ptr<NativeMessagingChannel> channel,
scoped_ptr is sorta deprecated with the advent of C++11's unique_ptr. Why not
use std::unique_ptr? Is Chromium still not allowing such features?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Owners...

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:24: namespace {
You probably want the unnamed namespace *inside* the named namespace "remoting"
that wraps all the rest of the code here. This makes symbol resolution work the
same across the whole code, which makes it far more readable.

[weitao, 2014-02-19 00:40:03]

PTAL

Adding Sergey to see if he has any insight on the (lack of) usage of C++11
features.

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.cc:58: bool xmpp_server_valid =
On 2014/02/06 08:23:49, nadav wrote:
> const?
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of...

Done.

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.h:24: typedef
NativeMessagingChannel::SendMessageCallback SendMessageCallback;
No, removed.

On 2014/02/06 08:23:49, nadav wrote:
> Why do you need this typedef as part of the class's public interface? It seems
> to not be needed by users of this class.

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.h:28:
scoped_ptr<NativeMessagingChannel> channel,
Many of the C++11 features (unique_ptr and shared_ptr in particular) are not
used at all in the chromium project and I do not know the reason. Could it just
be because of the different level of support of the C++11 features on the
different platforms (Windows, Mac, Win, and Android)? 

I am adding Sergey to the reviewer list and see if he can provide some insight.

On 2014/02/06 08:23:49, nadav wrote:
> scoped_ptr is sorta deprecated with the advent of C++11's unique_ptr. Why not
> use std::unique_ptr? Is Chromium still not allowing such features?
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Owners...

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:24: namespace {
On 2014/02/06 08:23:49, nadav wrote:
> You probably want the unnamed namespace *inside* the named namespace
"remoting"
> that wraps all the rest of the code here. This makes symbol resolution work
the
> same across the whole code, which makes it far more readable.

Done.

[Sergey Ulanov, 2014-02-19 01:52:52]

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/138833009/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.h:28:
scoped_ptr<NativeMessagingChannel> channel,
On 2014/02/06 08:23:49, nadav wrote:
> scoped_ptr is sorta deprecated with the advent of C++11's unique_ptr. Why not
> use std::unique_ptr? Is Chromium still not allowing such features?
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Owners...

We don't use C++11 in chromium yet because not all platforms we need to compile
for support it. Relevant thread in chromium-dev with more details:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/oVTPsijC7d0

[nadav, 2014-02-19 16:21:52]

Thanks for the changes. Getting close :)

https://codereview.chromium.org/138833009/diff/80001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/138833009/diff/80001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_native_messaging_host.cc:57: const ServiceUrls* const
service_urls = ServiceUrls::GetInstance();
That's probably too many consts.
Unless you really need all of them to compile, or the code around you does it
too, in nested pointer situation we typically only put the first "const":
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[weitao, 2014-02-19 17:52:18]

PTAL

https://codereview.chromium.org/138833009/diff/80001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/138833009/diff/80001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_native_messaging_host.cc:57: const ServiceUrls* const
service_urls = ServiceUrls::GetInstance();
On 2014/02/19 16:21:53, nadav wrote:
> That's probably too many consts.
> Unless you really need all of them to compile, or the code around you does it
> too, in nested pointer situation we typically only put the first "const":
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Done.

[nadav, 2014-02-20 00:48:47]

Thanks for making all these changes. Just remove the empty lines, get a LGTM
from someone on your team, and you're done! Congratulations :)

https://codereview.chromium.org/138833009/diff/160001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/138833009/diff/160001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:85: 
You should now be able to remove the trailing empty line

[nadav, 2014-02-20 00:49:14]

lgtm

[nadav, 2014-02-20 00:49:16]

lgtm

[Sergey Ulanov, 2014-02-20 00:52:47]

lgtm

[weitao, 2014-02-20 00:54:31]

The CQ bit was checked by weitaosu@chromium.org

[commit-bot: I haz the power, 2014-02-20 01:43:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/138833009/160001

[commit-bot: I haz the power, 2014-02-20 05:13:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/138833009/160001

[commit-bot: I haz the power, 2014-02-20 09:09:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/138833009/160001

[commit-bot: I haz the power, 2014-02-20 12:30:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/138833009/160001

[commit-bot: I haz the power, 2014-02-20 16:12:58]

Change committed as 252258