Refactored the VS add-in

Refactored the VS add-in

Overall we've split the old PluginDebuggerHelper into the Web server, and Debugger components and the debugger component is now a class hierarchy, where a new debugger type can be derived from PluginDebuggerBase.

We've also added PropertyManager to standardize reading properties from the property pages.


Fixed a bug where having spaces in paths broke things for install script.


Fixed unit tests and style for refactoring.


Refactored the add-in.  Split PluginDebuggerHelper into the WebServer, PropertyManager, PluginDebugger(Base, GDB, VS). Unit tests do not match yet.


BUG=136414
Committed: https://code.google.com/p/nativeclient-sdk/source/detail?r=1408

[noelallen1, 2012-08-08 00:22:15]

Just a few nits so far, still need to look at test files.

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PropertyManager.cs
(right):

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PropertyManager.cs:238:
ProjectPlatform = ProjectPlatformType.Other;
Strange we keep going...  Is this to ensure configuration_ gets set?

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs (right):

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:81: {
How can that page not exist yet?  Has it been destroyed?

[tysand, 2012-08-08 04:15:58]

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PropertyManager.cs
(right):

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PropertyManager.cs:238:
ProjectPlatform = ProjectPlatformType.Other;
On 2012/08/08 00:22:15, noelallen1 wrote:
> Strange we keep going...  Is this to ensure configuration_ gets set?

Done.

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs (right):

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/Utility.cs:81: {
Since this is just a static utility function it can't be sure that the output
pane is created yet (by the Connect class).  In theory it should always be there
though.
On 2012/08/08 00:22:15, noelallen1 wrote:
> How can that page not exist yet?  Has it been destroyed?

[noelallen1, 2012-08-08 18:33:00]

One other nit otherwise LGTM

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
File visual_studio/NativeClientVSAddIn/UnitTests/PropertyManagerTest.cs (right):

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/UnitTests/PropertyManagerTest.cs:100: string
expectedSDKRootDir =
Does the environment variable need to be set?  The user could put in the path
themselves...   It's okay if this is a test requirement but just wanted to make
sure there's no assumption for the end user.

[tysand, 2012-08-08 20:55:02]

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
File visual_studio/NativeClientVSAddIn/UnitTests/PropertyManagerTest.cs (right):

http://codereview.chromium.org/10836143/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/UnitTests/PropertyManagerTest.cs:100: string
expectedSDKRootDir =
This is an assumption of the test that NACL_SDK_ROOT is set.  We also require
users to have that set during installation, but this test is not assuming a
specific user path, just that what is read matches what is set on the test
machine.
On 2012/08/08 18:33:00, noelallen1 wrote:
> Does the environment variable need to be set?  The user could put in the path
> themselves...   It's okay if this is a test requirement but just wanted to
make
> sure there's no assumption for the end user.