Fix some issues found looking at the code.

Patch from Gaetano Mendola <mendola@gmail.com>
Original review: http://codereview.chromium.org/4273

I added some additions on my part and two unit test fix due to the added DCHECK. Reduced atl header inclusion.
Review URL: http://codereview.chromium.org/5009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@2730 0039d316-1c4b-4281-b951-d872f2087c98

15 files changed, 57 insertions, 61 deletions

diff --git a/AUTHORS b/AUTHORS
index fcd8c47..ed6d78c 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -14,3 +14,4 @@ Matthias Reitinger <reimarvin@gmail.com>
 Peter Bright <drpizza@quiscalusmexicanus.org>
 Arthur Lussos <developer0420@gmail.com>
 Masahiro Yado <yado.masa@gmail.com>
+Gaetano Mendola <mendola@gmail.com>
diff --git a/base/base_drag_source.cc b/base/base_drag_source.cc
index 4d3c510..bdaab1e 100644
--- a/base/base_drag_source.cc
+++ b/base/base_drag_source.cc
@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <atlbase.h>
-
 #include "base/base_drag_source.h"
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -50,15 +48,13 @@ HRESULT BaseDragSource::QueryInterface(const IID& iid, void** object) {
 }
 
 ULONG BaseDragSource::AddRef() {
-  return InterlockedIncrement(&ref_count_);
+  return ++ref_count_;
 }
 
 ULONG BaseDragSource::Release() {
-  if (InterlockedDecrement(&ref_count_) == 0) {
-    ULONG copied_refcnt = ref_count_;
+  if (--ref_count_ == 0) {
     delete this;
-    return copied_refcnt;
+    return 0U;
   }
   return ref_count_;
 }
-
diff --git a/base/base_drag_source.h b/base/base_drag_source.h
index d5f224b..1ca96dd 100644
--- a/base/base_drag_source.h
+++ b/base/base_drag_source.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef BASE_BASE_DRAG_SOURCE_H__
-#define BASE_BASE_DRAG_SOURCE_H__
+#ifndef BASE_BASE_DRAG_SOURCE_H_
+#define BASE_BASE_DRAG_SOURCE_H_
 
 #include <objidl.h>
 
@@ -43,5 +43,4 @@ class BaseDragSource : public IDropSource {
   DISALLOW_EVIL_CONSTRUCTORS(BaseDragSource);
 };
 
-#endif  // #ifndef BASE_DRAG_SOURCE_H__
-
+#endif  // #ifndef BASE_DRAG_SOURCE_H_
diff --git a/base/base_drop_target.cc b/base/base_drop_target.cc
index 7c1790e..9809255 100644
--- a/base/base_drop_target.cc
+++ b/base/base_drop_target.cc
@@ -2,11 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <windows.h>
-#include <shlobj.h>
-
 #include "base/base_drop_target.h"
 
+#include <shlobj.h>
+
 #include "base/logging.h"
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -14,11 +13,12 @@
 IDropTargetHelper* BaseDropTarget::cached_drop_target_helper_ = NULL;
 
 BaseDropTarget::BaseDropTarget(HWND hwnd)
-    : suspend_(false),
-      ref_count_(0),
-      hwnd_(hwnd) {
+    : hwnd_(hwnd),
+      suspend_(false),
+      ref_count_(0) {
   DCHECK(hwnd);
   HRESULT result = RegisterDragDrop(hwnd, this);
+  DCHECK(SUCCEEDED(result));
 }
 
 BaseDropTarget::~BaseDropTarget() {
@@ -125,14 +125,13 @@ HRESULT BaseDropTarget::QueryInterface(const IID& iid, void** object) {
 }
 
 ULONG BaseDropTarget::AddRef() {
-  return InterlockedIncrement(&ref_count_);
+  return ++ref_count_;
 }
 
 ULONG BaseDropTarget::Release() {
-  if (InterlockedDecrement(&ref_count_) == 0) {
-    ULONG copied_refcnt = ref_count_;
+  if (--ref_count_ == 0) {
     delete this;
-    return copied_refcnt;
+    return 0U;
   }
   return ref_count_;
 }
diff --git a/base/base_drop_target.h b/base/base_drop_target.h
index 1accbd1..9f20752 100644
--- a/base/base_drop_target.h
+++ b/base/base_drop_target.h
@@ -2,14 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef BASE_BASE_DROP_TARGET_H__
-#define BASE_BASE_DROP_TARGET_H__
+#ifndef BASE_BASE_DROP_TARGET_H_
+#define BASE_BASE_DROP_TARGET_H_
 
-#include <atlbase.h>
 #include <objidl.h>
-#include <shobjidl.h>
 
-#include "base/basictypes.h"
+#include "base/ref_counted.h"
+
+struct IDropTargetHelper;
 
 // A DropTarget implementation that takes care of the nitty gritty
 // of dnd. While this class is concrete, subclasses will most likely
@@ -18,6 +18,8 @@
 // Because BaseDropTarget is ref counted you shouldn't delete it directly,
 // rather wrap it in a scoped_refptr. Be sure and invoke RevokeDragDrop(m_hWnd)
 // before the HWND is deleted too.
+//
+// This class is meant to be used in a STA and is not multithread-safe.
 class BaseDropTarget : public IDropTarget {
  public:
   // Create a new BaseDropTarget associating it with the given HWND.
@@ -89,7 +91,7 @@ class BaseDropTarget : public IDropTarget {
   static IDropTargetHelper* DropHelper();
 
   // The data object currently being dragged over this drop target.
-  CComPtr<IDataObject> current_data_object_;
+  scoped_refptr<IDataObject> current_data_object_;
 
   // A helper object that is used to provide drag image support while the mouse
   // is dragging over the content area.
@@ -114,5 +116,4 @@ class BaseDropTarget : public IDropTarget {
   DISALLOW_EVIL_CONSTRUCTORS(BaseDropTarget);
 };
 
-#endif  // BASE_BASE_DROP_TARGET_H__
-
+#endif  // BASE_BASE_DROP_TARGET_H_
diff --git a/base/event_recorder.cc b/base/event_recorder.cc
index 23c092f..a7650a3 100644
--- a/base/event_recorder.cc
+++ b/base/event_recorder.cc
@@ -40,7 +40,7 @@ EventRecorder::~EventRecorder() {
   DCHECK(!is_recording_ && !is_playing_);
 }
 
-bool EventRecorder::StartRecording(std::wstring& filename) {
+bool EventRecorder::StartRecording(const std::wstring& filename) {
   if (journal_hook_ != NULL)
     return false;
   if (is_recording_ || is_playing_)
@@ -90,7 +90,7 @@ void EventRecorder::StopRecording() {
   }
 }
 
-bool EventRecorder::StartPlayback(std::wstring& filename) {
+bool EventRecorder::StartPlayback(const std::wstring& filename) {
   if (journal_hook_ != NULL)
     return false;
   if (is_recording_ || is_playing_)
@@ -160,7 +160,7 @@ void EventRecorder::StopPlayback() {
 // Windows callback hook for the recorder.
 LRESULT EventRecorder::RecordWndProc(int nCode, WPARAM wParam, LPARAM lParam) {
   static bool recording_enabled = true;
-  EVENTMSG *msg_ptr = NULL;
+  EVENTMSG* msg_ptr = NULL;
 
   // The API says we have to do this.
   // See http://msdn2.microsoft.com/en-us/library/ms644983(VS.85).aspx
@@ -175,13 +175,12 @@ LRESULT EventRecorder::RecordWndProc(int nCode, WPARAM wParam, LPARAM lParam) {
 
   // The Journal Recorder must stop recording events when system modal
   // dialogs are present. (see msdn link above)
-  switch(nCode)
-  {
+  switch(nCode) {
     case HC_SYSMODALON:
-	    recording_enabled = false;
+      recording_enabled = false;
       break;
     case HC_SYSMODALOFF:
-	    recording_enabled = true;
+      recording_enabled = true;
       break;
   }
 
@@ -197,41 +196,41 @@ LRESULT EventRecorder::RecordWndProc(int nCode, WPARAM wParam, LPARAM lParam) {
 }
 
 // Windows callback for the playback mode.
-LRESULT EventRecorder::PlaybackWndProc(int nCode, WPARAM wParam, LPARAM lParam)
-{
+LRESULT EventRecorder::PlaybackWndProc(int nCode, WPARAM wParam,
+                                       LPARAM lParam) {
   static bool playback_enabled = true;
   int delay = 0;
 
-  switch(nCode)
-  {
+  switch(nCode) {
     // A system modal dialog box is being displayed.  Stop playing back
     // messages.
     case HC_SYSMODALON:
-	    playback_enabled = false;
-	    break;
+      playback_enabled = false;
+      break;
 
     // A system modal dialog box is destroyed.  We can start playing back
     // messages again.
     case HC_SYSMODALOFF:
-	    playback_enabled = true;
-	    break;
+      playback_enabled = true;
+      break;
 
     // Prepare to copy the next mouse or keyboard event to playback.
     case HC_SKIP:
-	    if (!playback_enabled)
-	      break;
+      if (!playback_enabled)
+        break;
 
       // Read the next event from the record.
       if (fread(&playback_msg_, sizeof(EVENTMSG), 1, file_) != 1)
         this->StopPlayback();
-	    break;
+      break;
 
     // Copy the mouse or keyboard event to the EVENTMSG structure in lParam.
     case HC_GETNEXT:
-	    if (!playback_enabled)
+      if (!playback_enabled)
         break;
 
-      memcpy(reinterpret_cast<void*>(lParam), &playback_msg_, sizeof(playback_msg_));
+      memcpy(reinterpret_cast<void*>(lParam), &playback_msg_,
+             sizeof(playback_msg_));
 
       // The return value is the amount of time (in milliseconds) to wait
       // before playing back the next message in the playback queue.  Each
@@ -254,4 +253,3 @@ LRESULT EventRecorder::PlaybackWndProc(int nCode, WPARAM wParam, LPARAM lParam)
 }
 
 }  // namespace base
-
diff --git a/base/event_recorder.h b/base/event_recorder.h
index 500ad1a..a258d3a 100644
--- a/base/event_recorder.h
+++ b/base/event_recorder.h
@@ -37,7 +37,7 @@ class EventRecorder {
   // Starts recording events.
   // Will clobber the file if it already exists.
   // Returns true on success, or false if an error occurred.
-  bool StartRecording(std::wstring &filename);
+  bool StartRecording(const std::wstring& filename);
 
   // Stops recording.
   void StopRecording();
@@ -47,7 +47,7 @@ class EventRecorder {
 
   // Plays events previously recorded.
   // Returns true on success, or false if an error occurred.
-  bool StartPlayback(std::wstring &filename);
+  bool StartPlayback(const std::wstring& filename);
 
   // Stops playback.
   void StopPlayback();
@@ -68,7 +68,9 @@ class EventRecorder {
       : is_recording_(false),
         is_playing_(false),
         journal_hook_(NULL),
-        file_(NULL) {
+        file_(NULL),
+        playback_first_msg_time_(0),
+        playback_start_time_(0) {
   }
   ~EventRecorder();
 
@@ -88,4 +90,3 @@ class EventRecorder {
 }  // namespace base
 
 #endif // BASE_EVENT_RECORDER_H_
-
diff --git a/base/histogram.h b/base/histogram.h
index a2672e0..a7c1b92 100644
--- a/base/histogram.h
+++ b/base/histogram.h
@@ -193,7 +193,7 @@ class Histogram : public StatsRate {
             Sample maximum, size_t bucket_count);
   Histogram(const wchar_t* name, TimeDelta minimum,
             TimeDelta maximum, size_t bucket_count);
-  ~Histogram();
+  virtual ~Histogram();
 
   // Hooks to override stats counter methods.  This ensures that we gather all
   // input the stats counter sees.
diff --git a/base/hmac.h b/base/hmac.h
index ac48ec07..bbfe855 100644
--- a/base/hmac.h
+++ b/base/hmac.h
@@ -11,6 +11,7 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/scoped_ptr.h"
 
 namespace base {
 
@@ -34,7 +35,7 @@ class HMAC {
 
  private:
   HashAlgorithm hash_alg_;
-  HMACPlatformData* plat_;
+  scoped_ptr<HMACPlatformData> plat_;
 
   DISALLOW_COPY_AND_ASSIGN(HMAC);
 };
diff --git a/base/hmac_mac.cc b/base/hmac_mac.cc
index b2d0a33..668b306 100644
--- a/base/hmac_mac.cc
+++ b/base/hmac_mac.cc
@@ -24,8 +24,6 @@ HMAC::~HMAC() {
   plat_->key_.assign(plat_->key_.length(), std::string::value_type());
   plat_->key_.clear();
   plat_->key_.reserve(0);
-
-  delete plat_;
 }
 
 bool HMAC::Sign(const std::string& data,
diff --git a/base/hmac_nss.cc b/base/hmac_nss.cc
index 9ef1903..293f61d 100644
--- a/base/hmac_nss.cc
+++ b/base/hmac_nss.cc
@@ -66,7 +66,6 @@ HMAC::HMAC(HashAlgorithm hash_alg, const unsigned char* key, int key_length)
 }
 
 HMAC::~HMAC() {
-  delete plat_;
 }
 
 bool HMAC::Sign(const std::string& data,
diff --git a/base/hmac_win.cc b/base/hmac_win.cc
index d611993..d927ac1 100644
--- a/base/hmac_win.cc
+++ b/base/hmac_win.cc
@@ -67,8 +67,6 @@ HMAC::~HMAC() {
     CryptDestroyHash(plat_->hash_);
   if (plat_->provider_)
     CryptReleaseContext(plat_->provider_, 0);
-
-  delete plat_;
 }
 
 bool HMAC::Sign(const std::string& data,
diff --git a/chrome/views/focus_manager_unittest.cc b/chrome/views/focus_manager_unittest.cc
index 8f0040f..78d02b4 100644
--- a/chrome/views/focus_manager_unittest.cc
+++ b/chrome/views/focus_manager_unittest.cc
@@ -514,6 +514,7 @@ TestViewWindow* FocusManagerTest::GetWindow() {
 }
 
 void FocusManagerTest::SetUp() {
+  OleInitialize(NULL);
   test_window_ = new TestViewWindow(this);
   test_window_->Init();
   ShowWindow(test_window_->GetHWND(), SW_SHOW);
@@ -524,6 +525,7 @@ void FocusManagerTest::TearDown() {
 
   // Flush the message loop to make Purify happy.
   message_loop_.RunAllPending();
+  OleUninitialize();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/views/table_view_unittest.cc b/chrome/views/table_view_unittest.cc
index 2467f04..2e3ab7f 100644
--- a/chrome/views/table_view_unittest.cc
+++ b/chrome/views/table_view_unittest.cc
@@ -137,6 +137,7 @@ class TableViewTest : public testing::Test, ChromeViews::WindowDelegate {
 };
 
 void TableViewTest::SetUp() {
+  OleInitialize(NULL);
   model_.reset(CreateModel());
   std::vector<ChromeViews::TableColumn> columns;
   columns.resize(2);
@@ -154,6 +155,7 @@ void TableViewTest::TearDown() {
   window_->CloseNow();
   // Temporary workaround to avoid leak of RootView::pending_paint_task_.
   message_loop_.RunAllPending();
+  OleUninitialize();
 }
 
 void TableViewTest::VeriyViewOrder(int first, ...) {
diff --git a/webkit/tools/test_shell/test_webview_delegate.cc b/webkit/tools/test_shell/test_webview_delegate.cc
index 9339961..9711aa4 100644
--- a/webkit/tools/test_shell/test_webview_delegate.cc
+++ b/webkit/tools/test_shell/test_webview_delegate.cc
@@ -10,6 +10,7 @@
 
 #include <objidl.h>
 #include <shlobj.h>
+#include <shlwapi.h>
 
 #include "base/gfx/point.h"
 #include "base/message_loop.h"