Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24449006:
Add non_compiled_schema_files in api.gyp to disable schema model code. (Closed)
|
Created:
7 years, 2 months ago by Haojian Wu Modified:
7 years, 2 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAllow to disable API schema(JSON and IDL) to generate model code.
BUG=294203
TEST=compiled
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230478
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : fix an error #Patch Set 4 : upload again #
Total comments: 4
Patch Set 5 : Update #Patch Set 6 : Update #
Total comments: 6
Patch Set 7 : Address comments #Patch Set 8 : Add all non-compiled api definition files. #Patch Set 9 : Add all non-compiled API files. #Patch Set 10 : #Patch Set 11 : upload #
Total comments: 6
Patch Set 12 : Fix chromeos build error #
Messages
Total messages: 21 (0 generated)
Please review it. Thanks.
I think a better way to do this might be at the gyp level; see api.gyp - perhaps we should have 2 separate lists, one to compile both, and the other to compile just the bindings code.
On 2013/09/26 16:19:33, kalman wrote: > I think a better way to do this might be at the gyp level; see api.gyp - perhaps > we should have 2 separate lists, one to compile both, and the other to compile > just the bindings code. Sorry for the very delayed update. Done. I have greped the whole chromium code, I find there are few extension API has their own implementation(with "implemented_in" fields in compiler_options), most of them are using the model code generated code by schema compiler.
I'm going to send this to yoz, he's more familiar with this gyp stuff than me.
https://chromiumcodereview.appspot.com/24449006/diff/12001/build/json_schema_... File build/json_schema_bundle_compile.gypi (right): https://chromiumcodereview.appspot.com/24449006/diff/12001/build/json_schema_... build/json_schema_bundle_compile.gypi:46: '<@(schema_files_disable_compile)', This name is a little awkward. Perhaps non_compiled_schema_files https://chromiumcodereview.appspot.com/24449006/diff/12001/chrome/common/exte... File chrome/common/extensions/api/api.gyp (right): https://chromiumcodereview.appspot.com/24449006/diff/12001/chrome/common/exte... chrome/common/extensions/api/api.gyp:23: 'schema_files_disable_compile': [ There should be a comment here explaining how this is different from schema_files. And ideally, at least one schema file should be here.
https://codereview.chromium.org/24449006/diff/12001/build/json_schema_bundle_... File build/json_schema_bundle_compile.gypi (right): https://codereview.chromium.org/24449006/diff/12001/build/json_schema_bundle_... build/json_schema_bundle_compile.gypi:46: '<@(schema_files_disable_compile)', On 2013/10/16 21:07:44, Yoyo Zhou wrote: > This name is a little awkward. Perhaps > non_compiled_schema_files Done. https://codereview.chromium.org/24449006/diff/12001/chrome/common/extensions/... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/24449006/diff/12001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:23: 'schema_files_disable_compile': [ On 2013/10/16 21:07:44, Yoyo Zhou wrote: > There should be a comment here explaining how this is different from > schema_files. And ideally, at least one schema file should be here. Done. https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:26: 'infobars.json', I search the whole chromium code and find infobars API implementation doesn't use the generated model code. It can be disable the schema code generation at here.
https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:23: # Disable schema compiler to generate model extension API code. kalman, can you suggest improvements to this? I don't know exactly what it's for. https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:124: 'non_compiled_schema_files': [ Rather than putting this here in every conditional, you can put it under the 'variables' dictionary.
https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:23: # Disable schema compiler to generate model extension API code. On 2013/10/17 00:41:14, Yoyo Zhou wrote: > kalman, can you suggest improvements to this? I don't know exactly what it's > for. From what I understand, this will save the binary size since we don't compile unnecessary generated model code. https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:124: 'non_compiled_schema_files': [ On 2013/10/17 00:41:14, Yoyo Zhou wrote: > Rather than putting this here in every conditional, you can put it under the > 'variables' dictionary. Done.
https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:23: # Disable schema compiler to generate model extension API code. On 2013/10/17 12:29:06, Haojian Wu wrote: > On 2013/10/17 00:41:14, Yoyo Zhou wrote: > > kalman, can you suggest improvements to this? I don't know exactly what it's > > for. > > From what I understand, this will save the binary size since we don't compile > unnecessary generated model code. yes but there's way more than just infobars.
On 2013/10/17 14:21:49, kalman wrote: > https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... > File chrome/common/extensions/api/api.gyp (right): > > https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... > chrome/common/extensions/api/api.gyp:23: # Disable schema compiler to generate > model extension API code. > On 2013/10/17 12:29:06, Haojian Wu wrote: > > On 2013/10/17 00:41:14, Yoyo Zhou wrote: > > > kalman, can you suggest improvements to this? I don't know exactly what it's > > > for. > > > > From what I understand, this will save the binary size since we don't compile > > unnecessary generated model code. > > yes but there's way more than just infobars. Add more non-used API definition files. Are there any files still missing? By the way, the way I find non-used API generated model code is that I searched all JSON/IDL files with their own "implemented_in" field in "compiler_options" and make sure no code in chromium source use it.
LGTM
On 2013/10/18 12:18:48, Haojian Wu wrote: > On 2013/10/17 14:21:49, kalman wrote: > > > https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... > > File chrome/common/extensions/api/api.gyp (right): > > > > > https://codereview.chromium.org/24449006/diff/21001/chrome/common/extensions/... > > chrome/common/extensions/api/api.gyp:23: # Disable schema compiler to generate > > model extension API code. > > On 2013/10/17 12:29:06, Haojian Wu wrote: > > > On 2013/10/17 00:41:14, Yoyo Zhou wrote: > > > > kalman, can you suggest improvements to this? I don't know exactly what > it's > > > > for. > > > > > > From what I understand, this will save the binary size since we don't > compile > > > unnecessary generated model code. > > > > yes but there's way more than just infobars. > > Add more non-used API definition files. Are there any files still missing? > > By the way, the way I find non-used API generated model code is that I searched > all JSON/IDL files with their own > "implemented_in" field in "compiler_options" and make sure no code in chromium > source use it. "implemented_in" doesn't have anything to do with whether the actual generated model code is used. What I'd do here is to put all the files in the "don't compile model" list and then see where the compile failures happen.
On 2013/10/18 19:09:14, kalman wrote: > > "implemented_in" doesn't have anything to do with whether the actual generated > model code is used. > > What I'd do here is to put all the files in the "don't compile model" list and > then see where the compile failures happen. Done. I passed compilation on window7. Could you please use the try-bot to test this CL?
https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... chrome/common/extensions/api/api.gyp:28: 'enterprise_platform_keys_private.json', This one looks needed on chromeos. https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... chrome/common/extensions/api/api.gyp:30: 'file_browser_private.json', And this one. https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... chrome/common/extensions/api/api.gyp:33: 'input_ime.json', Also this one.
Thanks for the suggestions Done. I have passed the compilation with chromeos build on Linux. Could you please use the try-bot again to test it? Thanks. https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... chrome/common/extensions/api/api.gyp:28: 'enterprise_platform_keys_private.json', On 2013/10/21 17:20:34, Yoyo Zhou wrote: > This one looks needed on chromeos. Done. https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... chrome/common/extensions/api/api.gyp:30: 'file_browser_private.json', On 2013/10/21 17:20:34, Yoyo Zhou wrote: > And this one. Done. https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... chrome/common/extensions/api/api.gyp:33: 'input_ime.json', On 2013/10/21 17:20:34, Yoyo Zhou wrote: > Also this one. Done.
On 2013/10/22 00:54:57, Haojian Wu wrote: > Thanks for the suggestions > > Done. I have passed the compilation with chromeos build on Linux. > Could you please use the try-bot again to test it? Thanks. > > https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... > File chrome/common/extensions/api/api.gyp (right): > > https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... > chrome/common/extensions/api/api.gyp:28: > 'enterprise_platform_keys_private.json', > On 2013/10/21 17:20:34, Yoyo Zhou wrote: > > This one looks needed on chromeos. > > Done. > > https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... > chrome/common/extensions/api/api.gyp:30: 'file_browser_private.json', > On 2013/10/21 17:20:34, Yoyo Zhou wrote: > > And this one. > > Done. > > https://codereview.chromium.org/24449006/diff/150001/chrome/common/extensions... > chrome/common/extensions/api/api.gyp:33: 'input_ime.json', > On 2013/10/21 17:20:34, Yoyo Zhou wrote: > > Also this one. > > Done. ping kalman@.
lgtm. out of curiosity, what's the difference in file size (of built chrome) before and after?
On 2013/10/23 00:53:55, kalman wrote: > lgtm. out of curiosity, what's the difference in file size (of built chrome) > before and after? With release build, api.lib file size reduced from 52407KB to 50552KB, about 1.8MB.
On 2013/10/23 14:59:55, Haojian Wu wrote: > On 2013/10/23 00:53:55, kalman wrote: > > lgtm. out of curiosity, what's the difference in file size (of built chrome) > > before and after? > > With release build, api.lib file size reduced from 52407KB to 50552KB, about > 1.8MB. Wow, that's... impressive.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/24449006/230001
Message was sent while issue was closed.
Change committed as 230478 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
