Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259463002:
[Bindings] Specify if we aggregate partial interfaces by arguments (Closed)
DescriptionSpecify if we aggregate partial interfaces by an argument.
Before this CL, file names to include were decided with
the path of original IDL files.
This behavior could mislead us as if we included generated
files for partial interface and non-partial interface
in one aggregation file, although we aggregate files for
all partial interfaces into one file.
This change make it obvious in .gn/.gyp files to aggregate
files for (non-)partial interfaces.
BUG=634231
Committed: https://crrev.com/df0a0c5106ff224578092178373faca20bca2b02
Cr-Commit-Position: refs/heads/master@{#412756}
Patch Set 1 #
Messages
Total messages: 21 (7 generated)
Description was changed from
==========
Specify if we're aggregating patial interfaces or not by argument
BUG=634231
==========
to
==========
Specify if we aggregate partial interfaces by an argument.
This change make it obvious to handle V8Foo.{cpp|h} or V8BarPartial.{cpp|h}
in .gn/.gyp files.
BUG=634231
==========
peria@chromium.org changed reviewers: + bashi@chromium.org, yukishiino@chromium.org
PTL
I'm a bit confused. We're already generating V8NavigatorPartial.cpp, no? https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/...
On 2016/08/18 01:58:22, haraken wrote: > I'm a bit confused. We're already generating V8NavigatorPartial.cpp, no? > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/... The most important change in this CL is in aggregate_generated_bindings.py, and this script just creates aggregation files like V8GeneratedCoreBindings00.cpp. https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8G... This CL does not touch in compiling IDL files.
On 2016/08/18 02:20:06, peria wrote: > On 2016/08/18 01:58:22, haraken wrote: > > I'm a bit confused. We're already generating V8NavigatorPartial.cpp, no? > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/... > > The most important change in this CL is in aggregate_generated_bindings.py, > and this script just creates aggregation files like > V8GeneratedCoreBindings00.cpp. > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8G... > > This CL does not touch in compiling IDL files. Sorry, would you elaborate on what this CL is going to change? The CL description seems to be talking about each compiling IDL file but here you're talking about generated files...?
On 2016/08/18 02:26:48, haraken wrote: > On 2016/08/18 02:20:06, peria wrote: > > On 2016/08/18 01:58:22, haraken wrote: > > > I'm a bit confused. We're already generating V8NavigatorPartial.cpp, no? > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/... > > > > The most important change in this CL is in aggregate_generated_bindings.py, > > and this script just creates aggregation files like > > V8GeneratedCoreBindings00.cpp. > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8G... > > > > This CL does not touch in compiling IDL files. > > Sorry, would you elaborate on what this CL is going to change? > > The CL description seems to be talking about each compiling IDL file but here > you're talking about generated files...? Unfortunately, the current system of IDL compiling tools need IDL files at least twice. - to generate V8 binding files (e.g. V8NavigatorPartial.cpp) - to generate aggregation files (e.g. V8GeneratedCoreBindings00.cpp) I think you're considering the first case, but this CL works for the second case.
On 2016/08/18 02:42:28, peria wrote: > On 2016/08/18 02:26:48, haraken wrote: > > On 2016/08/18 02:20:06, peria wrote: > > > On 2016/08/18 01:58:22, haraken wrote: > > > > I'm a bit confused. We're already generating V8NavigatorPartial.cpp, no? > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/... > > > > > > The most important change in this CL is in aggregate_generated_bindings.py, > > > and this script just creates aggregation files like > > > V8GeneratedCoreBindings00.cpp. > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8G... > > > > > > This CL does not touch in compiling IDL files. > > > > Sorry, would you elaborate on what this CL is going to change? > > > > The CL description seems to be talking about each compiling IDL file but here > > you're talking about generated files...? > > Unfortunately, the current system of IDL compiling tools need IDL files at least > twice. > - to generate V8 binding files (e.g. V8NavigatorPartial.cpp) > - to generate aggregation files (e.g. V8GeneratedCoreBindings00.cpp) > > I think you're considering the first case, but this CL works for the second > case. Are you changing V8GeneratedCoreBindings00.cpp to V8GeneratedCoreBindings00Partial.cpp or something? Then why don't we need to change file names in gn files accordingly?
On 2016/08/18 02:45:03, haraken wrote: > On 2016/08/18 02:42:28, peria wrote: > > On 2016/08/18 02:26:48, haraken wrote: > > > On 2016/08/18 02:20:06, peria wrote: > > > > On 2016/08/18 01:58:22, haraken wrote: > > > > > I'm a bit confused. We're already generating V8NavigatorPartial.cpp, no? > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/... > > > > > > > > The most important change in this CL is in > aggregate_generated_bindings.py, > > > > and this script just creates aggregation files like > > > > V8GeneratedCoreBindings00.cpp. > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8G... > > > > > > > > This CL does not touch in compiling IDL files. > > > > > > Sorry, would you elaborate on what this CL is going to change? > > > > > > The CL description seems to be talking about each compiling IDL file but > here > > > you're talking about generated files...? > > > > Unfortunately, the current system of IDL compiling tools need IDL files at > least > > twice. > > - to generate V8 binding files (e.g. V8NavigatorPartial.cpp) > > - to generate aggregation files (e.g. V8GeneratedCoreBindings00.cpp) > > > > I think you're considering the first case, but this CL works for the second > > case. > > Are you changing V8GeneratedCoreBindings00.cpp to > V8GeneratedCoreBindings00Partial.cpp or something? Then why don't we need to > change file names in gn files accordingly? IIUC, peria-san isn't going to change any generated file names. He is trying to remove a hacky way to detect partial interfaces in aggregate_generated_bindings.py. I think this is a right way to go, so lgtm.
On 2016/08/18 02:45:03, haraken wrote: > On 2016/08/18 02:42:28, peria wrote: > > On 2016/08/18 02:26:48, haraken wrote: > > > On 2016/08/18 02:20:06, peria wrote: > > > > On 2016/08/18 01:58:22, haraken wrote: > > > > > I'm a bit confused. We're already generating V8NavigatorPartial.cpp, no? > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/... > > > > > > > > The most important change in this CL is in > aggregate_generated_bindings.py, > > > > and this script just creates aggregation files like > > > > V8GeneratedCoreBindings00.cpp. > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8G... > > > > > > > > This CL does not touch in compiling IDL files. > > > > > > Sorry, would you elaborate on what this CL is going to change? > > > > > > The CL description seems to be talking about each compiling IDL file but > here > > > you're talking about generated files...? > > > > Unfortunately, the current system of IDL compiling tools need IDL files at > least > > twice. > > - to generate V8 binding files (e.g. V8NavigatorPartial.cpp) > > - to generate aggregation files (e.g. V8GeneratedCoreBindings00.cpp) > > > > I think you're considering the first case, but this CL works for the second > > case. > > Are you changing V8GeneratedCoreBindings00.cpp to > V8GeneratedCoreBindings00Partial.cpp or something? Then why don't we need to > change file names in gn files accordingly? No. As you see, this script concatenates many #include's in aggregation files. We have to know the names of the binding files to be included there, and we're using IDL files to get the names. Before this CL, this script decided to include V8Interface.cpp or V8InterfacePartial.cpp based on the path of IDL files.
Description was changed from
==========
Specify if we aggregate partial interfaces by an argument.
This change make it obvious to handle V8Foo.{cpp|h} or V8BarPartial.{cpp|h}
in .gn/.gyp files.
BUG=634231
==========
to
==========
Specify if we aggregate partial interfaces by an argument.
Before this CL, file names to include were decided with
the path of original IDL files.
This behavior could mislead us as if we have included
generated files for partial interface and non-partial interface
in one aggregation file, although we aggregate files for
all partial interfaces into one file.
This change make it obvious in .gn/.gyp files to aggregate
files for (non-)partial interfaces.
BUG=634231
==========
On 2016/08/18 02:59:06, peria wrote: > On 2016/08/18 02:45:03, haraken wrote: > > On 2016/08/18 02:42:28, peria wrote: > > > On 2016/08/18 02:26:48, haraken wrote: > > > > On 2016/08/18 02:20:06, peria wrote: > > > > > On 2016/08/18 01:58:22, haraken wrote: > > > > > > I'm a bit confused. We're already generating V8NavigatorPartial.cpp, > no? > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/... > > > > > > > > > > The most important change in this CL is in > > aggregate_generated_bindings.py, > > > > > and this script just creates aggregation files like > > > > > V8GeneratedCoreBindings00.cpp. > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8G... > > > > > > > > > > This CL does not touch in compiling IDL files. > > > > > > > > Sorry, would you elaborate on what this CL is going to change? > > > > > > > > The CL description seems to be talking about each compiling IDL file but > > here > > > > you're talking about generated files...? > > > > > > Unfortunately, the current system of IDL compiling tools need IDL files at > > least > > > twice. > > > - to generate V8 binding files (e.g. V8NavigatorPartial.cpp) > > > - to generate aggregation files (e.g. V8GeneratedCoreBindings00.cpp) > > > > > > I think you're considering the first case, but this CL works for the second > > > case. > > > > Are you changing V8GeneratedCoreBindings00.cpp to > > V8GeneratedCoreBindings00Partial.cpp or something? Then why don't we need to > > change file names in gn files accordingly? > > No. > As you see, this script concatenates many #include's in aggregation files. > We have to know the names of the binding files to be included there, > and we're using IDL files to get the names. > > Before this CL, this script decided to include V8Interface.cpp or > V8InterfacePartial.cpp > based on the path of IDL files. updated the description.
Description was changed from ========== Specify if we aggregate partial interfaces by an argument. Before this CL, file names to include were decided with the path of original IDL files. This behavior could mislead us as if we have included generated files for partial interface and non-partial interface in one aggregation file, although we aggregate files for all partial interfaces into one file. This change make it obvious in .gn/.gyp files to aggregate files for (non-)partial interfaces. BUG=634231 ========== to ========== Specify if we aggregate partial interfaces by an argument. Before this CL, file names to include were decided with the path of original IDL files. This behavior could mislead us as if we included generated files for partial interface and non-partial interface in one aggregation file, although we aggregate files for all partial interfaces into one file. This change make it obvious in .gn/.gyp files to aggregate files for (non-)partial interfaces. BUG=634231 ==========
On 2016/08/18 03:12:57, peria wrote: > On 2016/08/18 02:59:06, peria wrote: > > On 2016/08/18 02:45:03, haraken wrote: > > > On 2016/08/18 02:42:28, peria wrote: > > > > On 2016/08/18 02:26:48, haraken wrote: > > > > > On 2016/08/18 02:20:06, peria wrote: > > > > > > On 2016/08/18 01:58:22, haraken wrote: > > > > > > > I'm a bit confused. We're already generating V8NavigatorPartial.cpp, > > no? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/... > > > > > > > > > > > > The most important change in this CL is in > > > aggregate_generated_bindings.py, > > > > > > and this script just creates aggregation files like > > > > > > V8GeneratedCoreBindings00.cpp. > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8G... > > > > > > > > > > > > This CL does not touch in compiling IDL files. > > > > > > > > > > Sorry, would you elaborate on what this CL is going to change? > > > > > > > > > > The CL description seems to be talking about each compiling IDL file but > > > here > > > > > you're talking about generated files...? > > > > > > > > Unfortunately, the current system of IDL compiling tools need IDL files at > > > least > > > > twice. > > > > - to generate V8 binding files (e.g. V8NavigatorPartial.cpp) > > > > - to generate aggregation files (e.g. V8GeneratedCoreBindings00.cpp) > > > > > > > > I think you're considering the first case, but this CL works for the > second > > > > case. > > > > > > Are you changing V8GeneratedCoreBindings00.cpp to > > > V8GeneratedCoreBindings00Partial.cpp or something? Then why don't we need to > > > change file names in gn files accordingly? > > > > No. > > As you see, this script concatenates many #include's in aggregation files. > > We have to know the names of the binding files to be included there, > > and we're using IDL files to get the names. > > > > Before this CL, this script decided to include V8Interface.cpp or > > V8InterfacePartial.cpp > > based on the path of IDL files. > > updated the description. ah, makes sense. LGTM.
The CQ bit was checked by peria@chromium.org
On 2016/08/18 03:54:04, haraken wrote: > On 2016/08/18 03:12:57, peria wrote: > > On 2016/08/18 02:59:06, peria wrote: > > > On 2016/08/18 02:45:03, haraken wrote: > > > > On 2016/08/18 02:42:28, peria wrote: > > > > > On 2016/08/18 02:26:48, haraken wrote: > > > > > > On 2016/08/18 02:20:06, peria wrote: > > > > > > > On 2016/08/18 01:58:22, haraken wrote: > > > > > > > > I'm a bit confused. We're already generating > V8NavigatorPartial.cpp, > > > no? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/... > > > > > > > > > > > > > > The most important change in this CL is in > > > > aggregate_generated_bindings.py, > > > > > > > and this script just creates aggregation files like > > > > > > > V8GeneratedCoreBindings00.cpp. > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8G... > > > > > > > > > > > > > > This CL does not touch in compiling IDL files. > > > > > > > > > > > > Sorry, would you elaborate on what this CL is going to change? > > > > > > > > > > > > The CL description seems to be talking about each compiling IDL file > but > > > > here > > > > > > you're talking about generated files...? > > > > > > > > > > Unfortunately, the current system of IDL compiling tools need IDL files > at > > > > least > > > > > twice. > > > > > - to generate V8 binding files (e.g. V8NavigatorPartial.cpp) > > > > > - to generate aggregation files (e.g. V8GeneratedCoreBindings00.cpp) > > > > > > > > > > I think you're considering the first case, but this CL works for the > > second > > > > > case. > > > > > > > > Are you changing V8GeneratedCoreBindings00.cpp to > > > > V8GeneratedCoreBindings00Partial.cpp or something? Then why don't we need > to > > > > change file names in gn files accordingly? > > > > > > No. > > > As you see, this script concatenates many #include's in aggregation files. > > > We have to know the names of the binding files to be included there, > > > and we're using IDL files to get the names. > > > > > > Before this CL, this script decided to include V8Interface.cpp or > > > V8InterfacePartial.cpp > > > based on the path of IDL files. > > > > updated the description. > > ah, makes sense. LGTM. Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Specify if we aggregate partial interfaces by an argument. Before this CL, file names to include were decided with the path of original IDL files. This behavior could mislead us as if we included generated files for partial interface and non-partial interface in one aggregation file, although we aggregate files for all partial interfaces into one file. This change make it obvious in .gn/.gyp files to aggregate files for (non-)partial interfaces. BUG=634231 ========== to ========== Specify if we aggregate partial interfaces by an argument. Before this CL, file names to include were decided with the path of original IDL files. This behavior could mislead us as if we included generated files for partial interface and non-partial interface in one aggregation file, although we aggregate files for all partial interfaces into one file. This change make it obvious in .gn/.gyp files to aggregate files for (non-)partial interfaces. BUG=634231 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Specify if we aggregate partial interfaces by an argument. Before this CL, file names to include were decided with the path of original IDL files. This behavior could mislead us as if we included generated files for partial interface and non-partial interface in one aggregation file, although we aggregate files for all partial interfaces into one file. This change make it obvious in .gn/.gyp files to aggregate files for (non-)partial interfaces. BUG=634231 ========== to ========== Specify if we aggregate partial interfaces by an argument. Before this CL, file names to include were decided with the path of original IDL files. This behavior could mislead us as if we included generated files for partial interface and non-partial interface in one aggregation file, although we aggregate files for all partial interfaces into one file. This change make it obvious in .gn/.gyp files to aggregate files for (non-)partial interfaces. BUG=634231 Committed: https://crrev.com/df0a0c5106ff224578092178373faca20bca2b02 Cr-Commit-Position: refs/heads/master@{#412756} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/df0a0c5106ff224578092178373faca20bca2b02 Cr-Commit-Position: refs/heads/master@{#412756} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
