Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(123)

Issue 10797040: PPAPI Patching System and Unit Tests (Closed)

Created:
8 years, 5 months ago by tysand
Modified:
8 years, 5 months ago
Reviewers:
noelallen1, binji
CC:
native-client-reviews_googlegroups.com
Visibility:
Public.

Description

There are two things in this review: Unit Tests to verify that project settings default to the correct values, and a patching system to copy Win32 settings to create the PPAPI template. Note the only code that needs reviewing is the .py and .cs files. The other files are patch xml files that create the PPAPI template. Added the patching system to copy the Win32 settings to create the PPAPI settings. Removed the vsmdi file since it is better as a developer dependent file. Added unit tests to verify the default settings for PPAPI and NaCl platforms. Added ProjectSettingsTest project BUG=136414 TEST= Committed: https://code.google.com/p/nativeclient-sdk/source/detail?r=1404

Patch Set 1 #

Total comments: 51

Patch Set 2 : Finalized PPAPI settings #

Total comments: 6

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1061 lines, -44 lines) Patch
A visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/ImportAfter/PPAPI.override.props View 1 1 chunk +33 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/Microsoft.Cpp.[platform].default.props.patch View 1 1 chunk +29 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/Microsoft.Cpp.[platform].props.patch View 1 chunk +20 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/Microsoft.Cpp.[platform].targets.patch View 1 chunk +63 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/PlatformToolsets/v100/microsoft.cpp.[platform].v100.props.patch View 1 1 chunk +31 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/PlatformToolsets/v100/microsoft.cpp.[platform].v100.targets.patch View 1 chunk +14 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/Props/ppapi_general.xml.patch View 1 1 chunk +45 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/Props/ppapi_general_ps.xml.patch View 1 chunk +7 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/InstallerResources/create_ppapi_platform.py View 1 2 1 chunk +180 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/InstallerResources/xml_patch.py View 1 2 1 chunk +174 lines, -0 lines 0 comments Download
D visual_studio/NativeClientVSAddIn/NativeClientVSAddIn.vsmdi View 1 chunk +0 lines, -6 lines 0 comments Download
M visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/index.html View 1 2 chunks +16 lines, -16 lines 0 comments Download
M visual_studio/NativeClientVSAddIn/TestingProjects/DummyLoop/DummyLoop/main.cpp View 1 2 chunks +5 lines, -3 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/TestingProjects/ProjectSettingsTest/ProjectSettingsTest.sln View 1 chunk +20 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/TestingProjects/ProjectSettingsTest/ProjectSettingsTest/ProjectSettingsTest.vcxproj View 1 chunk +85 lines, -0 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/TestingProjects/ProjectSettingsTest/ProjectSettingsTest/ProjectSettingsTest.vcxproj.user View 1 chunk +3 lines, -0 lines 0 comments Download
A + visual_studio/NativeClientVSAddIn/TestingProjects/ProjectSettingsTest/ProjectSettingsTest/index.html View 1 2 chunks +16 lines, -16 lines 0 comments Download
A + visual_studio/NativeClientVSAddIn/TestingProjects/ProjectSettingsTest/ProjectSettingsTest/main.cpp View 1 2 chunks +5 lines, -3 lines 0 comments Download
A visual_studio/NativeClientVSAddIn/UnitTests/ProjectSettingsTest.cs View 1 1 chunk +227 lines, -0 lines 0 comments Download
M visual_studio/NativeClientVSAddIn/UnitTests/TestUtilities.cs View 2 chunks +87 lines, -0 lines 0 comments Download
M visual_studio/NativeClientVSAddIn/UnitTests/UnitTests.csproj View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
tysand
8 years, 5 months ago (2012-07-20 04:11:18 UTC) #1
noelallen1
I didn't see where you actually drive the compiler and verify the nexe was created. ...
8 years, 5 months ago (2012-07-20 21:46:00 UTC) #2
binji
http://codereview.chromium.org/10797040/diff/1/visual_studio/NativeClientVSAddIn/InstallerResources/create_ppapi_platform.py File visual_studio/NativeClientVSAddIn/InstallerResources/create_ppapi_platform.py (right): http://codereview.chromium.org/10797040/diff/1/visual_studio/NativeClientVSAddIn/InstallerResources/create_ppapi_platform.py#newcode62 visual_studio/NativeClientVSAddIn/InstallerResources/create_ppapi_platform.py:62: source_file = open(source_file_name, 'r') with open(...) as source_file: ...
8 years, 5 months ago (2012-07-20 22:54:49 UTC) #3
tysand
Fixed Ben and Noel's suggestions. Finalized PPAPI settings. Removed NaCl tests for now until I ...
8 years, 5 months ago (2012-07-24 21:24:15 UTC) #4
binji
a few changes, otherwise lgtm http://codereview.chromium.org/10797040/diff/1/visual_studio/NativeClientVSAddIn/InstallerResources/xml_patch.py File visual_studio/NativeClientVSAddIn/InstallerResources/xml_patch.py (right): http://codereview.chromium.org/10797040/diff/1/visual_studio/NativeClientVSAddIn/InstallerResources/xml_patch.py#newcode162 visual_studio/NativeClientVSAddIn/InstallerResources/xml_patch.py:162: return tag[-8:] == 'PatchAdd' ...
8 years, 5 months ago (2012-07-24 22:13:08 UTC) #5
noelallen1
Address Ben's comments, otherwise LGTM.
8 years, 5 months ago (2012-07-24 22:48:59 UTC) #6
tysand
8 years, 5 months ago (2012-07-24 23:19:48 UTC) #7
http://codereview.chromium.org/10797040/diff/2002/visual_studio/NativeClientV...
File
visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/Props/ppapi_general.xml.patch
(right):

http://codereview.chromium.org/10797040/diff/2002/visual_studio/NativeClientV...
visual_studio/NativeClientVSAddIn/InstallerResources/PPAPI_Patch/Props/ppapi_general.xml.patch:19:
<StringProperty Name="VSNaClSDKRoot" DisplayName="NaCl SDK Root"
This variable refers to the visual studio project's idea of where the SDK root
directory is.  It defaults to the NACL_SDK_ROOT environment variable but can be
changed on a per-project basis.
On 2012/07/24 22:13:08, binji wrote:
> seems weird to call it VSNaClSDKRoot... what does it have to do with Visual
> Studio?

http://codereview.chromium.org/10797040/diff/2002/visual_studio/NativeClientV...
File
visual_studio/NativeClientVSAddIn/InstallerResources/create_ppapi_platform.py
(right):

http://codereview.chromium.org/10797040/diff/2002/visual_studio/NativeClientV...
visual_studio/NativeClientVSAddIn/InstallerResources/create_ppapi_platform.py:14:
import pdb
On 2012/07/24 22:13:08, binji wrote:
> remove debugging code

Done.

http://codereview.chromium.org/10797040/diff/2002/visual_studio/NativeClientV...
visual_studio/NativeClientVSAddIn/InstallerResources/create_ppapi_platform.py:114:
if tag[:1] == "{":
Changed this to tag.startswith("{") following your other comment's advice.
On 2012/07/24 22:13:08, binji wrote:
> nice trick, tag[:1] is the same as tag[0] but it works if tag is empty... may
> want to mention it.

Powered by Google App Engine
This is Rietveld 408576698