C++ readability review for lazyboy@

C++ readability review for lazyboy@

The changes are just code cleanups and no functional change.

This is the original CL that was submitted: https://chromiumcodereview.appspot.com/10868012.
This patch evolved initially from http://chromiumcodereview.appspot.com/10560022
I've included the files from browser_plugin/* dir.

Update:
Since the original CL became stale, I've uploaded a smaller subset from browser_plugin/ dir.
BUG=

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

[smo, 2013-11-12 21:25:02]

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:58:
BrowserPluginHostFactory* BrowserPluginGuest::factory_ = NULL;
If supported, use "nullptr" instead of NULL everywhere.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:67: return false;
Fix indent.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:112: if (should_allow &&
web_contents) {
(no change needed)

Or

  "web_contents != nullptr"

to be more explicit/avoid implicit cast-to-bool. This occurs a few times in this
file so please make the change everywhere if you decide to change it here.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:301: return
std::string();
or 

  return "";

to make it more obvious what's being returned.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:391: DCHECK_EQ(0ul,
pending_new_windows_.size());
(optional)

Or

  DCHECK(pending_new_windows_.empty());

if the type supports it.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:1127: break;
  return false;

then delete 1129...?

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:99: SiteInstance*
guest_site_instance,
In general, when passing pointers add comments about ownership and deletion. If
it's not owned by the callee, state how long the pointer needs to be valid
(until the end of the call? until the end of existence of some object?).

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:101:
scoped_ptr<base::DictionaryValue> extra_params);
Had to read up on this* since having scoped_ptr's as arguments is uncommon in
Google code. But it looks like that change to scoped_ptr was from 2011. Are you
now allowed to use std::unique_ptr?

*
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/RTd7rNxHjqk

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:103: static
BrowserPluginGuest* CreateWithOpener(
Same comment re: pointer ownership as before.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:105: WebContentsImpl*
web_contents,
If appropriate, have the pointer arguments come last:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

(Same comment applies throughout this file, although presumably this can't be
helped for virtual methods.)

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:126: static void
set_factory_for_testing(BrowserPluginHostFactory* factory) {
I would place all the _testing methods in their own section at the bottom of
public: so they don't clutter the normal client interface.

Alternatively, in situations where you can't test all the behavior of the class
through the client interface (which is of course the ideal), a common
alternative is using a Peer class which derives from the class being tested and
encapsulates all the messiness. Something like this:

class MyClass {
 public:
  ...
 private:
  int a_;
  friend class MyClassPeer;
};

and then in myclass_test.cc:

class MyClassPeer : public MyClass {
 public:
  void set_a(int a) { a_ = a; }
};

MyClassPeer behaves like a MyClass but gives controlled access to its private
state. Tests can't do anything unexpected to the internal state unless
MyClassPeer gives permission. "FRIEND_TEST" and other "friend" lines also become
unnecessary. And the client interface never needs to be changed again for the
sake of testing.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:165: virtual void
DidStopLoading(RenderViewHost* render_view_host) OVERRIDE;
Is the "override" keyword allowed now? Should that be used instead?

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:253: bool attached() const
{ return !!embedder_web_contents_; }
(optional change)

"!!" may not be commonly known and in fact is probably a no-op as far as the
generated machine code is concerned (since "return embedder_web_contents_;"
causes an implicit cast-to-bool anyway). Something like this makes it more
obvious what the intention of the code is:

  return embedder_web_contents_ != nullptr;

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:335:
scoped_refptr<BrowserPluginGuest::PermissionRequest> request,
Not familiar with scoped_refptr (is this like linked_ptr?), but if it's been
obsoleted by something like std::unique_ptr since this code was written, please
use that instead.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:512: struct NewWindowInfo
{
Follow standard declaration order unless there's a good reason not to:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Specifically, type definitions (typedef, struct, class, etc.) come before
everything else and data comes last.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:529: typedef std::map<int,
scoped_refptr<PermissionRequest> > RequestMap;
Not sure how this code is compiled, but ">>" is understood by recent compilers
and the "> >" hack is no longer needed. (Only change this if it compiles in your
context, of course. :)

[lazyboy, 2013-11-13 01:18:16]

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:58:
BrowserPluginHostFactory* BrowserPluginGuest::factory_ = NULL;
On 2013/11/12 21:25:02, smo wrote:
> If supported, use "nullptr" instead of NULL everywhere.

It isn't.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:67: return false;
On 2013/11/12 21:25:02, smo wrote:
> Fix indent.

Done.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:112: if (should_allow &&
web_contents) {
On 2013/11/12 21:25:02, smo wrote:
> (no change needed)
> 
> Or
> 
>   "web_contents != nullptr"
> 
> to be more explicit/avoid implicit cast-to-bool. This occurs a few times in
this
> file so please make the change everywhere if you decide to change it here.

As mentioned above, no nullptr.
I can change it to if (should_allow && web_contents != NULL) to be more explicit
in this case if you prefer.
However for a single conditional ptr to null check, chromium codebase
consistently uses if (ptr) {} style.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:301: return
std::string();
On 2013/11/12 21:25:02, smo wrote:
> or 
> 
>   return "";
> 
> to make it more obvious what's being returned.

Done.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:391: DCHECK_EQ(0ul,
pending_new_windows_.size());
On 2013/11/12 21:25:02, smo wrote:
> (optional)
> 
> Or
> 
>   DCHECK(pending_new_windows_.empty());
> 
> if the type supports it.

Done.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:1127: break;
On 2013/11/12 21:25:02, smo wrote:
>   return false;
> 
> then delete 1129...?

Done.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:99: SiteInstance*
guest_site_instance,
On 2013/11/12 21:25:02, smo wrote:
> In general, when passing pointers add comments about ownership and deletion.
If
> it's not owned by the callee, state how long the pointer needs to be valid
> (until the end of the call? until the end of existence of some object?).

(Here and below), since this is a static method, the ownership comment probably
is more helpful on the constructor? I've added comment before the constructor.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:101:
scoped_ptr<base::DictionaryValue> extra_params);
On 2013/11/12 21:25:02, smo wrote:
> Had to read up on this* since having scoped_ptr's as arguments is uncommon in
> Google code. But it looks like that change to scoped_ptr was from 2011. Are
you
> now allowed to use std::unique_ptr?
> 
> *
>
https://groups.google.com/a/chromium.org/forum/#%21topic/chromium-dev/RTd7rNx...
> 

Yes, no unique_ptr in chromium codebase.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:103: static
BrowserPluginGuest* CreateWithOpener(
On 2013/11/12 21:25:02, smo wrote:
> Same comment re: pointer ownership as before.

ditto.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:105: WebContentsImpl*
web_contents,
On 2013/11/12 21:25:02, smo wrote:
> If appropriate, have the pointer arguments come last:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> (Same comment applies throughout this file, although presumably this can't be
> helped for virtual methods.)

I've moved non virtual method's pointer/output args to end of param list for the
ones I could.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:126: static void
set_factory_for_testing(BrowserPluginHostFactory* factory) {
On 2013/11/12 21:25:02, smo wrote:
> I would place all the _testing methods in their own section at the bottom of
> public: so they don't clutter the normal client interface.
> 
> Alternatively, in situations where you can't test all the behavior of the
class
> through the client interface (which is of course the ideal), a common
> alternative is using a Peer class which derives from the class being tested
and
> encapsulates all the messiness. Something like this:
> 
> class MyClass {
>  public:
>   ...
>  private:
>   int a_;
>   friend class MyClassPeer;
> };
> 
> and then in myclass_test.cc:
> 
> class MyClassPeer : public MyClass {
>  public:
>   void set_a(int a) { a_ = a; }
> };
> 
> MyClassPeer behaves like a MyClass but gives controlled access to its private
> state. Tests can't do anything unexpected to the internal state unless
> MyClassPeer gives permission. "FRIEND_TEST" and other "friend" lines also
become
> unnecessary. And the client interface never needs to be changed again for the
> sake of testing.

Cool, Yes, TestBrowserPluginGuest was added for this purpose. I've moved the
set_guest_hang_timeout_for_testing() method below (and took out the _for_testing
suffix).
For the method here (set_factory_for_testing), this is static factory override,
I can't do the same trick as the other one.

Moved method to the very end of this section.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:165: virtual void
DidStopLoading(RenderViewHost* render_view_host) OVERRIDE;
On 2013/11/12 21:25:02, smo wrote:
> Is the "override" keyword allowed now? Should that be used instead?

Chromium's style is to use OVERRIDE instead of override keyword.
https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec...
The reason is I believe that non all compilers that are used to build chromium
support override.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:253: bool attached() const
{ return !!embedder_web_contents_; }
On 2013/11/12 21:25:02, smo wrote:
> (optional change)
> 
> "!!" may not be commonly known and in fact is probably a no-op as far as the
> generated machine code is concerned (since "return embedder_web_contents_;"
> causes an implicit cast-to-bool anyway). Something like this makes it more
> obvious what the intention of the code is:
> 
>   return embedder_web_contents_ != nullptr;

Since I can't use nullptr, changed it to "return embedder_web_contents != NULL"

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:335:
scoped_refptr<BrowserPluginGuest::PermissionRequest> request,
On 2013/11/12 21:25:02, smo wrote:
> Not familiar with scoped_refptr (is this like linked_ptr?), but if it's been
> obsoleted by something like std::unique_ptr since this code was written,
please
> use that instead.

There's no unique_ptr usage in chromium codebase, scoped_refptr is used
everywhere.
From http://src.chromium.org/chrome/trunk/src/base/memory/ref_counted.h
(scoped_refptr is) A smart pointer class for reference counted objects.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:512: struct NewWindowInfo
{
On 2013/11/12 21:25:02, smo wrote:
> Follow standard declaration order unless there's a good reason not to:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> 
> Specifically, type definitions (typedef, struct, class, etc.) come before
> everything else and data comes last.

Done for the struct.
typedef is supposed to be close to the point where it is used. This is chromium
specific convention, though I cannot find a reference at this point.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:529: typedef std::map<int,
scoped_refptr<PermissionRequest> > RequestMap;
On 2013/11/12 21:25:02, smo wrote:
> Not sure how this code is compiled, but ">>" is understood by recent compilers
> and the "> >" hack is no longer needed. (Only change this if it compiles in
your
> context, of course. :)

It compiles on my compilers (ubuntu and win), but I believe there are some
compiler combo for which it doesn't work.
Search for "typedef.*>>" in chromium returns only one result in chrome's tree
https://code.google.com/p/chromium/codesearch#search/&q=typedef.*%3E%3E%20fil...
And that file is windows specific.
In contrast, "typedef.*>\ >" has 232 results atm:
https://code.google.com/p/chromium/codesearch#search/&q=typedef.*%3E%5C%20%3E...

[smo, 2013-11-13 01:50:54]

LGTM

Thanks for the already clean code! Just a couple minor remaining comments.

You'll need to go through a normal review with an owner from your team and
you're good to go!

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:112: if (should_allow &&
web_contents) {
On 2013/11/13 01:18:16, lazyboy wrote:
> On 2013/11/12 21:25:02, smo wrote:
> > (no change needed)
> > 
> > Or
> > 
> >   "web_contents != nullptr"
> > 
> > to be more explicit/avoid implicit cast-to-bool. This occurs a few times in
> this
> > file so please make the change everywhere if you decide to change it here.
> 
> As mentioned above, no nullptr.
> I can change it to if (should_allow && web_contents != NULL) to be more
explicit
> in this case if you prefer.
> However for a single conditional ptr to null check, chromium codebase
> consistently uses if (ptr) {} style.

Consistency with existing code is fine.

https://chromiumcodereview.appspot.com/11312179/diff/69001/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/11312179/diff/69001/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:1309:
!!new_contents_impl->opener(),
(optional)

I'd either stick with the existing style of implicit casts (i.e., this without
the "!!") or do "!= NULL" explicitly.

[lazyboy, 2013-11-13 02:07:19]

Thanks for the review!

+fsamuel for browser_plugin/* OWNERS.
+creis for web_contents_impl.cc OWNERS.

https://chromiumcodereview.appspot.com/11312179/diff/69001/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/11312179/diff/69001/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:1309:
!!new_contents_impl->opener(),
On 2013/11/13 01:50:54, smo wrote:
> (optional)
> 
> I'd either stick with the existing style of implicit casts (i.e., this without
> the "!!") or do "!= NULL" explicitly.

Changed to != NULL

[Charlie Reis, 2013-11-13 19:04:27]

lazyboy: Can you update the CL description to indicate that it's just code
cleanup and no functional change, if that's true?

smo: One question about the parameter reordering below.

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11312179/diff/9001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:105: WebContentsImpl*
web_contents,
On 2013/11/13 01:18:16, lazyboy wrote:
> On 2013/11/12 21:25:02, smo wrote:
> > If appropriate, have the pointer arguments come last:
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> > 
> > (Same comment applies throughout this file, although presumably this can't
be
> > helped for virtual methods.)
> 
> I've moved non virtual method's pointer/output args to end of param list for
the
> ones I could.

smo: Can you elaborate on this?  The |web_contents| and |opener| parameters here
are inputs and not outputs, and I didn't see anything in the style rule about
putting pointer arguments at the end of the input parameters.

https://chromiumcodereview.appspot.com/11312179/diff/119002/content/browser/b...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11312179/diff/119002/content/browser/b...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:813: 
nit: I don't think we want to add the extra line here.

https://chromiumcodereview.appspot.com/11312179/diff/119002/content/browser/b...
File content/browser/browser_plugin/test_browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11312179/diff/119002/content/browser/b...
content/browser/browser_plugin/test_browser_plugin_guest.h:110: 
nit: Or here.

[lazyboy, 2013-11-13 19:21:10]

For input/output params comment: the reasoning I went for is: |web_contents| and
|opener| are not purely input args, they get mutated in the function and
non-const pointers. However they are not simple output params either. Just
noting. I'd be happy to know the answer here.

https://chromiumcodereview.appspot.com/11312179/diff/119002/content/browser/b...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11312179/diff/119002/content/browser/b...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:813: 
On 2013/11/13 19:04:28, creis wrote:
> nit: I don't think we want to add the extra line here.

Right, Done.

https://chromiumcodereview.appspot.com/11312179/diff/119002/content/browser/b...
File content/browser/browser_plugin/test_browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11312179/diff/119002/content/browser/b...
content/browser/browser_plugin/test_browser_plugin_guest.h:110: 
On 2013/11/13 19:04:28, creis wrote:
> nit: Or here.

Removed,
Done.

[Fady Samuel, 2013-11-13 19:24:18]

lgtm

[Charlie Reis, 2013-11-13 19:38:06]

On 2013/11/13 19:21:10, lazyboy wrote:
> For input/output params comment: the reasoning I went for is: |web_contents|
and
> |opener| are not purely input args, they get mutated in the function and
> non-const pointers. However they are not simple output params either. Just
> noting. I'd be happy to know the answer here.



On Wed, Nov 13, 2013 at 11:30 AM, Sean O'Malley <smo@google.com> wrote:
>
> On Wed, Nov 13, 2013 at 11:04 AM, <creis@chromium.org> wrote:
>>
>> smo: Can you elaborate on this?  The |web_contents| and |opener|
>> parameters here are inputs and not outputs, and I didn't see anything in
>> the style rule about putting pointer arguments at the end of the input
>> parameters.
>
>
> It's not a strict rule so whatever makes sense in this codebase is fine^.
According to the style, in the majority case, inputs are "const Foo&" and
outputs are "Foo*", with the consequence that methods are almost always of the
form Bar(const Foo1&, const Foo2&, Foo3*, Foo4*). I'm not familiar enough with
this codebase to choose the right form for every non-virtual method in this CL,
though, so it's up to your discretion. I just have to point it out since
readability CLs are super nitpicky. :-p
>
> ^ "This is not a hard-and-fast rule. Parameters that are both input and output
(often classes/structs) muddy the waters, and, as always, consistency with
related functions may require you to bend the rule."

I see-- thanks for clarifying.  I'd probably still call these input parameters
and wouldn't enforce that in other CLs (since Foo* inputs are fairly common in
Chrome code), but I'm happy with the teaching lesson on a readability CL. 
web_contents_impl.cc LGTM.

[commit-bot: I haz the power, 2013-11-14 03:26:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/11312179/379001

[commit-bot: I haz the power, 2013-11-14 04:43:33]

Retried try job too often on linux_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-14 05:53:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/11312179/379001

[commit-bot: I haz the power, 2013-11-14 06:40:48]

Change committed as 235067