Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10758009:
Native Client Visual Studio Add-in (Closed)
|
Created:
8 years, 5 months ago by tysand Modified:
8 years, 4 months ago CC:
native-client-reviews_googlegroups.com Visibility:
Public. |
DescriptionNative Client Visual Studio Add-in
Note there are many auto-generated files here from Visual Studio. All files that need reviewing have the .cs extension.
Specifically, the most important files are:
PluginDebuggerHelper.cs
Utility.cs
ProcessSearcher.cs
PluginDebuggerHelperTest.cs
TestUtilities.cs
MockProcessSearcher.cs
BUG=136414
TEST=
Committed: https://code.google.com/p/nativeclient-sdk/source/detail?r=1401
Patch Set 1 #
Total comments: 70
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Style Fixed #
Total comments: 142
Patch Set 5 : #Patch Set 6 : StyleCop #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 66
Messages
Total messages: 14 (0 generated)
Seems like a few of the files are empty. Make sure you don't have any files set to binary. Look for files that are +-1 lines. Also make sure you have LF correctly configured. Also see if you can avoid spaces in file names. Remove user files like .user and .suo
I've fixed the 'empty' files, which were caused by git interpreting them as binary because Visual Studio placed a UTF-16 indicator at the beginning of auto-generated files but oddly places a UTF-8 indicator for non-autogenerated files (what we want). Also, I removed .suo files but am keeping the .csproj.user file since it contains necessary build information and I have made it user-neutral On 2012/07/09 21:55:05, noelallen1 wrote: > Seems like a few of the files are empty. Make sure you don't have any files set > to binary. Look for files that are +-1 lines. > > Also make sure you have LF correctly configured. > > Also see if you can avoid spaces in file names. > > Remove user files like .user and .suo
Also, the .addin files need to be UTF-16 so I am leaving them untouched. On 2012/07/10 00:44:52, tysand wrote: > I've fixed the 'empty' files, which were caused by git interpreting them as > binary because Visual Studio placed a UTF-16 indicator at the beginning of > auto-generated files but oddly places a UTF-8 indicator for non-autogenerated > files (what we want). > > Also, I removed .suo files but am keeping the .csproj.user file since it > contains necessary build information and I have made it user-neutral > > On 2012/07/09 21:55:05, noelallen1 wrote: > > Seems like a few of the files are empty. Make sure you don't have any files > set > > to binary. Look for files that are +-1 lines. > > > > Also make sure you have LF correctly configured. > > > > Also see if you can avoid spaces in file names. > > > > Remove user files like .user and .suo
I have not been repeating comments for issues that are repeated over and over again in different files such as automatic properties or field naming. I would definitely consider using Microsoft StyleCop (i.e. LINT for .NET integrated into Visual Studio) check that you are following the recommended style. I do not know if there is a Google style file for StyleCop already, if not I would definitely consider creating one. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:14: List<ProcessInfo> GetResults(string Constraints); Names for method parameters should in general use camelCase. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:35: CreationDate = creationDate; Why do not you store creation date as DateTime type rather than string? Then you do not need to do more parsing and you can also do things like comparison, format conversion, etc. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:40: public uint ID For simple properties like those in this class, you can use automatic properties, i.e. public uint ID { get; set; }; the private variable in that case will be generated automatically. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:46: public uint ParentId I would probably go for ParentID name to make it consistent with ID. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:70: private uint _parentId; Underscoring private variables was recommended in the past to ensure compatibility with VB.NET, but this recommendation was already deprecated. See rule FieldNamesMustNotBeginWithUnderscore in Microsoft StyleCop fore more details. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:32: } Pre/post-checks are much better done with Code Contracts, see http://msdn.microsoft.com/en-us/devlabs/dd491992.aspx for more details. In this case you would simply use `Contract.Requires(dte != null)`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:35: _isDebuggingNow = false; I would rather do the initialization for simple variables at the point of declaration, i.e. `private bool isDebuggingNow = false`, rather than in the constructor. The advantage is that if you introduce other constructors, you do not need to repeat the same code over and over again. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:123: } Make sure you are using brackets consistently. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:209: process.CommandLine.IndexOf(typeFlagTarget, StringComparison.InvariantCultureIgnoreCase) != -1 && I would rather introduce an extension method like this `public static bool Contains(this string source, string toCheck, StringComparison comparison) { return source.IndexOf(toCheck, comparison) >= 0; }` and than simply use `!Contains(typeFlagTarget, StringComparison.InvariantCultureIgnoreCase)`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:250: contents.Add(String.Format("add-symbol-file {0} 0xC0fc00080", _irtPath.Replace("\\", "\\\\"))); I would rather consider using StringBuilder class here. You can then invoke AppendFormat(...) method rather then combination of Add(String.Format(...)). http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:14: public ProcessSearcher() Default constructor is generated automatically. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:20: List<ProcessInfo> results = new List<ProcessInfo>(); You can use `var results = new List<ProcessInfo>()` to avoid repeating type signature over and over again. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:24: using (ManagementObjectCollection result = searcher.Get()) You can combine the two using statements to one as `using (ManagementObjectSearcher searcher = new ManagementObjectSearcher(query), ManagementObjectCollection result = searcher.Get())`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:33: process["Name"] as string)); Rather than passing all arguments to constructor, you could possibly use object initializer as `new ProcessInfo { ID = process["ProcessID"], ParentID = process["ParentProcessID"], ... };` or use named arguments. It does not really save that much space, but makes code more readable since you can immediately tell which argument goes where. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:46: public List<ProcessInfo> GetResultsById(uint procId) I would again go for ID rather Id just for the sake of consistency (sorry for being picky ;) http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:49: return null; Use LINQ `return configs.FirstOrDefault(c => c.ConfigurationName == active.ConfigurationName && c.Platform.Name == active.PlatformName)`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:78: public static bool IsDescendantOfProcessHelper(IProcessSearcher processSearcher, uint descendant, uint anscestor, DateTime previousCreationTime) You can use the default value `IsDescendantOfProcess(IProcessSearcher processSearcher, uint descendant, uint anscestor, DateTime previousCreationTime = DateTime.UtcNow)` and omit the overloaded version. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:88: long timeZoneMinutes = long.Parse(proc.CreationDate.Substring(21)); If you store creation date as DateTime in ProcessInfo, you do not need to do the parsing here. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs:29: return ret; You can do this using a single expression with LINQ as `return _results.Select(p => Name.Equals(p.Name, StringComparison.OrdinalIgnoreCase))`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs:41: return ret; Same here `return _results.Select(p => procId == proc.ID)`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:26: private const string DummyLoopSolution = "\\DummyLoop\\DummyLoop.sln"; You can use non-interpreted string to avoid those double back-slashes as @"\DummyLoop\DummyLoop.sln". http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:65: DTE2 dte = TestUtilities.StartVisualStudioInstance(); Since you are using this in every single test method, could you move to a separate initialization method and mark it with [TestInitialize()] attribute? http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:76: TestUtilities.CleanUpVisualStudioInstance(dte); The same here, could you move this to a shared cleanup method and mark it with [TestCleanup()] attribute? http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:95: target._pluginProjectDirectory = TestContext.DeploymentDirectory; I would turn these into properties, accessing class fields directly is in general a code smell plus it reduces future extensibility. With automatic properties, it is just a matter of adding `{ get; set; }` to every field. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:197: delegate(object unused, PluginFoundEventArgs args) I would use lambda instead of delegate, i.e. `(unused, args) => { ... }`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:331: Process dummyProc = null; You might consider wrapping the Process into using block to ensure it is disposed properly. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:381: target._pluginFinderTimer.Tick += new EventHandler(delegate(object a, EventArgs b) { finderSuccess.Set(); }); I would use lambda here. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:403: } When handling events, you might consider using Reactive Extensions which is an evented equivalent of LINQ, see http://msdn.microsoft.com/en-us/data/gg577609.aspx for more details. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:25: DTE2 vs = Activator.CreateInstance(vsType) as DTE2; You should check for vs being null here. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:84: public static bool DoesProcessExist(String[] commandLineIdentifiers, String processName) You could possible switch the order of arguments and use varargs, i.e. `DoesProcessExists(string processName, param string[] commandLineIdentifiers`, then you do not need the overloaded method. Make sure you use consistent type names, i.e. string vs String. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:90: using (ManagementObjectCollection result = searcher.Get()) You can combine the two using statements together. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:104: return true; You can use the LINQ here `return commandLineIdentifiers.Any(i => commandLine.Contains(i))`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:141: if (context.PlatformName == platformName && context.ConfigurationName == configurationName) You should probably check here that the context is not null.
There are a few high level things that I think we should settle on before I get into the actual contents of the change, some should make this change easier to review: 1) What column limit should this code obey? Java style says 100-120, but all other code in chromium is 80. This code review tool also breaks lines at 80. I would prefer 80 columns unless it becomes extremely inconvenient. From the looks of most of the source it should be workable. 2) What tab size to use? Google3 Java and other chromium code are 2, I would prefer that. It would also make fitting into 80 columns easier. 3) Member variable naming: C++ chromium code uses underscore suffix, which is discouraged in Google3 Java. You're using an underscore prefix, which is not part of any guide I've seen. I would prefer an underscore suffix to match chromium source. 4) You should put a chromium copyright at the head of these sources that matches the C++ copyright notice.
I've implemented Elijah's suggestions about style and Petr's suggestions for improved C#. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:14: List<ProcessInfo> GetResults(string Constraints); On 2012/07/10 05:37:21, phosek1 wrote: > Names for method parameters should in general use camelCase. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:35: CreationDate = creationDate; Good catch. This was left-over after a refactoring. Done. On 2012/07/10 05:37:21, phosek1 wrote: > Why do not you store creation date as DateTime type rather than string? Then you > do not need to do more parsing and you can also do things like comparison, > format conversion, etc. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:40: public uint ID On 2012/07/10 05:37:21, phosek1 wrote: > For simple properties like those in this class, you can use automatic > properties, i.e. public uint ID { get; set; }; the private variable in that case > will be generated automatically. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:46: public uint ParentId On 2012/07/10 05:37:21, phosek1 wrote: > I would probably go for ParentID name to make it consistent with ID. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:70: private uint _parentId; To be consistent with Chrome C++ I'm changing this to an underscore suffix. On 2012/07/10 05:37:21, phosek1 wrote: > Underscoring private variables was recommended in the past to ensure > compatibility with VB.NET, but this recommendation was already deprecated. See > rule FieldNamesMustNotBeginWithUnderscore in Microsoft StyleCop fore more > details. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:32: } We examined this, and it seems that this would require installing an external package to use. On 2012/07/10 05:37:21, phosek1 wrote: > Pre/post-checks are much better done with Code Contracts, see > http://msdn.microsoft.com/en-us/devlabs/dd491992.aspx for more details. In this > case you would simply use `Contract.Requires(dte != null)`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:35: _isDebuggingNow = false; On 2012/07/10 05:37:21, phosek1 wrote: > I would rather do the initialization for simple variables at the point of > declaration, i.e. `private bool isDebuggingNow = false`, rather than in the > constructor. The advantage is that if you introduce other constructors, you do > not need to repeat the same code over and over again. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:123: } On 2012/07/10 05:37:21, phosek1 wrote: > Make sure you are using brackets consistently. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:209: process.CommandLine.IndexOf(typeFlagTarget, StringComparison.InvariantCultureIgnoreCase) != -1 && On 2012/07/10 05:37:21, phosek1 wrote: > I would rather introduce an extension method like this `public static bool > Contains(this string source, string toCheck, StringComparison comparison) { > return source.IndexOf(toCheck, comparison) >= 0; }` and than simply use > `!Contains(typeFlagTarget, StringComparison.InvariantCultureIgnoreCase)`. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:250: contents.Add(String.Format("add-symbol-file {0} 0xC0fc00080", _irtPath.Replace("\\", "\\\\"))); On 2012/07/10 05:37:21, phosek1 wrote: > I would rather consider using StringBuilder class here. You can then invoke > AppendFormat(...) method rather then combination of Add(String.Format(...)). Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:14: public ProcessSearcher() On 2012/07/10 05:37:21, phosek1 wrote: > Default constructor is generated automatically. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:20: List<ProcessInfo> results = new List<ProcessInfo>(); On 2012/07/10 05:37:21, phosek1 wrote: > You can use `var results = new List<ProcessInfo>()` to avoid repeating type > signature over and over again. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:24: using (ManagementObjectCollection result = searcher.Get()) Since they are of different types this is not allowed. Also the dependency of result on searcher is reflected by the nested statements. On 2012/07/10 05:37:21, phosek1 wrote: > You can combine the two using statements to one as `using > (ManagementObjectSearcher searcher = new ManagementObjectSearcher(query), > ManagementObjectCollection result = searcher.Get())`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:33: process["Name"] as string)); On 2012/07/10 05:37:21, phosek1 wrote: > Rather than passing all arguments to constructor, you could possibly use object > initializer as `new ProcessInfo { ID = process["ProcessID"], ParentID = > process["ParentProcessID"], ... };` or use named arguments. It does not really > save that much space, but makes code more readable since you can immediately > tell which argument goes where. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:46: public List<ProcessInfo> GetResultsById(uint procId) On 2012/07/10 05:37:21, phosek1 wrote: > I would again go for ID rather Id just for the sake of consistency (sorry for > being picky ;) Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:49: return null; Unfortunately IVCCollection is not IEnumerable Generic which is required by Linq. On 2012/07/10 05:37:21, phosek1 wrote: > Use LINQ `return configs.FirstOrDefault(c => c.ConfigurationName == > active.ConfigurationName && c.Platform.Name == active.PlatformName)`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:78: public static bool IsDescendantOfProcessHelper(IProcessSearcher processSearcher, uint descendant, uint anscestor, DateTime previousCreationTime) I've changed the helper to be private since that argument shouldn't be exposed externally. Thus the overload is necessary. On 2012/07/10 05:37:21, phosek1 wrote: > You can use the default value `IsDescendantOfProcess(IProcessSearcher > processSearcher, uint descendant, uint anscestor, DateTime previousCreationTime > = DateTime.UtcNow)` and omit the overloaded version. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:88: long timeZoneMinutes = long.Parse(proc.CreationDate.Substring(21)); On 2012/07/10 05:37:21, phosek1 wrote: > If you store creation date as DateTime in ProcessInfo, you do not need to do the > parsing here. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs:29: return ret; On 2012/07/10 05:37:21, phosek1 wrote: > You can do this using a single expression with LINQ as `return > _results.Select(p => Name.Equals(p.Name, StringComparison.OrdinalIgnoreCase))`. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs:41: return ret; On 2012/07/10 05:37:21, phosek1 wrote: > Same here `return > _results.Select(p => procId == proc.ID)`. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:26: private const string DummyLoopSolution = "\\DummyLoop\\DummyLoop.sln"; On 2012/07/10 05:37:21, phosek1 wrote: > You can use non-interpreted string to avoid those double back-slashes as > @"\DummyLoop\DummyLoop.sln". Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:65: DTE2 dte = TestUtilities.StartVisualStudioInstance(); On 2012/07/10 05:37:21, phosek1 wrote: > Since you are using this in every single test method, could you move to a > separate initialization method and mark it with [TestInitialize()] attribute? Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:76: TestUtilities.CleanUpVisualStudioInstance(dte); On 2012/07/10 05:37:21, phosek1 wrote: > The same here, could you move this to a shared cleanup method and mark it with > [TestCleanup()] attribute? Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:95: target._pluginProjectDirectory = TestContext.DeploymentDirectory; Most methods being tested are private and don't have outward visible effects. Thus to unit test them we need to set the member variables directly, but it doesn't make sense to publicly expose members just to test them.There might be an argument to make protected properties and derive the test class but this is no different in the amount of code maintenance. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:197: delegate(object unused, PluginFoundEventArgs args) Delegate seems more readable to me in this context since it's such a large function. On 2012/07/10 05:37:21, phosek1 wrote: > I would use lambda instead of delegate, i.e. `(unused, args) => { ... }`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:331: Process dummyProc = null; On 2012/07/10 05:37:21, phosek1 wrote: > You might consider wrapping the Process into using block to ensure it is > disposed properly. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:381: target._pluginFinderTimer.Tick += new EventHandler(delegate(object a, EventArgs b) { finderSuccess.Set(); }); On 2012/07/10 05:37:21, phosek1 wrote: > I would use lambda here. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:403: } Would require an external install in order to use the code, which we might not want for this project. On 2012/07/10 05:37:21, phosek1 wrote: > When handling events, you might consider using Reactive Extensions which is an > evented equivalent of LINQ, see > http://msdn.microsoft.com/en-us/data/gg577609.aspx for more details. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:25: DTE2 vs = Activator.CreateInstance(vsType) as DTE2; On 2012/07/10 05:37:21, phosek1 wrote: > You should check for vs being null here. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:84: public static bool DoesProcessExist(String[] commandLineIdentifiers, String processName) On 2012/07/10 05:37:21, phosek1 wrote: > You could possible switch the order of arguments and use varargs, i.e. > `DoesProcessExists(string processName, param string[] commandLineIdentifiers`, > then you do not need the overloaded method. > Make sure you use consistent type names, i.e. string vs String. Done. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:90: using (ManagementObjectCollection result = searcher.Get()) Can't since they are of different types. On 2012/07/10 05:37:21, phosek1 wrote: > You can combine the two using statements together. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:104: return true; Made this return true if All identifiers were found, otherwise continue the loop. On 2012/07/10 05:37:21, phosek1 wrote: > You can use the LINQ here `return commandLineIdentifiers.Any(i => > commandLine.Contains(i))`. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:141: if (context.PlatformName == platformName && context.ConfigurationName == configurationName) On 2012/07/10 05:37:21, phosek1 wrote: > You should probably check here that the context is not null. Done.
Global notes: I didn't comment on your comments much, but can you make a pass to clean them up? In particular Google comments should be complete sentences with proper punctuation. Also, we talked in person about variable/property names. I think the consensus was: member variables can remain camelCase_ and properties could be either camelCase or camelCase_. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/AssemblyInfo.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/AssemblyInfo.cs:17: [assembly: AssemblyProduct("")] Should we be filling in more information here? If we're distributing as a binary it might make sense. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:5: using System; ABC order http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:8: using EnvDTE80; add newline http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:23: public void OnConnection(object application, ext_ConnectMode connectMode, is connectMode unused intentionally? If so maybe comment as to why we don't care in the <param> http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:24: object addInInst, ref Array custom) You have no <param> for 'custom'... what is this? Why isn't it used? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:48: /// <param name="reason"></param> Unused http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:67: public void OnDisconnection(ext_DisconnectMode disconnectMode, ref Array custom) Can you add comments as to why we don't care about these functions that have no bodies? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:113: result = true; break? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:127: private static PluginDebuggerHelper DebuggerHelper; s/DebuggerHelper/debuggerHelper_/ throughout this file http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:14: /// Represents the standardized utility for retrieving the list I don't understand what "standardized utility" you're talking about here. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:66: //Example creationDate returned: "20120622150149.843021-420" space between // and Example http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:83: public uint ID { get; set; } Not sure about the naming of these properties. My preference would be that they look other variables (either with or without the underscore), these are currently named like functions. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:15: using Microsoft.VisualStudio.Shell; ABC order of 'using' directives, I don't mind if you want to group them logically like this though (i.e., Env, Microsoft, System separated). http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:23: /// "presses F5" or otherwise starts debugging from within Visual Studio. quotes not needed http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:29: /// Object is not useable until InitializeFromProjectSettings() is called. s/useable/usable/ http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:59: // We require that there is only a single start-up project This doesn't seem to be the case... it looks like it's only a warning if there is more than one startup project (also, is this even possible? I haven't used VS in a while but previously I thought only one startup project was possible) http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:97: if (Utility.IsVisualCProject(startProject)) should this be an exception/error if this is not true? It doesn't get initialized properly otherwise http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:108: int webServerPort = 5103; consider using a user parameter or a random/increasing port number for this. What happens if a server crashes and doesn't relinquish the port in a timely manner? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:117: irtPath_ = sdkRootDirectory_ + @"\tools\irt_x86_64.nexe"; why is this hardcoded to 64-bit? What about 32 bit? Even if 32-bit debugging isn't supported yet, it would be better to detect what bitsize the user is using and warn/fail on 32-bit rather than just assume 64-bit http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:133: pluginFinderTimer_.Interval = 1000; comment on units? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:215: /* To identify our target plug-in we look for: it's process name (e.g. chrome.exe), s/it's/its/ http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:229: pluginFinderForbiddenPids_.Add(process.ID); I'm unclear what the "forbidden" list is for, comments? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:233: pluginFinderTimer_.Interval = 5000; Why do we still need to check for connections if we're already connected? Do you still want to attach to more processes after finding one? If not, break http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:243: private void AttachVSDebugger(object src, PluginFoundEventArgs args) src unused http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:249: proc.Attach(); break http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:255: /// Will do magic to attach GDB to NaCl process properly, load symbols, and load breakpoints Ha, can you change this comment to something more descriptive and less magical? Something like: "Attaches the NaCl GDB debugger to the NaCl process instead of the normal VS debugger, handles loading symbols/breakpoints from VS." http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:261: private void AttachNaClGDB(object src, PluginFoundEventArgs args) src unused http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:263: // NOTE: The settings listed here are a placeholder until nacl-gdb is ready Can you be more specific? what settings? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:275: contents.AppendFormat("target remote localhost:{0}", 4014); comment on 4014 constant, maybe set a constant variable in this class or method instead http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:275: contents.AppendFormat("target remote localhost:{0}", 4014); comment on 4014, maybe use a constant variable in this class or method instead http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:277: contents.Append("set architecture i386:x86-64"); same comment as above with assuming 64-bit http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:281: contents.AppendFormat("add-symbol-file {0} 0xC00020080", pluginAssemblyEscaped); same comment as above for hardcoded constants http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:283: contents.AppendFormat("add-symbol-file {0} 0xC0fc00080", irtPathEscaped); same comment as above for hardcoded constants http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:300: } are there any other types of breakpoints we can or can't handle? Data breakpoints? Maybe check/warn for those? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:428: public event EventHandler<PluginFoundEventArgs> PluginFoundEvent; member variable naming http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:433: public bool IsDebuggerRunning { get; private set; } property naming http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:453: public uint ProcessID { get; set; } property naming http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Strings.resx (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Strings.resx:158: <value>Warning: Failed to start web server. Is python.exe on the path? Is httpd.py in the plug-in project folder?</value> s/on the path/in PATH/ http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:13: using Microsoft.VisualStudio.VCProject; ABC order http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs:59: public List<NativeClientVSAddIn.ProcessInfo> Results { get; set; } property naming http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:36: public TestContext TestContext { get; set; } property naming http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:75: // This is expected for a correct implementation you should set something here to verify you did get an exception http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:11: using EnvDTE; ABC order
You can ignore the 80 char comment as long as we agree the default for CS files is 100. 1- Clean up your comments make sure they are descriptive and correct. 2- Exit early if you fail assuming it's no longer possible to continue. 3- Check return values of bool functions, and act appropriately if you see a failure. 4- Minimize state by combining or removing. For example if two things can fail, why track them separately? Generally you are good to go or not. A partially configured state can lead to unexpected results. http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:24: /// Constructs the PluginDebuggerHelper. Object is not useable until InitializeFromProjectSettings() is called 80 Char http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:40: // Every second, check for a new instance of the plug-in to attach to 80 http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:57: // We require that there is only a single start-up project Comments should have ending '.' http://codereview.chromium.org/10758009/diff/1/visual_studio/NativeClientVSAd... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:201: /* To identify our target plug-in we look for: it's process name (e.g. chrome.exe), Avoid mixing // and /* */ in the same file. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/AssemblyInfo.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/AssemblyInfo.cs:18: [assembly: AssemblyCopyright("")] Add Copyright & Company? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/AssemblyInfo.cs:33: [assembly: AssemblyVersion("1.0.*")] How do you plan to update this? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:5: using System; Should using be in alphabetical like includes? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:9: namespace NativeClientVSAddIn LF? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:37: /// Called when Visual Studio ends a debugging session session. All sentences end with '.'. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:46: /// Called when Visual Studio starts a debugging session session. Check your comments. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:52: // then create our debug helper 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. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:55: DebuggerHelper.InitializeFromProjectSettings(); 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? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:104: /// <returns>True if this add-in is applicable to this solution</returns> Applicable to this solution, or to the current configuration of this solution? Make sure you are precise with the comments. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:113: result = true; If any true is true, why not return true instead of continue looping? This would let you get rid of the 'result' variable completely. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:120: private DTE2 applicationObject_; Private? Where are they used? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:127: private static PluginDebuggerHelper DebuggerHelper; 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? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:15: /// of running processes on the system system. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:43: pluginFinderTimer_ = new Timer(); Who is providing Timer? Is this a system Timer, is it called on a different thread? Should FindAndAttachToPlugin be static and use appropriate locking? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:107: Why track isPropertyInitialized, why not return false? As a general rule, fail early makes code easier to read. So if your object can not be used, why continue initializing it? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:126: { Should this throw or return false? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:142: { Are there really two sets of state you care about? IsDebuggerRunning and IsInitialize? Don't you just care about IsRunning? IsRunning = True implied it must have been initalized. Since you always do both at the same time... http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:249: proc.Attach(); Should you break or return. Can the same ProcessID show up twice? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:281: contents.AppendFormat("add-symbol-file {0} 0xC00020080", pluginAssemblyEscaped); Are you sure this value is fixed? Can't it change? Also, how will you handle 32 bit when that is enabled? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:389: WinGDB is WinGDB unused? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:28: using (ManagementObjectSearcher searcher = new ManagementObjectSearcher(query)) Why using scope here, but not in other locations? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:84: /// </summary> Lost me on the last bit. Aren't parent's always created before the child they create by definition? Do you mean: The solution is to check if the ID was created before or after. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/NotNaCl/Program.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/NotNaCl/Program.cs:5: What is this file used for?
Found few more issues and opportunities for improvement. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:15: using Microsoft.VisualStudio.Shell; On 2012/07/11 20:56:18, elijahtaylor1 wrote: > ABC order of 'using' directives, I don't mind if you want to group them > logically like this though (i.e., Env, Microsoft, System separated). You can use 'Remove and Sort' feature from 'Organize Usings' in editor context menu to do that for you. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:100: VCProject vcproj = (VCProject)startProject.Object; I would rather use `startProject.Object as VCProject` here and then check for null. This way it will throw an exception in case the cast is invalid which is something you probably want to avoid. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:133: pluginFinderTimer_.Interval = 1000; On 2012/07/11 20:56:18, elijahtaylor1 wrote: > comment on units? How about using a constant? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:27: String query = String.Format("select {0} from Win32_Process where {1}", fields, constraints); I would rather go for LINQ here to make the query strongly typed which is always better than strings (let the compiler do the hard work). There is a bit of an issue with this since WMI doesn't yet support LINQ, but you can go around that using the following approach: using(ManagementObjectSearcher searcher = new ManagementObjectSearcher("SELECT * FROM Win32_Process")) { var unit = searcher.Get().Cast<ManagementObject>().ToList().Where(constraints).Select(p => new ProcessInfo(...)); } The SQL query can be turned into a constant. The constraint now will be Func<ManagementBaseObject, bool> rather than string. The ProcessInfo construction will look the same as before. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:54: return GetResults(String.Format("Name='{0}'", name)); Using the change above, this will now look as follow: return GetResults(m => m.Properties["Name"].Value == name); http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:64: return GetResults(String.Format("ProcessID='{0}'", procID)); The same here: return GetResults(m => m.Properties["ProcessID"].Value == procID); http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:31: return projectType == "VCProject"; Possibly make this into a constant?
I've addressed Noel's, Elijah's and Petr's (2nd round) comments. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/AssemblyInfo.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/AssemblyInfo.cs:17: [assembly: AssemblyProduct("")] On 2012/07/11 20:56:18, elijahtaylor1 wrote: > Should we be filling in more information here? If we're distributing as a > binary it might make sense. Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/AssemblyInfo.cs:18: [assembly: AssemblyCopyright("")] On 2012/07/11 22:16:46, noelallen1 wrote: > Add Copyright & Company? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/AssemblyInfo.cs:33: [assembly: AssemblyVersion("1.0.*")] The Visual Studio functionality can be updated by replacing this dll. To maintain backwards compatibility with project settings we can provide a utility to update project settings from older versions. Versions of the Add-in need to be backwards compatible with several versions of the SDK, as such I am considering refactoring the code, for example, to have PluginDebuggerHelper as a base class, and overriding functionality depending on the SDK version in classes like PluginDebuggerHelperSDK22. Yay or nay? On 2012/07/11 22:16:46, noelallen1 wrote: > How do you plan to update this? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:5: using System; On 2012/07/11 20:56:18, elijahtaylor1 wrote: > ABC order Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:5: using System; On 2012/07/11 22:16:46, noelallen1 wrote: > Should using be in alphabetical like includes? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:8: using EnvDTE80; On 2012/07/11 20:56:18, elijahtaylor1 wrote: > add newline Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:9: namespace NativeClientVSAddIn On 2012/07/11 22:16:46, noelallen1 wrote: > LF? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:23: public void OnConnection(object application, ext_ConnectMode connectMode, On 2012/07/11 20:56:18, elijahtaylor1 wrote: > is connectMode unused intentionally? If so maybe comment as to why we don't > care in the <param> Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:24: object addInInst, ref Array custom) On 2012/07/11 20:56:18, elijahtaylor1 wrote: > You have no <param> for 'custom'... what is this? Why isn't it used? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:37: /// Called when Visual Studio ends a debugging session On 2012/07/11 22:16:46, noelallen1 wrote: > session. > > All sentences end with '.'. Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:46: /// Called when Visual Studio starts a debugging session On 2012/07/11 22:16:46, noelallen1 wrote: > session. > > Check your comments. Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:48: /// <param name="reason"></param> On 2012/07/11 20:56:18, elijahtaylor1 wrote: > Unused Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:52: // then create our debug helper 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: > 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. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:55: DebuggerHelper.InitializeFromProjectSettings(); On 2012/07/11 22:16:46, noelallen1 wrote: > 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? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:67: public void OnDisconnection(ext_DisconnectMode disconnectMode, ref Array custom) 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: > Can you add comments as to why we don't care about these functions that have no > bodies? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:104: /// <returns>True if this add-in is applicable to this solution</returns> I've removed this function as it was outdated/unused. On 2012/07/11 22:16:46, noelallen1 wrote: > Applicable to this solution, or to the current configuration of this solution? > Make sure you are precise with the comments. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:113: result = true; On 2012/07/11 20:56:18, elijahtaylor1 wrote: > break? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:113: result = true; On 2012/07/11 22:16:46, noelallen1 wrote: > If any true is true, why not return true instead of continue looping? This > would let you get rid of the 'result' variable completely. Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:120: private DTE2 applicationObject_; 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? Where are they used? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:127: private static PluginDebuggerHelper DebuggerHelper; 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: > 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? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:14: /// Represents the standardized utility for retrieving the list On 2012/07/11 20:56:18, elijahtaylor1 wrote: > I don't understand what "standardized utility" you're talking about here. Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:15: /// of running processes on the system On 2012/07/11 22:16:46, noelallen1 wrote: > system. Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:66: //Example creationDate returned: "20120622150149.843021-420" On 2012/07/11 20:56:18, elijahtaylor1 wrote: > space between // and Example Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/IProcessSearcher.cs:83: public uint ID { get; set; } To stay consistent with standard C# style (as used by Microsoft and a majority of existing 3rd party C# code) we have decided to keep properties in PascalCase. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > Not sure about the naming of these properties. My preference would be that they > look other variables (either with or without the underscore), these are > currently named like functions. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:15: using Microsoft.VisualStudio.Shell; On 2012/07/11 20:56:18, elijahtaylor1 wrote: > ABC order of 'using' directives, I don't mind if you want to group them > logically like this though (i.e., Env, Microsoft, System separated). Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:15: using Microsoft.VisualStudio.Shell; On 2012/07/12 00:25:46, phosek1 wrote: > On 2012/07/11 20:56:18, elijahtaylor1 wrote: > > ABC order of 'using' directives, I don't mind if you want to group them > > logically like this though (i.e., Env, Microsoft, System separated). > > You can use 'Remove and Sort' feature from 'Organize Usings' in editor context > menu to do that for you. Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:23: /// "presses F5" or otherwise starts debugging from within Visual Studio. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > quotes not needed Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:29: /// Object is not useable until InitializeFromProjectSettings() is called. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > s/useable/usable/ Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:43: pluginFinderTimer_ = new Timer(); This is a System.Windows.Forms.Timer. It works by spawning a thread to run the timer, but when the time has elapsed it adds a message on the main UI thread's message queue instead of firing its event directly. The actual event is fired from the main UI thread when the message is processed. Thus no locking is required in my code. I'll add a comment. On 2012/07/11 22:16:46, noelallen1 wrote: > Who is providing Timer? Is this a system Timer, is it called on a different > thread? Should FindAndAttachToPlugin be static and use appropriate locking? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:59: // We require that there is only a single start-up project Yes, you can specify multiple start-up projects in the solution settings. If this is the case we only use the first project (see line 72: startupProjects.GetValue(0)). I note this in the actual warning message but I should also leave a comment here. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > This doesn't seem to be the case... it looks like it's only a warning if there > is more than one startup project (also, is this even possible? I haven't used > VS in a while but previously I thought only one startup project was possible) http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:97: if (Utility.IsVisualCProject(startProject)) No, the idea is that we return true if everything initialized properly and this indicates that we should be running all of our NaCl/Pepper specific code. The add-in will be running even when the user is developing non-nacl projects and we don't want to error out if this is the case. However, it might be more clear to have an else case that returns false. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > should this be an exception/error if this is not true? It doesn't get > initialized properly otherwise http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:100: VCProject vcproj = (VCProject)startProject.Object; The function IsVisualCProject should ensure that this cast is valid. And if it's not then throwing an exception is what we should do so that we can debug IsVisualCProject. We don't want to silently fail since this case should never happen. On 2012/07/12 00:25:46, phosek1 wrote: > I would rather use `startProject.Object as VCProject` here and then check for > null. This way it will throw an exception in case the cast is invalid which is > something you probably want to avoid. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:107: On 2012/07/11 22:16:46, noelallen1 wrote: > Why track isPropertyInitialized, why not return false? > As a general rule, fail early makes code easier to read. So if your object can > not be used, why continue initializing it? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:108: int webServerPort = 5103; This is a future todo. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > consider using a user parameter or a random/increasing port number for this. > What happens if a server crashes and doesn't relinquish the port in a timely > manner? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:117: irtPath_ = sdkRootDirectory_ + @"\tools\irt_x86_64.nexe"; This code is a placeholder for the real nacl-gdb, which won't require us to load the irt ourselves, so this will be removed in the future. I'll add TODOs. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > why is this hardcoded to 64-bit? What about 32 bit? Even if 32-bit debugging > isn't supported yet, it would be better to detect what bitsize the user is using > and warn/fail on 32-bit rather than just assume 64-bit http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:126: { Probably. I'm changing it to throw. On 2012/07/11 22:16:46, noelallen1 wrote: > Should this throw or return false? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:133: pluginFinderTimer_.Interval = 1000; On 2012/07/11 20:56:18, elijahtaylor1 wrote: > comment on units? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:133: pluginFinderTimer_.Interval = 1000; On 2012/07/12 00:25:46, phosek1 wrote: > On 2012/07/11 20:56:18, elijahtaylor1 wrote: > > comment on units? > > How about using a constant? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:142: { Initially IsDebuggerRunning was used externally by the Connect class to determine the state of Visual Studio debugging. After a recent change this is no longer used, so while they are different concepts, I will remove IsDebuggerRunning. On 2012/07/11 22:16:46, noelallen1 wrote: > Are there really two sets of state you care about? IsDebuggerRunning and > IsInitialize? Don't you just care about IsRunning? IsRunning = True implied it > must have been initalized. Since you always do both at the same time... http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:215: /* To identify our target plug-in we look for: it's process name (e.g. chrome.exe), On 2012/07/11 20:56:18, elijahtaylor1 wrote: > s/it's/its/ Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:229: pluginFinderForbiddenPids_.Add(process.ID); Comment added: If we are attaching to a plug-in, add it to the forbidden list to ensure we don't try to attach again later. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > I'm unclear what the "forbidden" list is for, comments? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:233: pluginFinderTimer_.Interval = 5000; Although contrived, there could be multiple processes we want to attach to, for example if the user opens a second tab which also loads the plug-in. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > Why do we still need to check for connections if we're already connected? > > Do you still want to attach to more processes after finding one? If not, break http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:243: private void AttachVSDebugger(object src, PluginFoundEventArgs args) On 2012/07/11 20:56:18, elijahtaylor1 wrote: > src unused Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:249: proc.Attach(); On 2012/07/11 20:56:18, elijahtaylor1 wrote: > break Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:255: /// Will do magic to attach GDB to NaCl process properly, load symbols, and load breakpoints Heh yep I'll change it now. But the magic should go away as soon as nacl-gdb is ready. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > Ha, can you change this comment to something more descriptive and less magical? > Something like: "Attaches the NaCl GDB debugger to the NaCl process instead of > the normal VS debugger, handles loading symbols/breakpoints from VS." http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:261: private void AttachNaClGDB(object src, PluginFoundEventArgs args) On 2012/07/11 20:56:18, elijahtaylor1 wrote: > src unused Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:263: // NOTE: The settings listed here are a placeholder until nacl-gdb is ready On 2012/07/11 20:56:18, elijahtaylor1 wrote: > Can you be more specific? what settings? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:275: contents.AppendFormat("target remote localhost:{0}", 4014); On 2012/07/11 20:56:18, elijahtaylor1 wrote: > comment on 4014, maybe use a constant variable in this class or method instead Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:277: contents.Append("set architecture i386:x86-64"); On 2012/07/11 20:56:18, elijahtaylor1 wrote: > same comment as above with assuming 64-bit Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:281: contents.AppendFormat("add-symbol-file {0} 0xC00020080", pluginAssemblyEscaped); The value technically shouldn't be fixed in some cases (I'm unsure about the specifics, ask Christian). However, the current setting works in most cases. Nacl-GDB will handle this value when it is ready, and calculating it here would take an unreasonable amount of work for a placeholder. On 2012/07/11 22:16:46, noelallen1 wrote: > Are you sure this value is fixed? Can't it change? Also, how will you handle > 32 bit when that is enabled? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:283: contents.AppendFormat("add-symbol-file {0} 0xC0fc00080", irtPathEscaped); On 2012/07/11 20:56:18, elijahtaylor1 wrote: > same comment as above for hardcoded constants Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:300: } On 2012/07/11 20:56:18, elijahtaylor1 wrote: > are there any other types of breakpoints we can or can't handle? Data > breakpoints? Maybe check/warn for those? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:389: WinGDB On 2012/07/11 22:16:46, noelallen1 wrote: > is WinGDB unused? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:428: public event EventHandler<PluginFoundEventArgs> PluginFoundEvent; Microsoft style on events is PascalCase because I think they are essentially properties. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > member variable naming http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:433: public bool IsDebuggerRunning { get; private set; } On 2012/07/11 20:56:18, elijahtaylor1 wrote: > property naming Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:453: public uint ProcessID { get; set; } On 2012/07/11 20:56:18, elijahtaylor1 wrote: > property naming Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:27: String query = String.Format("select {0} from Win32_Process where {1}", fields, constraints); Neat. Done. On 2012/07/12 00:25:46, phosek1 wrote: > I would rather go for LINQ here to make the query strongly typed which is always > better than strings (let the compiler do the hard work). There is a bit of an > issue with this since WMI doesn't yet support LINQ, but you can go around that > using the following approach: > > using(ManagementObjectSearcher searcher = new ManagementObjectSearcher("SELECT * > FROM Win32_Process")) > { > var unit = > searcher.Get().Cast<ManagementObject>().ToList().Where(constraints).Select(p => > new ProcessInfo(...)); > } > > The SQL query can be turned into a constant. The constraint now will be > Func<ManagementBaseObject, bool> rather than string. The ProcessInfo > construction will look the same as before. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:28: using (ManagementObjectSearcher searcher = new ManagementObjectSearcher(query)) Because ManagementObjectCollection and ManagementObject implement the IDisposable interface which means we need to call Dispose() on them to clean up unmanaged resources. The using statement ensures this happens. On 2012/07/11 22:16:46, noelallen1 wrote: > Why using scope here, but not in other locations? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:54: return GetResults(String.Format("Name='{0}'", name)); On 2012/07/12 00:25:46, phosek1 wrote: > Using the change above, this will now look as follow: > > return GetResults(m => m.Properties["Name"].Value == name); Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessSearcher.cs:64: return GetResults(String.Format("ProcessID='{0}'", procID)); On 2012/07/12 00:25:46, phosek1 wrote: > The same here: > > return GetResults(m => m.Properties["ProcessID"].Value == procID); Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Strings.resx (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Strings.resx:158: <value>Warning: Failed to start web server. Is python.exe on the path? Is httpd.py in the plug-in project folder?</value> On 2012/07/11 20:56:18, elijahtaylor1 wrote: > s/on the path/in PATH/ Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:13: using Microsoft.VisualStudio.VCProject; On 2012/07/11 20:56:18, elijahtaylor1 wrote: > ABC order Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:31: return projectType == "VCProject"; I think this is essentially already handled by having this function dedicated to VCProject, adding a constant would just be verbose. On 2012/07/12 00:25:46, phosek1 wrote: > Possibly make this into a constant? http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:84: /// </summary> Right, the word 'ensure' is key. I will rephrase for clarity. On 2012/07/11 22:16:46, noelallen1 wrote: > Lost me on the last bit. Aren't parent's always created before the child they > create by definition? Do you mean: The solution is to check if the ID was > created before or after. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/NotNaCl/Program.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/NotNaCl/Program.cs:5: On 2012/07/11 22:16:46, noelallen1 wrote: > What is this file used for? Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/MockProcessSearcher.cs:59: public List<NativeClientVSAddIn.ProcessInfo> Results { get; set; } On 2012/07/11 20:56:18, elijahtaylor1 wrote: > property naming Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:36: public TestContext TestContext { get; set; } On 2012/07/11 20:56:18, elijahtaylor1 wrote: > property naming Done. http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:75: // This is expected for a correct implementation Note that we Assert.Fail above in the try block if no exception is thrown. A test is considered passing if nothing fails. If the function throws a different exception, this won't be caught and will also be considered a failure. On 2012/07/11 20:56:18, elijahtaylor1 wrote: > you should set something here to verify you did get an exception http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs (right): http://codereview.chromium.org/10758009/diff/12002/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:11: using EnvDTE; On 2012/07/11 20:56:18, elijahtaylor1 wrote: > ABC order Done.
LGTM. We'll walk through the testing stuff and get another cl to integrate it into the SDK build. http://codereview.chromium.org/10758009/diff/26001/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/26001/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:434: // TODO: Nacl-gdb should handle the offset automatically. Remove 0xC00020080. Use // TODO(tysand) Also double check this is true. NaCl-GDB will need to disambiguate between trusted and untrusted code, so I'm not sure we can avoid the address here.
http://codereview.chromium.org/10758009/diff/26001/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/26001/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:434: // TODO: Nacl-gdb should handle the offset automatically. Remove 0xC00020080. Currently there is no option to use NaCl-GDB on untrusted code. When that is added we will test that scenario. On 2012/07/16 06:47:20, noelallen1 wrote: > Use // TODO(tysand) > Also double check this is true. NaCl-GDB will need to disambiguate between > trusted and untrusted code, so I'm not sure we can avoid the address here.
In general, I'd like to see the PluginDebuggerHelper class broken up into smaller components. I think some of your tests are more complex than they need to be because the class does too much. That can wait for another CL though. :) http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:65: /// <param name="reason">The parameter is not used.</param> This parameter is used below... http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:94: private System.Diagnostics.Process webServer_ = null; nit: = null unnecessary? Doesn't c# initialize reference types to null by default? http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:99: private OutputWindowPane webServerOutputPane_ = null; ditto here http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:240: int webServerPort = 5103; move this down to where it is used http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:272: throw new Exception(Strings.NotInitializedMessage); this looks like if you install the add-in, and try to load a project that fails LoadProjectSettings(), it will throw an exception here... does this show an error message to the user? If so, I don't think this is desired behavior. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:285: isProperlyInitialized_ = false; might be simpler to just let the GC collect this object when debugging stops. That way you don't have to do all this cleanup, just kill the webserver and the gdb processes. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:300: webServer_.Kill(); this will throw if the web server process was killed outside this function (and other reasons). Probably better to just kill and catch exceptions. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:315: if (!gdbProcess_.HasExited) Same here, just kill and catch exceptions. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:317: gdbProcess_.Kill(); According to http://msdn.microsoft.com/en-us/library/system.diagnostics.process.kill.aspx, Kill is asynchronous. Maybe should use WaitForExit with a timeout? Not sure if gdb is sane if you launch another copy while the other is still running. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:323: File.Delete(gdbInitFileName_); Again, usually better to just do the operation and catch exceptions when dealing with shared resources. Also, this doesn't really have anything to do with killing the gdb process, so you should either change the method name or move this part out. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:416: gdbInitFileName_ = Path.GetTempFileName(); consider moving all of the nacl-gdb stuff into its own class. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:445: if (bp.Enabled && if (!bp.Enabled) continue; if (bp.LocationType... http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:487: private void StartWebServer() Consider moving all of the webserver stuff into its own class. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:507: webServer_.StartInfo.FileName = webServerExecutable_; I think it's cleaner to avoid using a member variables for webServerExecutable_ and webServerArguments_. Just calculate and use them here. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:540: webServerOutputPane_.Activate(); This seems to set focus to the output pane... is that necessary for all output from the webserver? http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessInfo.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessInfo.cs:32: creationDate = string.Format("{0:yyyyMMddHHmmss.ffffff}{1}", DateTime.Now, timezoneMinutes); seems weird to create the date string here just to parse it below. Also, if it is empty, why do we want to set it to now? Where is this used? http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessInfo.cs:79: public static explicit operator ProcessInfo(ManagementObject from) Seems more natural to just have a second constructor for this. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:9: <meta http-equiv="Pragma" content="no-cache" /> <meta> tag does not need to be closed http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:23: if (opt_message == "relay1") { nit: seems like the message ping-ponging makes more sense in handleMessage. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:38: <div id="listener"> nit: match the NaCl SDK example layout: <h1>title</h1> <h2>Status: <code id="statusField">NO-STATUS</code></h2> <div>Description of example</div> <div id="listener"></div> ... http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:43: <embed name='nacl_module' nit: use double-quotes for element attributes http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:44: id='dummy_loop' nit: use nacl_module for id. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:165: if (!string.IsNullOrEmpty(target.gdbInitFileName_) && File.Exists(target.gdbInitFileName_)) This seems like duplication of the responsibilities of the class you're testing. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:170: if (target.gdbProcess_ != null && !target.gdbProcess_.HasExited) same here http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:349: ////Assert.AreEqual(target._webServerArguments, ""); Remove commented code. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:392: new NativeClientVSAddIn.PluginDebuggerHelper.PluginFoundEventArgs((uint)dummyProc.Id)); nit: wrap at 100 chars http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:410: dummyProc.Dispose(); dispose is unnecessary because of using above, right? http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:420: public void StartDebuggingTest() I don't understand what this is testing. It looks like it is just checking to see if the pluginFinderTimer fires -- is that the only observable result from calling StartDebugging? http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:503: string result = TestUtilities.GetPaneText(target.webServerOutputPane_); move this inside the for loop, remove the duplicated code at the end of the loop http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:553: target.pluginFinderForbiddenPids_.Add(123); what is 123? http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:589: string result = TestUtilities.GetPaneText(target.webServerOutputPane_); same as above, move inside loop, etc. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:595: result.Contains(successMessage)) nit: looks like this should fit on the previous line http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:21: /// This starts an instance of Visual Studio and get it's DTE object. s/it's/its/
I've fixed Ben's comments. The minor ones have gone into other CLs and the major one (the refactoring) is here: http://codereview.chromium.org/10758009/ http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Connect.cs:65: /// <param name="reason">The parameter is not used.</param> On 2012/07/20 00:24:53, binji wrote: > This parameter is used below... Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:94: private System.Diagnostics.Process webServer_ = null; On 2012/07/20 00:24:53, binji wrote: > nit: = null unnecessary? > Doesn't c# initialize reference types to null by default? Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:99: private OutputWindowPane webServerOutputPane_ = null; On 2012/07/20 00:24:53, binji wrote: > ditto here Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:240: int webServerPort = 5103; On 2012/07/20 00:24:53, binji wrote: > move this down to where it is used Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:272: throw new Exception(Strings.NotInitializedMessage); On 2012/07/20 00:24:53, binji wrote: > this looks like if you install the add-in, and try to load a project that fails > LoadProjectSettings(), it will throw an exception here... does this show an > error message to the user? If so, I don't think this is desired behavior. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:285: isProperlyInitialized_ = false; On 2012/07/20 00:24:53, binji wrote: > might be simpler to just let the GC collect this object when debugging stops. > That way you don't have to do all this cleanup, just kill the webserver and the > gdb processes. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:300: webServer_.Kill(); On 2012/07/20 00:24:53, binji wrote: > this will throw if the web server process was killed outside this function (and > other reasons). Probably better to just kill and catch exceptions. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:315: if (!gdbProcess_.HasExited) On 2012/07/20 00:24:53, binji wrote: > Same here, just kill and catch exceptions. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:317: gdbProcess_.Kill(); On 2012/07/20 00:24:53, binji wrote: > According to > http://msdn.microsoft.com/en-us/library/system.diagnostics.process.kill.aspx, > Kill is asynchronous. Maybe should use WaitForExit with a timeout? Not sure if > gdb is sane if you launch another copy while the other is still running. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:323: File.Delete(gdbInitFileName_); The file name is randomly generated so it should be unique to this gdb instance. No conflict with sharing. On 2012/07/20 00:24:53, binji wrote: > Again, usually better to just do the operation and catch exceptions when dealing > with shared resources. > > Also, this doesn't really have anything to do with killing the gdb process, so > you should either change the method name or move this part out. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:416: gdbInitFileName_ = Path.GetTempFileName(); On 2012/07/20 00:24:53, binji wrote: > consider moving all of the nacl-gdb stuff into its own class. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:445: if (bp.Enabled && On 2012/07/20 00:24:53, binji wrote: > if (!bp.Enabled) continue; > > if (bp.LocationType... Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:487: private void StartWebServer() On 2012/07/20 00:24:53, binji wrote: > Consider moving all of the webserver stuff into its own class. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:507: webServer_.StartInfo.FileName = webServerExecutable_; On 2012/07/20 00:24:53, binji wrote: > I think it's cleaner to avoid using a member variables for webServerExecutable_ > and webServerArguments_. Just calculate and use them here. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerHelper.cs:540: webServerOutputPane_.Activate(); Generally nothing else is happening at runtime and I wanted the output window to be noticed. However I could see scenarios where the user wanted to read something else and the window kept switching. I will remove this. On 2012/07/20 00:24:53, binji wrote: > This seems to set focus to the output pane... is that necessary for all output > from the webserver? http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessInfo.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessInfo.cs:32: creationDate = string.Format("{0:yyyyMMddHHmmss.ffffff}{1}", DateTime.Now, timezoneMinutes); On 2012/07/20 00:24:53, binji wrote: > seems weird to create the date string here just to parse it below. Also, if it > is empty, why do we want to set it to now? Where is this used? Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/ProcessInfo.cs:79: public static explicit operator ProcessInfo(ManagementObject from) On 2012/07/20 00:24:53, binji wrote: > Seems more natural to just have a second constructor for this. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:9: <meta http-equiv="Pragma" content="no-cache" /> On 2012/07/20 00:24:53, binji wrote: > <meta> tag does not need to be closed Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:23: if (opt_message == "relay1") { On 2012/07/20 00:24:53, binji wrote: > nit: seems like the message ping-ponging makes more sense in handleMessage. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:38: <div id="listener"> Done. I rearranged to match layout but excluded title and description. On 2012/07/20 00:24:53, binji wrote: > nit: match the NaCl SDK example layout: > <h1>title</h1> > <h2>Status: <code id="statusField">NO-STATUS</code></h2> > <div>Description of example</div> > <div id="listener"></div> > ... http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:43: <embed name='nacl_module' On 2012/07/20 00:24:53, binji wrote: > nit: use double-quotes for element attributes Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html:44: id='dummy_loop' On 2012/07/20 00:24:53, binji wrote: > nit: use nacl_module for id. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:165: if (!string.IsNullOrEmpty(target.gdbInitFileName_) && File.Exists(target.gdbInitFileName_)) Yes, but if the test fails then we're leaking files and processes each time we run the tests. Definitely dont want that. On 2012/07/20 00:24:53, binji wrote: > This seems like duplication of the responsibilities of the class you're testing. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:170: if (target.gdbProcess_ != null && !target.gdbProcess_.HasExited) On 2012/07/20 00:24:53, binji wrote: > same here Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:349: ////Assert.AreEqual(target._webServerArguments, ""); On 2012/07/20 00:24:53, binji wrote: > Remove commented code. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:392: new NativeClientVSAddIn.PluginDebuggerHelper.PluginFoundEventArgs((uint)dummyProc.Id)); On 2012/07/20 00:24:53, binji wrote: > nit: wrap at 100 chars Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:410: dummyProc.Dispose(); On 2012/07/20 00:24:53, binji wrote: > dispose is unnecessary because of using above, right? Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:420: public void StartDebuggingTest() On 2012/07/20 00:24:53, binji wrote: > I don't understand what this is testing. It looks like it is just checking to > see if the pluginFinderTimer fires -- is that the only observable result from > calling StartDebugging? Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:503: string result = TestUtilities.GetPaneText(target.webServerOutputPane_); On 2012/07/20 00:24:53, binji wrote: > move this inside the for loop, remove the duplicated code at the end of the loop Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:553: target.pluginFinderForbiddenPids_.Add(123); On 2012/07/20 00:24:53, binji wrote: > what is 123? Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:589: string result = TestUtilities.GetPaneText(target.webServerOutputPane_); On 2012/07/20 00:24:53, binji wrote: > same as above, move inside loop, etc. Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/PluginDebuggerHelperTest.cs:595: result.Contains(successMessage)) On 2012/07/20 00:24:53, binji wrote: > nit: looks like this should fit on the previous line Done. http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... File visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs (right): http://codereview.chromium.org/10758009/diff/32004/visual_studio/NativeClient... visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs:21: /// This starts an instance of Visual Studio and get it's DTE object. On 2012/07/20 00:24:53, binji wrote: > s/it's/its/ Done. |
