C++ readability change for jochen

chrome/browser/extensions/extension_webnavigation_api.cc

 // 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.
 
 // Implements the Chrome Extensions WebNavigation API.
 
 #include "chrome/browser/extensions/extension_webnavigation_api.h"
 
 #include "base/json/json_writer.h"
 #include "base/time.h"
@@ skipping 19 matching lines @@
   return details->main_frame() ? 0 : static_cast<int>(details->frame_id());
 }
 
 // Returns |time| as milliseconds since the epoch.
 double MilliSecondsFromTime(const base::Time& time) {
   return 1000 * time.ToDoubleT();
 }
 
 }  // namespace
 
+
 FrameNavigationState::FrameNavigationState() {
 }
 
 FrameNavigationState::~FrameNavigationState() {
 }
 
 bool FrameNavigationState::CanSendEvents(long long frame_id) const {
   FrameIdToStateMap::const_iterator frame_state =
       frame_state_map_.find(frame_id);
   return frame_state != frame_state_map_.end() &&
       !frame_state->second.error_occurred;
 }
 
 void FrameNavigationState::TrackFrame(long long frame_id,
                                       const GURL& url,
                                       bool is_main_frame,
                                       const TabContents* tab_contents) {
   if (is_main_frame)
     RemoveTabContentsState(tab_contents);
   tab_contents_map_.insert(
       TabContentsToFrameIdMap::value_type(tab_contents, frame_id));

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

Optional: I might have used std::make_pair(tab_contents, frame_id) here which
would be shorter, although I can see why some people might like your way better.

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

Done.

   FrameState& frame_state = frame_state_map_[frame_id];
   frame_state.error_occurred = (url.spec() == chrome::kUnreachableWebDataURL);
   frame_state.url = url;
   frame_state.is_main_frame = is_main_frame;
 }
 
 GURL FrameNavigationState::GetUrl(long long frame_id) const {
   FrameIdToStateMap::const_iterator frame_state =
       frame_state_map_.find(frame_id);
   if (frame_state == frame_state_map_.end()) {
@@ skipping 15 matching lines @@
 
 void FrameNavigationState::ErrorOccurredInFrame(long long frame_id) {
   DCHECK(frame_state_map_.find(frame_id) != frame_state_map_.end());
   frame_state_map_[frame_id].error_occurred = true;
 }
 
 void FrameNavigationState::RemoveTabContentsState(
     const TabContents* tab_contents) {
   typedef TabContentsToFrameIdMap::iterator FrameIdIterator;
   std::pair<FrameIdIterator, FrameIdIterator> frame_ids =
           tab_contents_map_.equal_range(tab_contents);

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

This only needs to be indented 4 spaces.

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

Done.

   for (FrameIdIterator frame_id = frame_ids.first; frame_id != frame_ids.second;
        ++frame_id) {
     frame_state_map_.erase(frame_id->second);
   }
   tab_contents_map_.erase(tab_contents);
 }
 
 
 // static
 ExtensionWebNavigationEventRouter*
@@ skipping 48 matching lines @@
       FrameDidFinishLoad(
           Source<NavigationController>(source).ptr(),
           *Details<long long>(details).ptr());
       break;
     case NotificationType::FAIL_PROVISIONAL_LOAD_WITH_ERROR:
       FailProvisionalLoadWithError(
           Source<NavigationController>(source).ptr(),
           Details<ProvisionalLoadDetails>(details).ptr());
       break;
 
     case NotificationType::TAB_CONTENTS_DESTROYED:

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

Can you be consistent about whether there's a blank line before each case or
not? I'd say it's not necessary so would just remove the line above this.

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

Done.

       navigation_state_.RemoveTabContentsState(
           Source<TabContents>(source).ptr());
       break;
 
     default:
       NOTREACHED();
   }
 }
 void ExtensionWebNavigationEventRouter::FrameProvisionalLoadStart(
     NavigationController* controller,
     ProvisionalLoadDetails* details) {
   navigation_state_.TrackFrame(details->frame_id(),
                                details->url(),
                                details->main_frame(),
                                controller->tab_contents());
   if (!navigation_state_.CanSendEvents(details->frame_id()))
     return;
   ListValue args;
   DictionaryValue* dict = new DictionaryValue();
   dict->SetInteger(keys::kTabIdKey,
                    ExtensionTabUtil::GetTabId(controller->tab_contents()));
   dict->SetString(keys::kUrlKey,

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

This can fit on one line (same below when you set the url key).

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

Done.

                   details->url().spec());
   dict->SetInteger(keys::kFrameIdKey, GetFrameId(details));
   dict->SetInteger(keys::kRequestIdKey, 0);
   dict->SetReal(keys::kTimeStampKey, MilliSecondsFromTime(base::Time::Now()));
   args.Append(dict);
 
   std::string json_args;
   base::JSONWriter::Write(&args, false, &json_args);
   DispatchEvent(controller->profile(), keys::kOnBeforeNavigate, json_args);
 }
@@ skipping 26 matching lines @@
 
 void ExtensionWebNavigationEventRouter::FrameDomContentLoaded(
     NavigationController* controller, long long frame_id) {
   if (!navigation_state_.CanSendEvents(frame_id))
     return;
   ListValue args;
   DictionaryValue* dict = new DictionaryValue();
   dict->SetInteger(keys::kTabIdKey,
                    ExtensionTabUtil::GetTabId(controller->tab_contents()));
   dict->SetString(keys::kUrlKey, navigation_state_.GetUrl(frame_id).spec());
   dict->SetInteger(keys::kFrameIdKey, navigation_state_.IsMainFrame(frame_id) ?

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

I always try to format the ternary operators as clearly as possible since they
can be confusing to follow. In this case, I'd probably try to fit it on one line
so move the IsMainFrame call to the next line so you can keep everything
together better.

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

Done.

       0 : static_cast<int>(frame_id));
   dict->SetReal(keys::kTimeStampKey, MilliSecondsFromTime(base::Time::Now()));
   args.Append(dict);
 
   std::string json_args;
   base::JSONWriter::Write(&args, false, &json_args);
   DispatchEvent(controller->profile(), keys::kOnDOMContentLoaded, json_args);
 }
 
 void ExtensionWebNavigationEventRouter::FrameDidFinishLoad(
     NavigationController* controller, long long frame_id) {

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

the frame_id should probably be on the next line like you do for the other
functions

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

Done.

   if (!navigation_state_.CanSendEvents(frame_id))
     return;
   ListValue args;
   DictionaryValue* dict = new DictionaryValue();
   dict->SetInteger(keys::kTabIdKey,
                    ExtensionTabUtil::GetTabId(controller->tab_contents()));
   dict->SetString(keys::kUrlKey, navigation_state_.GetUrl(frame_id).spec());
   dict->SetInteger(keys::kFrameIdKey, navigation_state_.IsMainFrame(frame_id) ?

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

Sane ternary formatting suggestion.

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

Done.

       0 : static_cast<int>(frame_id));
   dict->SetReal(keys::kTimeStampKey, MilliSecondsFromTime(base::Time::Now()));
   args.Append(dict);
 
   std::string json_args;
   base::JSONWriter::Write(&args, false, &json_args);
   DispatchEvent(controller->profile(), keys::kOnCompleted, json_args);
 }
 
 void ExtensionWebNavigationEventRouter::FailProvisionalLoadWithError(
@@ skipping 21 matching lines @@
 
 void ExtensionWebNavigationEventRouter::DispatchEvent(
     Profile* profile,
     const char* event_name,
     const std::string& json_args) {
   if (profile && profile->GetExtensionEventRouter()) {
     profile->GetExtensionEventRouter()->DispatchEventToRenderers(
         event_name, json_args, profile, GURL());
   }
 }