C++ readability change for jochen

chrome/browser/extensions/extension_webnavigation_api.h

 // Copyright (c) 2010 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.
 
 // Defines the Chrome Extensions WebNavigation API functions for observing and
 // intercepting navigation events, as specified in
 // chrome/common/extensions/api/extension_api.json.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_WEBNAVIGATION_API_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_WEBNAVIGATION_API_H_
 #pragma once
 
 #include <map>
 
 #include "base/singleton.h"
 #include "chrome/browser/extensions/extension_function.h"
 #include "chrome/common/notification_observer.h"
 #include "chrome/common/notification_registrar.h"
 #include "googleurl/src/gurl.h"
 
 class NavigationController;
 class ProvisionalLoadDetails;
 class TabContents;
 
 // Tracks which frames are in an error state, and no navigation events should
 // be sent for.

[brettw, 2010/11/23 22:44:09]

This comment and the name of the class don't really mesh in my mine. Does this
state object only exist for error frames (in which case it seems like it should
have some kind of "Error" in the name), or is the tracking everything such that
we can know which ones in particular have errors (in which case the comment
should probably be expanded).

[jochen (gone - plz use gerrit), 2010/11/24 07:37:37]

It's the latter.

Done.

 class FrameNavigationState {
  public:
   FrameNavigationState();
   ~FrameNavigationState();
 
   // True if navigation events for the given frame can be sent.
   bool CanSendEvents(long long frame_id) const;

[brettw, 2010/11/23 22:44:09]

What is the goal here of using "long long"? I've not seen this used anywhere
else. If you want a 64-bit value, use int64. Same elsewhere.

[jochen (gone - plz use gerrit), 2010/11/24 07:37:37]

In WebKit, you'd use long long instead. Since that's a value that's being passed
down from WebKit, I've used long long. Seems like in the majority of cases,
we're using int64 instead, so I changed that

Done.

 
   // Starts to track a frame given by its |frame_id| showing the URL |url| in
   // a |tab_contents|.
   void TrackFrame(long long frame_id,
                   const GURL& url,
                   bool is_main_frame,
                   const TabContents* tab_contents);
 
   // Returns the URL corresponding to a tracked frame given by its |frame_id|.
   GURL GetUrl(long long frame_id) const;
@@ skipping 18 matching lines @@
 
   // Tracks which frames belong to a given tab contents object.
   TabContentsToFrameIdMap tab_contents_map_;
 
   // Tracks the state of known frames.
   FrameIdToStateMap frame_state_map_;
 
   DISALLOW_COPY_AND_ASSIGN(FrameNavigationState);
 };
 
+
 // Observes navigation notifications and routes them as events to the extension
 // system.
 class ExtensionWebNavigationEventRouter : public NotificationObserver {
  public:
   // Single instance of the event router.

[brettw, 2010/11/23 22:44:09]

Function comments should be descriptive, so this should be "Returns the single
instance..." I might say "singleton" instead since that then implies the memory
management of the pointer, where "single instance" might be ambiguous.

[jochen (gone - plz use gerrit), 2010/11/24 07:37:37]

Done.

   static ExtensionWebNavigationEventRouter* GetInstance();
 
   void Init();

[brettw, 2010/11/23 22:44:09]

Who must call this? Normally if I see an init function, I expect to be required
to call it after the constructor, but callers are not in charge of constructing
this object, so this is a bit confusing.

[jochen (gone - plz use gerrit), 2010/11/24 07:37:37]

Done.

 
  private:
   friend struct DefaultSingletonTraits<ExtensionWebNavigationEventRouter>;
 
   ExtensionWebNavigationEventRouter() {}
   virtual ~ExtensionWebNavigationEventRouter() {}
 
   // NotificationObserver implementation.
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
@@ skipping 19 matching lines @@
   // Handler for the FRAME_DID_FINISH_LOAD event. The method takes the frame
   // ID and constructs a suitable JSON formatted extension event from it.
   void FrameDidFinishLoad(NavigationController* controller, long long frame_id);
 
   // Handler for the FAIL_PROVISIONAL_LOAD_WITH_ERROR event. The method takes
   // the details of such an event and constructs a suitable JSON formatted
   // extension event from it.
   void FailProvisionalLoadWithError(NavigationController* controller,
                                     ProvisionalLoadDetails* details);
 
   // This method dispatches events to the extension message service.

[brettw, 2010/11/23 22:44:09]

"This method dispatches..." -> "Dispatches..."

[jochen (gone - plz use gerrit), 2010/11/24 07:37:37]

Done.

   void DispatchEvent(Profile* context,
                      const char* event_name,
                      const std::string& json_args);
 
   // Tracks the state of the frames we are sending events for.
   FrameNavigationState navigation_state_;
 
   // Used for tracking registrations to navigation notifications.
   NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionWebNavigationEventRouter);
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_WEBNAVIGATION_API_H_