Native Client Visual Studio Add-in

visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs

Index: visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs
diff --git a/visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs b/visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs
new file mode 100644
index 0000000000000000000000000000000000000000..ae4c615d12c9ef428cb27cb40c7159dec3906f95
--- /dev/null
+++ b/visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs
@@ -0,0 +1,129 @@
+// 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.
+
+using System;

[elijahtaylor1, 2012/07/11 20:56:18]

ABC order

[noelallen1, 2012/07/11 22:16:46]

Should using be in alphabetical like includes?

[tysand, 2012/07/12 23:56:15]

Done.

[tysand, 2012/07/12 23:56:15]

Done.

+using Extensibility;
+using EnvDTE;
+using EnvDTE80;

[elijahtaylor1, 2012/07/11 20:56:18]

add newline

[tysand, 2012/07/12 23:56:15]

Done.

+namespace NativeClientVSAddIn

[noelallen1, 2012/07/11 22:16:46]

LF?

[tysand, 2012/07/12 23:56:15]

Done.

+{
+  /// <summary>The object for implementing an Add-in</summary>
+  /// <seealso class='IDTExtensibility2' />
+  public class Connect : IDTExtensibility2
+  {
+    /// <summary>
+    /// Implements the OnConnection method of the IDTExtensibility2 interface.
+    /// Receives notification that the Add-in is being loaded.
+    /// </summary>
+    /// <param term='application'>Root object of the host application.</param>
+    /// <param term='connectMode'>Describes how the Add-in is being loaded.</param>
+    /// <param term='addInInst'>Object representing this Add-in.</param>
+    /// <seealso class='IDTExtensibility2' />
+    public void OnConnection(object application, ext_ConnectMode connectMode,

[elijahtaylor1, 2012/07/11 20:56:18]

is connectMode unused intentionally?  If so maybe comment as to why we don't
care in the <param>

[tysand, 2012/07/12 23:56:15]

Done.

+        object addInInst, ref Array custom)

[elijahtaylor1, 2012/07/11 20:56:18]

You have no <param> for 'custom'... what is this? Why isn't it used?

[tysand, 2012/07/12 23:56:15]

Done.

+    {
+      applicationObject_ = (DTE2)application;
+      addInInstance_ = (AddIn)addInInst;
+
+      DebuggerHelper = new PluginDebuggerHelper(applicationObject_);
+
+      debuggerEvents_ = applicationObject_.Events.DebuggerEvents;
+      debuggerEvents_.OnEnterDesignMode += Connect.DebuggerOnEnterDesignMode;
+      debuggerEvents_.OnEnterRunMode += Connect.DebuggerOnEnterRunMode;
+    }
+
+    /// <summary>
+    /// Called when Visual Studio ends a debugging session

[noelallen1, 2012/07/11 22:16:46]

session.

All sentences end with '.'.

[tysand, 2012/07/12 23:56:15]

Done.

+    /// </summary>
+    /// <param name="reason">Unused</param>
+    public static void DebuggerOnEnterDesignMode(dbgEventReason reason)
+    {
+      DebuggerHelper.StopDebugging();
+    }
+
+    /// <summary>
+    /// Called when Visual Studio starts a debugging session

[noelallen1, 2012/07/11 22:16:46]

session.

Check your comments.

[tysand, 2012/07/12 23:56:15]

Done.

+    /// </summary>
+    /// <param name="reason"></param>

[elijahtaylor1, 2012/07/11 20:56:18]

Unused

[tysand, 2012/07/12 23:56:15]

Done.

+    public static void DebuggerOnEnterRunMode(dbgEventReason reason)
+    {
+      // If we are starting debugging (not re-entering from a breakpoint)
+      // then create our debug helper

[noelallen1, 2012/07/11 22:16:46]

Your comment suggests that you are determining if you are re-entering from a
breakpoint.  Perhpas 'reason' tells you but you ignore it?  I think what you
really mean is that you are "(re)starting" the helper if you were previously in
a "stopped" state.  It's also confusing that you would call Initialize more than
once.

In any case, what you have here may be correct and just needs a better comment.

[tysand, 2012/07/12 23:56:15]

You're right about using reason. Also, I will change
InitializeFromProjectSettings() to LoadProjectSettings() to be more clear.
On 2012/07/11 22:16:46, noelallen1 wrote:

+      if (!DebuggerHelper.IsDebuggerRunning)
+      {
+        DebuggerHelper.InitializeFromProjectSettings();

[noelallen1, 2012/07/11 22:16:46]

InitializeFromProjectSettings returns bool.  You are ignoring the failure case. 
 Seems like you have two code paths for this, one in DebuggerHelper and one
bellow in IsNativeClientSolution?

[tysand, 2012/07/12 23:56:15]

Done.

+        DebuggerHelper.StartDebugging();
+      }
+    }
+
+    /// <summary>
+    /// Implements the OnDisconnection method of the IDTExtensibility2
+    /// interface. Receives notification that the Add-in is being unloaded.
+    /// </summary>
+    /// <param term='disconnectMode'>Describes how the Add-in is being unloaded.</param>
+    /// <param term='custom'>Array of parameters that are host application specific.</param>
+    /// <seealso class='IDTExtensibility2' />
+    public void OnDisconnection(ext_DisconnectMode disconnectMode, ref Array custom)

[elijahtaylor1, 2012/07/11 20:56:18]

Can you add comments as to why we don't care about these functions that have no
bodies?

[tysand, 2012/07/12 23:56:15]

Is this necessary? We are required to implement them to satisfy the interface,
but we have no logic that needs these events so they are empty. They might be
used in the unforeseen future.
On 2012/07/11 20:56:18, elijahtaylor1 wrote:

+    {
+    }
+
+    /// <summary>
+    /// Implements the OnAddInsUpdate method of the IDTExtensibility2 interface.
+    /// Receives notification when the collection of Add-ins has changed.
+    /// </summary>
+    /// <param term='custom'>Array of parameters that are host application specific.</param>
+    /// <seealso class='IDTExtensibility2' />
+    public void OnAddInsUpdate(ref Array custom)
+    {
+    }
+
+    /// <summary>
+    /// Implements the OnStartupComplete method of the IDTExtensibility2
+    /// interface. Receives notification that the host application has completed loading.
+    /// </summary>
+    /// <param term='custom'>Array of parameters that are host application specific.</param>
+    /// <seealso class='IDTExtensibility2' />
+    public void OnStartupComplete(ref Array custom)
+    {
+    }
+
+    /// <summary>
+    /// Implements the OnBeginShutdown method of the IDTExtensibility2 interface.
+    /// Receives notification that the host application is being unloaded.
+    /// </summary>
+    /// <param term='custom'>Array of parameters that are host application specific.</param>
+    /// <seealso class='IDTExtensibility2' />
+    public void OnBeginShutdown(ref Array custom)
+    {
+    }
+
+    /// <summary>
+    /// Tells us if this add-in is applicable within this solution
+    /// </summary>
+    /// <returns>True if this add-in is applicable to this solution</returns>

[noelallen1, 2012/07/11 22:16:46]

Applicable to this solution, or to the current configuration of this solution? 
Make sure you are precise with the comments.

[tysand, 2012/07/12 23:56:15]

I've removed this function as it was outdated/unused.
On 2012/07/11 22:16:46, noelallen1 wrote:

+    private bool IsNativeClientSolution()
+    {
+      bool result = false;
+      foreach (Project proj in applicationObject_.Solution.Projects)
+      {
+        String activePlatformName = proj.ConfigurationManager.ActiveConfiguration.PlatformName;
+        if (String.Compare(activePlatformName, Strings.NaClPlatformName, true) == 0)
+        {
+          result = true;

[elijahtaylor1, 2012/07/11 20:56:18]

break?

[noelallen1, 2012/07/11 22:16:46]

If any true is true, why not return true instead of continue looping?  This
would let you get rid of the 'result' variable completely.

[tysand, 2012/07/12 23:56:15]

Done.

[tysand, 2012/07/12 23:56:15]

Done.

+        }
+      }
+
+      return result;
+    }
+
+    private DTE2 applicationObject_;

[noelallen1, 2012/07/11 22:16:46]

Private?  Where are they used?

[tysand, 2012/07/12 23:56:15]

The debuggerEvents_ object needs to not be GCed so we store it as a member. The
other objects existed to be useful in the future but were unused currently. 
I've removed them.
On 2012/07/11 22:16:46, noelallen1 wrote:

+    private AddIn addInInstance_;
+    private DebuggerEvents debuggerEvents_;
+
+    /// <summary>
+    /// Holds methods related to running the plug-in and debugging
+    /// </summary>
+    private static PluginDebuggerHelper DebuggerHelper;

[elijahtaylor1, 2012/07/11 20:56:18]

s/DebuggerHelper/debuggerHelper_/ throughout this file

[noelallen1, 2012/07/11 22:16:46]

Why is this static?  Your OnConnection function creates one every time.  There
is no test to see if it already exists.  So is the lifetime of the helper
contrained to the lifetime of the Connect object?  If so, static doesn't make
sense.  If it isn't then you need to check if it already exists?  Can there be
two Connect objects running?

[tysand, 2012/07/12 23:56:15]

Supposedly there should only be one add-in instance at a time, but you're right
that it's more appropriate to attach this to the life of the connect object.
On 2012/07/11 22:16:46, noelallen1 wrote:

+  }
+}