C++ readability review for lazyboy@

content/browser/browser_plugin/browser_plugin_guest.cc

 // 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.
 
 #include "content/browser/browser_plugin/browser_plugin_guest.h"
 
 #include <algorithm>
 
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_util.h"
@@ skipping 37 matching lines @@
 #include "ui/surface/transport_dib.h"
 #include "webkit/common/resource_type.h"
 
 #if defined(OS_MACOSX)
 #include "content/browser/browser_plugin/browser_plugin_popup_menu_helper_mac.h"
 #endif
 
 namespace content {
 
 // static
 BrowserPluginHostFactory* BrowserPluginGuest::factory_ = NULL;

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

If supported, use "nullptr" instead of NULL everywhere.

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

It isn't.

 
 // Parent class for the various types of permission requests, each of which
 // should be able to handle the response to their permission request.
 class BrowserPluginGuest::PermissionRequest :
     public base::RefCounted<BrowserPluginGuest::PermissionRequest> {
  public:
   virtual void Respond(bool should_allow, const std::string& user_input) = 0;
   virtual bool AllowedByDefault() const {
       return false;

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

Fix indent.

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

Done.

   }
  protected:
   PermissionRequest() {
     RecordAction(UserMetricsAction("BrowserPlugin.Guest.PermissionRequest"));
   }
   virtual ~PermissionRequest() {}
   // Friend RefCounted so that the dtor can be non-public.
   friend class base::RefCounted<BrowserPluginGuest::PermissionRequest>;
 };
 
@@ skipping 24 matching lines @@
                        weak_ptr_factory_(weak_ptr_factory) {
     RecordAction(
         UserMetricsAction("BrowserPlugin.Guest.PermissionRequest.Geolocation"));
   }
 
   virtual void Respond(bool should_allow,
                        const std::string& user_input) OVERRIDE {
     base::WeakPtr<BrowserPluginGuest> guest(weak_ptr_factory_->GetWeakPtr());
 
     WebContents* web_contents = guest->embedder_web_contents();
     if (should_allow && web_contents) {

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

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

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

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.

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

Consistency with existing code is fine.

       // If renderer side embedder decides to allow gelocation, we need to check
       // if the app/embedder itself has geolocation access.
       BrowserContext* browser_context = web_contents->GetBrowserContext();
       if (browser_context) {
         GeolocationPermissionContext* geolocation_context =
             browser_context->GetGeolocationPermissionContext();
         if (geolocation_context) {
           base::Callback<void(bool)> geolocation_callback = base::Bind(
               &BrowserPluginGuest::SetGeolocationPermission,
               guest,
@@ skipping 168 matching lines @@
     RenderViewHost* render_view_host,
     int url_request_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   int render_process_id = render_view_host->GetProcess()->GetID();
   GlobalRequestID global_id(render_process_id, url_request_id);
   net::URLRequest* url_request =
       ResourceDispatcherHostImpl::Get()->GetURLRequest(global_id);
   if (url_request)
     return url_request->url().possibly_invalid_spec();
   return std::string();

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

or 

  return "";

to make it more obvious what's being returned.

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

Done.

 }
 
 }  // namespace
 
 class BrowserPluginGuest::EmbedderWebContentsObserver
     : public WebContentsObserver {
  public:
   explicit EmbedderWebContentsObserver(BrowserPluginGuest* guest)
       : WebContentsObserver(guest->embedder_web_contents()),
         browser_plugin_guest_(guest) {
@@ skipping 69 matching lines @@
   // pending_new_windows_ set. To avoid mutating the set while iterating, we
   // create a copy of the pending new windows set and iterate over the copy.
   PendingWindowMap pending_new_windows(pending_new_windows_);
   // Clean up unattached new windows opened by this guest.
   for (PendingWindowMap::const_iterator it = pending_new_windows.begin();
        it != pending_new_windows.end(); ++it) {
     it->first->Destroy();
   }
   // All pending windows should be removed from the set after Destroy() is
   // called on all of them.
   DCHECK_EQ(0ul, pending_new_windows_.size());

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

(optional)

Or

  DCHECK(pending_new_windows_.empty());

if the type supports it.

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

Done.

 }
 
 void BrowserPluginGuest::LoadURLWithParams(WebContents* web_contents,
                                            const GURL& url,
                                            const Referrer& referrer,
                                            PageTransition transition_type) {
   NavigationController::LoadURLParams load_url_params(url);
   load_url_params.referrer = referrer;
   load_url_params.transition_type = transition_type;
   load_url_params.extra_headers = std::string();
@@ skipping 715 matching lines @@
     case BrowserPluginHostMsg_SetAutoSize::ID:
     case BrowserPluginHostMsg_SetEditCommandsForNextKeyEvent::ID:
     case BrowserPluginHostMsg_SetFocus::ID:
     case BrowserPluginHostMsg_SetName::ID:
     case BrowserPluginHostMsg_SetVisibility::ID:
     case BrowserPluginHostMsg_UnlockMouse_ACK::ID:
     case BrowserPluginHostMsg_UpdateGeometry::ID:
     case BrowserPluginHostMsg_UpdateRect_ACK::ID:
       return true;
     default:
       break;

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

  return false;

then delete 1129...?

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

Done.

   }
   return false;
 }
 
 bool BrowserPluginGuest::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(BrowserPluginGuest, message)
     IPC_MESSAGE_HANDLER(ViewHostMsg_HasTouchEventHandlers,
                         OnHasTouchEventHandlers)
     IPC_MESSAGE_HANDLER(ViewHostMsg_LockMouse, OnLockMouse)
@@ skipping 592 matching lines @@
   base::DictionaryValue request_info;
   request_info.Set(browser_plugin::kRequestMethod,
                    base::Value::CreateStringValue(request_method));
   request_info.Set(browser_plugin::kURL, base::Value::CreateStringValue(url));
 
   RequestPermission(BROWSER_PLUGIN_PERMISSION_TYPE_DOWNLOAD,
                     new DownloadRequest(callback),
                     request_info);
 }
 
+
 }  // namespace content