C++ readability review for lazyboy@

content/browser/browser_plugin/browser_plugin_guest.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // A BrowserPluginGuest is the browser side of a browser <--> embedder
 // renderer channel. A BrowserPlugin (a WebPlugin) is on the embedder
 // renderer side of browser <--> embedder renderer communication.
 //
 // BrowserPluginGuest lives on the UI thread of the browser process. Any
 // messages about the guest render process that the embedder might be interested
@@ skipping 78 matching lines @@
 
   // The WebContents passed into the factory method here has not been
   // initialized yet and so it does not yet hold a SiteInstance.
   // BrowserPluginGuest must be constructed and installed into a WebContents
   // prior to its initialization because WebContents needs to determine what
   // type of WebContentsView to construct on initialization. The content
   // embedder needs to be aware of |guest_site_instance| on the guest's
   // construction and so we pass it in here.
   static BrowserPluginGuest* Create(
       int instance_id,
       SiteInstance* guest_site_instance,

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

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?).

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

(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.

       WebContentsImpl* web_contents,
       scoped_ptr<base::DictionaryValue> extra_params);

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

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

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

Yes, no unique_ptr in chromium codebase.

 
   static BrowserPluginGuest* CreateWithOpener(

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

Same comment re: pointer ownership as before.

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

ditto.

       int instance_id,
       WebContentsImpl* web_contents,

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

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.)

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

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

[Charlie Reis, 2013/11/13 19:04:28]

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.

       BrowserPluginGuest* opener,
       bool has_render_view);
 
   // Called when the embedder WebContents is destroyed to give the
   // BrowserPluginGuest an opportunity to clean up after itself.
   void EmbedderDestroyed();
 
   // Called when the embedder WebContents changes visibility.
   void EmbedderVisibilityChanged(bool visible);
 
   // Destroys the guest WebContents and all its associated state, including
   // this BrowserPluginGuest, and its new unattached windows.
   void Destroy();
 
   // Returns the identifier that uniquely identifies a browser plugin guest
   // within an embedder.
   int instance_id() const { return instance_id_; }
 
   // Overrides factory for testing. Default (NULL) value indicates regular
   // (non-test) environment.
   static void set_factory_for_testing(BrowserPluginHostFactory* factory) {

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

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.

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

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.

     BrowserPluginGuest::factory_ = factory;
   }
 
   bool OnMessageReceivedFromEmbedder(const IPC::Message& message);
 
   void Initialize(WebContentsImpl* embedder_web_contents,
                   const BrowserPluginHostMsg_Attach_Params& params);
 
   void set_guest_hang_timeout_for_testing(const base::TimeDelta& timeout) {
     guest_hang_timeout_ = timeout;
@@ skipping 18 matching lines @@
   void UpdateVisibility();
 
   // WebContentsObserver implementation.
   virtual void DidCommitProvisionalLoadForFrame(
       int64 frame_id,
       const string16& frame_unique_name,
       bool is_main_frame,
       const GURL& url,
       PageTransition transition_type,
       RenderViewHost* render_view_host) OVERRIDE;
   virtual void DidStopLoading(RenderViewHost* render_view_host) OVERRIDE;

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

Is the "override" keyword allowed now? Should that be used instead?

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

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.

 
   virtual void RenderViewReady() OVERRIDE;
   virtual void RenderProcessGone(base::TerminationStatus status) OVERRIDE;
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 
   // WebContentsDelegate implementation.
   virtual bool AddMessageToConsole(WebContents* source,
                                    int32 level,
                                    const string16& message,
                                    int32 line_no,
@@ skipping 67 matching lines @@
 
   gfx::Point GetScreenCoordinates(const gfx::Point& relative_position) const;
 
   // Helper to send messages to embedder. This methods fills the message with
   // the correct routing id.
   // Overridden in test implementation since we want to intercept certain
   // messages for testing.
   virtual void SendMessageToEmbedder(IPC::Message* msg);
 
   // Returns whether the guest is attached to an embedder.
   bool attached() const { return !!embedder_web_contents_; }

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

(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;

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

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

 
   // Attaches this BrowserPluginGuest to the provided |embedder_web_contents|
   // and initializes the guest with the provided |params|. Attaching a guest
   // to an embedder implies that this guest's lifetime is no longer managed
   // by its opener, and it can begin loading resources. |extra_params| are
   // parameters passed into BrowserPlugin from JavaScript to be forwarded to
   // the content embedder.
   void Attach(WebContentsImpl* embedder_web_contents,
               BrowserPluginHostMsg_Attach_Params params,
               const base::DictionaryValue& extra_params);
@@ skipping 61 matching lines @@
                          PageTransition transition_type);
 
   // Bridge IDs correspond to a geolocation request. This method will remove
   // the bookkeeping for a particular geolocation request associated with the
   // provided |bridge_id|. It returns the request ID of the geolocation request.
   int RemoveBridgeID(int bridge_id);
 
   // Returns the |request_id| generated for the |request| provided.
   int RequestPermission(
       BrowserPluginPermissionType permission_type,
       scoped_refptr<BrowserPluginGuest::PermissionRequest> request,

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

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.

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

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.

       const base::DictionaryValue& request_info);
 
   // Creates a new guest window, and BrowserPluginGuest that is owned by this
   // BrowserPluginGuest.
   BrowserPluginGuest* CreateNewGuestWindow(const OpenURLParams& params);
 
   base::SharedMemory* damage_buffer() const { return damage_buffer_.get(); }
   const gfx::Size& damage_view_size() const { return damage_view_size_; }
   float damage_buffer_scale_factor() const {
     return damage_buffer_scale_factor_;
@@ skipping 156 matching lines @@
   bool embedder_visible_;
   std::string name_;
   bool auto_size_enabled_;
   gfx::Size max_auto_size_;
   gfx::Size min_auto_size_;
 
   // Tracks the name, and target URL of the new window and whether or not it has
   // changed since the WebContents has been created and before the new window
   // has been attached to a BrowserPlugin. Once the first navigation commits, we
   // no longer track this information.
   struct NewWindowInfo {

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

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.

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

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.

     bool changed;
     GURL url;
     std::string name;
     NewWindowInfo(const GURL& url, const std::string& name) :
         changed(false),
         url(url),
         name(name) {}
   };
   typedef std::map<BrowserPluginGuest*, NewWindowInfo> PendingWindowMap;
   PendingWindowMap pending_new_windows_;
   base::WeakPtr<BrowserPluginGuest> opener_;
   // A counter to generate a unique request id for a permission request.
   // We only need the ids to be unique for a given BrowserPluginGuest.
   int next_permission_request_id_;
 
   // A map to store relevant info for a request keyed by the request's id.
   typedef std::map<int, scoped_refptr<PermissionRequest> > RequestMap;

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

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]

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...

   RequestMap permission_request_map_;
 
   // Indicates that this BrowserPluginGuest has associated renderer-side state.
   // This is used to determine whether or not to create a new RenderView when
   // this guest is attached.
   bool has_render_view_;
 
   // Last seen size of guest contents (by OnUpdateRect).
   gfx::Size last_seen_view_size_;
   // Last seen autosize attribute state (by OnUpdateRect).
   bool last_seen_auto_size_enabled_;
 
   bool is_in_destruction_;
 
   // This is a queue of messages that are destined to be sent to the embedder
   // once the guest is attached to a particular embedder.
   std::queue<IPC::Message*> pending_messages_;
 
   scoped_ptr<BrowserPluginGuestDelegate> delegate_;
 
   // These are parameters passed from JavaScript on attachment to the content
   // embedder.
   scoped_ptr<base::DictionaryValue> extra_attach_params_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowserPluginGuest);
 };
 
 }  // namespace content
 
+
 #endif  // CONTENT_BROWSER_BROWSER_PLUGIN_BROWSER_PLUGIN_GUEST_H_