Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2759803002:
Layout tests: Enable fetching generated files from the build directory (Closed)
|
Created:
3 years, 9 months ago by yzshen1 Modified:
3 years, 9 months ago Reviewers:
Dirk Pranke, jochen (gone - plz use gerrit), qyearsley, dcheng, Ken Rockot(use gerrit already) CC:
Aaron Boodman, abarth-chromium, blink-reviews, chromium-reviews, darin (slow to review), darin-cc_chromium.org, einbinder+watch-test-runner_chromium.org, jam, jochen+watch_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, Peter Beverloo, qsr+mojo_chromium.org, tfarina, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLayout tests: Enable fetching generated files from the build directory.
This CL allows URLs that start with "file:///gen/" to be mapped to the files in "<build_dir>/gen/".
One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.)
BUG=699569
TBR=jam@chromium.org
(jam@ delegated to other reviewers)
Review-Url: https://codereview.chromium.org/2759803002
Cr-Commit-Position: refs/heads/master@{#459641}
Committed: https://chromium.googlesource.com/chromium/src/+/628771d2f70705a9aba44a5f994dafdac9fd61ea
Patch Set 1 #Patch Set 2 : sync & merge #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 6
Patch Set 5 : . #
Dependent Patchsets: Messages
Total messages: 53 (25 generated)
The CQ bit was checked by yzshen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== [Not for commit] new JS module loading. BUG= ========== to ========== Enable fetching generated files in the output directory from the build directory. This CL allows URLs that start with "/gen/" to be mapped to the files in "<build_dir>/gen/". One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.) BUG=699569 ==========
yzshen@chromium.org changed reviewers: + mkwst@chromium.org
The CQ bit was checked by yzshen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yzshen@chromium.org changed reviewers: + jam@chromium.org, qyearsley@chromium.org - mkwst@chromium.org
Hi, John and Quinten. Would you please take a look? Thanks! Quinten: main reviewer. John: high-level idea and OWNER review.
Description was changed from ========== Enable fetching generated files in the output directory from the build directory. This CL allows URLs that start with "/gen/" to be mapped to the files in "<build_dir>/gen/". One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.) BUG=699569 ========== to ========== Layout tests: Enable fetching generated files in the output directory from the build directory. This CL allows URLs that start with "/gen/" to be mapped to the files in "<build_dir>/gen/". One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.) BUG=699569 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yzshen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jam@chromium.org changed reviewers: + jochen@chromium.org - jam@chromium.org
I defer to Jochen
I'm actually not very familiar with this code, so I have a couple questions :-) My main concern with this is that if someone else wants to do this in the future they might not know that it's possible, and if someone comes reads /gen/ in a test it may not be clear what this means. Do you think there's anywhere in the documentation (https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_...) where it makes sense to mention this, maybe a short section at the end? https://codereview.chromium.org/2759803002/diff/60001/content/shell/browser/l... File content/shell/browser/layout_test/blink_test_controller.cc (right): https://codereview.chromium.org/2759803002/diff/60001/content/shell/browser/l... content/shell/browser/layout_test/blink_test_controller.cc:83: } Would it be possible to reuse the function in chromium/src/content/shell/browser/layout_test/layout_test_browser_context.cc? Or is there another place where it makes sense to put this function? https://codereview.chromium.org/2759803002/diff/60001/content/shell/common/la... File content/shell/common/layout_test.mojom (right): https://codereview.chromium.org/2759803002/diff/60001/content/shell/common/la... content/shell/common/layout_test.mojom:20: In general, what does this file control? Is ShellTestConfiguration something that is always set up when running content_shell? https://codereview.chromium.org/2759803002/diff/60001/content/shell/renderer/... File content/shell/renderer/layout_test/blink_test_runner.cc (right): https://codereview.chromium.org/2759803002/diff/60001/content/shell/renderer/... content/shell/renderer/layout_test/blink_test_runner.cc:376: base::FilePath replace_path = There might be another slightly better name replace_path; Would `generated_file_path` make sense? (At this point it's not yet the full replacement path, just the generated file path.)
yzshen@chromium.org changed reviewers: + dcheng@chromium.org
+Daniel for security review. This change to mojom only affects layout tests. https://codereview.chromium.org/2759803002/diff/60001/content/shell/browser/l... File content/shell/browser/layout_test/blink_test_controller.cc (right): https://codereview.chromium.org/2759803002/diff/60001/content/shell/browser/l... content/shell/browser/layout_test/blink_test_controller.cc:83: } On 2017/03/20 17:40:00, qyearsley wrote: > Would it be possible to reuse the function in > chromium/src/content/shell/browser/layout_test/layout_test_browser_context.cc? > > Or is there another place where it makes sense to put this function? Eventually the code path of AMD-style loading mojom.js will be removed. That will get rid of the copy in layout_test_browser_context.cc Therefore I feel it is unnecessary to try to refactor and share the code. https://codereview.chromium.org/2759803002/diff/60001/content/shell/common/la... File content/shell/common/layout_test.mojom (right): https://codereview.chromium.org/2759803002/diff/60001/content/shell/common/la... content/shell/common/layout_test.mojom:20: On 2017/03/20 17:40:00, qyearsley wrote: > In general, what does this file control? Is ShellTestConfiguration something > that is always set up when running content_shell? This is the layout test configuration information sending from the browser to the renderer process. It is only set up when running layout tests. https://codereview.chromium.org/2759803002/diff/60001/content/shell/renderer/... File content/shell/renderer/layout_test/blink_test_runner.cc (right): https://codereview.chromium.org/2759803002/diff/60001/content/shell/renderer/... content/shell/renderer/layout_test/blink_test_runner.cc:376: base::FilePath replace_path = On 2017/03/20 17:40:00, qyearsley wrote: > There might be another slightly better name replace_path; Would > `generated_file_path` make sense? (At this point it's not yet the full > replacement path, just the generated file path.) Done. I renamed it to "gen_directory_path".
Alright, makes sense; LGTM
The mojo change lgtm, but the feature as a whole feels fragile. To me, the right approach would be to package the generated mojom js alongside the content_shell test binary by copying them into a well-known location (inside the framework, if necessary). *shrug*
On 2017/03/21 06:40:07, dcheng wrote: > The mojo change lgtm, but the feature as a whole feels fragile. To me, the right > approach would be to package the generated mojom js alongside the content_shell > test binary by copying them into a well-known location (inside the framework, if > necessary). *shrug* For example, how we bundle the test plugin in Blink: https://cs.chromium.org/chromium/src/content/shell/BUILD.gn?rcl=fa791ce4bcf38...
jochen@chromium.org changed reviewers: + dpranke@chromium.org
I'd prefer if dpranke@ signs off
On 2017/03/21 06:40:43, dcheng wrote: > On 2017/03/21 06:40:07, dcheng wrote: > > The mojo change lgtm, but the feature as a whole feels fragile. To me, the > right > > approach would be to package the generated mojom js alongside the > content_shell > > test binary by copying them into a well-known location (inside the framework, > if > > necessary). *shrug* > > For example, how we bundle the test plugin in Blink: > https://cs.chromium.org/chromium/src/content/shell/BUILD.gn?rcl=fa791ce4bcf38... Thanks! I don't have a strong preference. As I mentioned in the CL description, we are already loading those generated mojom.js files from the output directory, but using a special JS function exposed to the testing environment for AMD-style module loading. At least, this CL doesn't make it more fragile.
Description was changed from ========== Layout tests: Enable fetching generated files in the output directory from the build directory. This CL allows URLs that start with "/gen/" to be mapped to the files in "<build_dir>/gen/". One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.) BUG=699569 ========== to ========== Layout tests: Enable fetching generated files in the output directory from the build directory. This CL allows URLs that start with "file:///gen/" to be mapped to the files in "<build_dir>/gen/". One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.) BUG=699569 ==========
dpranke@chromium.org changed reviewers: + rockot@chromium.org
+rockot my basic understanding is that right now the bindings are loaded via <script src="layout-tests-mojom:foo.mojom.js"> and you're suggesting to change it to <script src="file:///gen/foo.mojom.js">? Is that right? If so, two things ... First, $root_build_dir/gen is *large* (8000 files at ~300MB). You need a tiny subset of them, and we need to make sure whatever you need to reference is included in the isolates we use for swarming (this is analogous to the zip_build problem). But we don't want to include all of .../gen in swarming, and we don't want to accidentally include and depend improperly on other files in .../gen. So, at best, this should be like a .../gen/mojom subdirectory and "file:///mojom/foo.mojom.js" instead. Second, once you do that, it's not immediately obvious to me if this is better or worse than the layout-test-mojom: scheme now. I don't really care one way or another, and it looks like file:/// is less code, but I'll let you and rockot@ figure out what you want to do here first. Sound good?
On 2017/03/21 at 23:08:22, dpranke wrote: > +rockot > > my basic understanding is that right now the bindings are loaded via <script src="layout-tests-mojom:foo.mojom.js"> and you're suggesting to change it to <script src="file:///gen/foo.mojom.js">? > > Is that right? > > If so, two things ... > > First, $root_build_dir/gen is *large* (8000 files at ~300MB). You need a tiny subset of them, and we need to make sure whatever you need to reference is included in the isolates we use for swarming (this is analogous to the zip_build problem). But we don't want to include all of .../gen in swarming, and we don't want to accidentally include and depend improperly on other files in .../gen. So, at best, this should be like a .../gen/mojom > subdirectory and "file:///mojom/foo.mojom.js" instead. This is already a requirement today since the layout-test-mojom scheme maps to gen/. This is why we've had so many bugs where someone introduces a new mojom JS dependency only to have isolated tests fail to find the file. I think it would be unfortunate to have generated mojom JS in gen/mojom/target/path and all other generated mojom C++/Java in gen/target/path. If we moved all generated sources wholesale into gen/mojom/target/path, we could introduce an implied public_config in all mojom targets which adds gen/mojom to the include path. That might be a reasonable solution. Then we could change this CL to file://mojom as you suggest. > > Second, once you do that, it's not immediately obvious to me if this is better or worse than the layout-test-mojom: scheme now. I don't really care one way or another, and it looks like file:/// is less code, but I'll let you and rockot@ figure out what you want to do here first. > I am quite pleased that file:/// is so much less code. I like less code. For that reason alone, I would prefer this approach. > Sound good?
On 2017/03/21 23:15:16, Ken Rockot wrote: > On 2017/03/21 at 23:08:22, dpranke wrote: > > +rockot > > > > my basic understanding is that right now the bindings are loaded via <script > src="layout-tests-mojom:foo.mojom.js"> and you're suggesting to change it to > <script src="file:///gen/foo.mojom.js">? > > > > Is that right? > > > > If so, two things ... > > > > First, $root_build_dir/gen is *large* (8000 files at ~300MB). You need a tiny > subset of them, and we need to make sure whatever you need to reference is > included in the isolates we use for swarming (this is analogous to the zip_build > problem). But we don't want to include all of .../gen in swarming, and we don't > want to accidentally include and depend improperly on other files in .../gen. > So, at best, this should be like a .../gen/mojom > > subdirectory and "file:///mojom/foo.mojom.js" instead. > > This is already a requirement today since the layout-test-mojom scheme maps to > gen/. This is why we've had so many bugs where someone introduces a new mojom JS > dependency only to have isolated tests fail to find the file. Ah, then clearly we need to fix this. Where do we declare the data dependencies on the generated files in the BUILD.gn files? as part of the mojom targets? As a group on some content target? Nowhere? > > I think it would be unfortunate to have generated mojom JS in > gen/mojom/target/path and all other generated mojom C++/Java in gen/target/path. > > If we moved all generated sources wholesale into gen/mojom/target/path, we could > introduce an implied public_config in all mojom targets which adds gen/mojom to > the include path. That might be a reasonable solution. Then we could change this > CL to file://mojom as you suggest. That would be fine w/ me. It seems like collecting the generated mojom files in a single subtree might be a good idea generally.
On Tue, Mar 21, 2017 at 4:26 PM, <dpranke@chromium.org> wrote: > On 2017/03/21 23:15:16, Ken Rockot wrote: > > On 2017/03/21 at 23:08:22, dpranke wrote: > > > +rockot > > > > > > my basic understanding is that right now the bindings are loaded via > <script > > src="layout-tests-mojom:foo.mojom.js"> and you're suggesting to change > it to > > <script src="file:///gen/foo.mojom.js">? > > > > > > Is that right? > > > > > > If so, two things ... > > > > > > First, $root_build_dir/gen is *large* (8000 files at ~300MB). You need > a > tiny > > subset of them, and we need to make sure whatever you need to reference > is > > included in the isolates we use for swarming (this is analogous to the > zip_build > > problem). But we don't want to include all of .../gen in swarming, and we > don't > > want to accidentally include and depend improperly on other files in > .../gen. > > So, at best, this should be like a .../gen/mojom > > > subdirectory and "file:///mojom/foo.mojom.js" instead. > > > > This is already a requirement today since the layout-test-mojom scheme > maps to > > gen/. This is why we've had so many bugs where someone introduces a new > mojom > JS > > dependency only to have isolated tests fail to find the file. > > Ah, then clearly we need to fix this. Where do we declare the data > dependencies > on the generated files in the BUILD.gn files? as part of the mojom > targets? As a > group > on some content target? Nowhere? > They are currently implied by the mojom targets yes, but this is not even strictly accurate as there is no guarantee that the dependent target wants to load the generated JS at runtime - and in fact this is almost never the case. Sometimes you even use the JS, but in another way (e.g. packaged into resources at build time). Unfortunately I don't have any immediate better ideas for how to express that you depend on generated JS output at runtime. I guess the simpliest approach would be to make it a separate generated target you have to depend on, e.g. for mojom("foo") you depend on ":foo_js_runtime" or something. > > > > > > I think it would be unfortunate to have generated mojom JS in > > gen/mojom/target/path and all other generated mojom C++/Java in > gen/target/path. > > > > If we moved all generated sources wholesale into gen/mojom/target/path, > we > could > > introduce an implied public_config in all mojom targets which adds > gen/mojom > to > > the include path. That might be a reasonable solution. Then we could > change > this > > CL to file://mojom as you suggest. > > That would be fine w/ me. It seems like collecting the generated mojom > files in > a single > subtree might be a good idea generally. > It would certainly simplify that zip script if we could just slurp in gen/mojom/**.js. > > > https://codereview.chromium.org/2759803002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Mar 21, 2017 at 4:26 PM, <dpranke@chromium.org> wrote: > On 2017/03/21 23:15:16, Ken Rockot wrote: > > On 2017/03/21 at 23:08:22, dpranke wrote: > > > +rockot > > > > > > my basic understanding is that right now the bindings are loaded via > <script > > src="layout-tests-mojom:foo.mojom.js"> and you're suggesting to change > it to > > <script src="file:///gen/foo.mojom.js">? > > > > > > Is that right? > > > > > > If so, two things ... > > > > > > First, $root_build_dir/gen is *large* (8000 files at ~300MB). You need > a > tiny > > subset of them, and we need to make sure whatever you need to reference > is > > included in the isolates we use for swarming (this is analogous to the > zip_build > > problem). But we don't want to include all of .../gen in swarming, and we > don't > > want to accidentally include and depend improperly on other files in > .../gen. > > So, at best, this should be like a .../gen/mojom > > > subdirectory and "file:///mojom/foo.mojom.js" instead. > > > > This is already a requirement today since the layout-test-mojom scheme > maps to > > gen/. This is why we've had so many bugs where someone introduces a new > mojom > JS > > dependency only to have isolated tests fail to find the file. > > Ah, then clearly we need to fix this. Where do we declare the data > dependencies > on the generated files in the BUILD.gn files? as part of the mojom > targets? As a > group > on some content target? Nowhere? > They are currently implied by the mojom targets yes, but this is not even strictly accurate as there is no guarantee that the dependent target wants to load the generated JS at runtime - and in fact this is almost never the case. Sometimes you even use the JS, but in another way (e.g. packaged into resources at build time). Unfortunately I don't have any immediate better ideas for how to express that you depend on generated JS output at runtime. I guess the simpliest approach would be to make it a separate generated target you have to depend on, e.g. for mojom("foo") you depend on ":foo_js_runtime" or something. > > > > > > I think it would be unfortunate to have generated mojom JS in > > gen/mojom/target/path and all other generated mojom C++/Java in > gen/target/path. > > > > If we moved all generated sources wholesale into gen/mojom/target/path, > we > could > > introduce an implied public_config in all mojom targets which adds > gen/mojom > to > > the include path. That might be a reasonable solution. Then we could > change > this > > CL to file://mojom as you suggest. > > That would be fine w/ me. It seems like collecting the generated mojom > files in > a single > subtree might be a good idea generally. > It would certainly simplify that zip script if we could just slurp in gen/mojom/**.js. > > > https://codereview.chromium.org/2759803002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2017/03/21 23:39:56, Ken Rockot wrote:
> On Tue, Mar 21, 2017 at 4:26 PM, <mailto:dpranke@chromium.org> wrote:
>
> > On 2017/03/21 23:15:16, Ken Rockot wrote:
> > > On 2017/03/21 at 23:08:22, dpranke wrote:
> > > > +rockot
> > > >
> > > > my basic understanding is that right now the bindings are loaded via
> > <script
> > > src="layout-tests-mojom:foo.mojom.js"> and you're suggesting to change
> > it to
> > > <script src="file:///gen/foo.mojom.js">?
> > > >
> > > > Is that right?
> > > >
> > > > If so, two things ...
> > > >
> > > > First, $root_build_dir/gen is *large* (8000 files at ~300MB). You need
> > a
> > tiny
> > > subset of them, and we need to make sure whatever you need to reference
> > is
> > > included in the isolates we use for swarming (this is analogous to the
> > zip_build
> > > problem). But we don't want to include all of .../gen in swarming, and we
> > don't
> > > want to accidentally include and depend improperly on other files in
> > .../gen.
> > > So, at best, this should be like a .../gen/mojom
> > > > subdirectory and "file:///mojom/foo.mojom.js" instead.
> > >
> > > This is already a requirement today since the layout-test-mojom scheme
> > maps to
> > > gen/. This is why we've had so many bugs where someone introduces a new
> > mojom
> > JS
> > > dependency only to have isolated tests fail to find the file.
> >
> > Ah, then clearly we need to fix this. Where do we declare the data
> > dependencies
> > on the generated files in the BUILD.gn files? as part of the mojom
> > targets? As a
> > group
> > on some content target? Nowhere?
> >
>
> They are currently implied by the mojom targets yes, but this is not even
> strictly accurate as there is no guarantee that the dependent target wants
> to load the generated JS at runtime - and in fact this is almost never the
> case. Sometimes you even use the JS, but in another way (e.g. packaged into
> resources at build time).
>
> Unfortunately I don't have any immediate better ideas for how to express
> that you depend on generated JS output at runtime. I guess the simpliest
> approach would be to make it a separate generated target you have to depend
> on, e.g. for mojom("foo") you depend on ":foo_js_runtime" or something.
One way or another you need to get the layout tests targets to have data
dependencies on this file, or the isolates won't work. If there's not a good
way to know which JS files should be available directly from the build graph
(e.g., because you don't actually want content_shell to have the dependencies)
you could just directly declare a data dependency from the layout tests
onto gen/mojom.
> > That would be fine w/ me. It seems like collecting the generated mojom
> > files in
> > a single
> > subtree might be a good idea generally.
> >
>
> It would certainly simplify that zip script if we could just slurp in
> gen/mojom/**.js.
>
Right. Though we actually want the zip_build script to go away and we should be
able to dynamically figure out which things we need based on the correct data
dependencies from the targets we care about.
On Tue, Mar 21, 2017 at 4:47 PM, <dpranke@chromium.org> wrote: > On 2017/03/21 23:39:56, Ken Rockot wrote: > > > On Tue, Mar 21, 2017 at 4:26 PM, <mailto:dpranke@chromium.org> wrote: > > > > > On 2017/03/21 23:15:16, Ken Rockot wrote: > > > > On 2017/03/21 at 23:08:22, dpranke wrote: > > > > > +rockot > > > > > > > > > > my basic understanding is that right now the bindings are loaded > via > > > <script > > > > src="layout-tests-mojom:foo.mojom.js"> and you're suggesting to > change > > > it to > > > > <script src="file:///gen/foo.mojom.js">? > > > > > > > > > > Is that right? > > > > > > > > > > If so, two things ... > > > > > > > > > > First, $root_build_dir/gen is *large* (8000 files at ~300MB). You > need > > > a > > > tiny > > > > subset of them, and we need to make sure whatever you need to > reference > > > is > > > > included in the isolates we use for swarming (this is analogous to > the > > > zip_build > > > > problem). But we don't want to include all of .../gen in swarming, > and we > > > don't > > > > want to accidentally include and depend improperly on other files in > > > .../gen. > > > > So, at best, this should be like a .../gen/mojom > > > > > subdirectory and "file:///mojom/foo.mojom.js" instead. > > > > > > > > This is already a requirement today since the layout-test-mojom > scheme > > > maps to > > > > gen/. This is why we've had so many bugs where someone introduces a > new > > > mojom > > > JS > > > > dependency only to have isolated tests fail to find the file. > > > > > > Ah, then clearly we need to fix this. Where do we declare the data > > > dependencies > > > on the generated files in the BUILD.gn files? as part of the mojom > > > targets? As a > > > group > > > on some content target? Nowhere? > > > > > > > They are currently implied by the mojom targets yes, but this is not even > > strictly accurate as there is no guarantee that the dependent target > wants > > to load the generated JS at runtime - and in fact this is almost never > the > > case. Sometimes you even use the JS, but in another way (e.g. packaged > into > > resources at build time). > > > > Unfortunately I don't have any immediate better ideas for how to express > > that you depend on generated JS output at runtime. I guess the simpliest > > approach would be to make it a separate generated target you have to > depend > > on, e.g. for mojom("foo") you depend on ":foo_js_runtime" or something. > > One way or another you need to get the layout tests targets to have data > dependencies on this file, or the isolates won't work. If there's not a > good > way to know which JS files should be available directly from the build > graph > (e.g., because you don't actually want content_shell to have the > dependencies) > you could just directly declare a data dependency from the layout tests > onto gen/mojom. > > > > That would be fine w/ me. It seems like collecting the generated mojom > > > files in > > > a single > > > subtree might be a good idea generally. > > > > > > > It would certainly simplify that zip script if we could just slurp in > > gen/mojom/**.js. > > > > Right. Though we actually want the zip_build script to go away and we > should be > able to dynamically figure out which things we need based on the correct > data > dependencies from the targets we care about. > Makes sense. > > > https://codereview.chromium.org/2759803002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Tue, Mar 21, 2017 at 4:47 PM, <dpranke@chromium.org> wrote: > On 2017/03/21 23:39:56, Ken Rockot wrote: > > > On Tue, Mar 21, 2017 at 4:26 PM, <mailto:dpranke@chromium.org> wrote: > > > > > On 2017/03/21 23:15:16, Ken Rockot wrote: > > > > On 2017/03/21 at 23:08:22, dpranke wrote: > > > > > +rockot > > > > > > > > > > my basic understanding is that right now the bindings are loaded > via > > > <script > > > > src="layout-tests-mojom:foo.mojom.js"> and you're suggesting to > change > > > it to > > > > <script src="file:///gen/foo.mojom.js">? > > > > > > > > > > Is that right? > > > > > > > > > > If so, two things ... > > > > > > > > > > First, $root_build_dir/gen is *large* (8000 files at ~300MB). You > need > > > a > > > tiny > > > > subset of them, and we need to make sure whatever you need to > reference > > > is > > > > included in the isolates we use for swarming (this is analogous to > the > > > zip_build > > > > problem). But we don't want to include all of .../gen in swarming, > and we > > > don't > > > > want to accidentally include and depend improperly on other files in > > > .../gen. > > > > So, at best, this should be like a .../gen/mojom > > > > > subdirectory and "file:///mojom/foo.mojom.js" instead. > > > > > > > > This is already a requirement today since the layout-test-mojom > scheme > > > maps to > > > > gen/. This is why we've had so many bugs where someone introduces a > new > > > mojom > > > JS > > > > dependency only to have isolated tests fail to find the file. > > > > > > Ah, then clearly we need to fix this. Where do we declare the data > > > dependencies > > > on the generated files in the BUILD.gn files? as part of the mojom > > > targets? As a > > > group > > > on some content target? Nowhere? > > > > > > > They are currently implied by the mojom targets yes, but this is not even > > strictly accurate as there is no guarantee that the dependent target > wants > > to load the generated JS at runtime - and in fact this is almost never > the > > case. Sometimes you even use the JS, but in another way (e.g. packaged > into > > resources at build time). > > > > Unfortunately I don't have any immediate better ideas for how to express > > that you depend on generated JS output at runtime. I guess the simpliest > > approach would be to make it a separate generated target you have to > depend > > on, e.g. for mojom("foo") you depend on ":foo_js_runtime" or something. > > One way or another you need to get the layout tests targets to have data > dependencies on this file, or the isolates won't work. If there's not a > good > way to know which JS files should be available directly from the build > graph > (e.g., because you don't actually want content_shell to have the > dependencies) > you could just directly declare a data dependency from the layout tests > onto gen/mojom. > > > > That would be fine w/ me. It seems like collecting the generated mojom > > > files in > > > a single > > > subtree might be a good idea generally. > > > > > > > It would certainly simplify that zip script if we could just slurp in > > gen/mojom/**.js. > > > > Right. Though we actually want the zip_build script to go away and we > should be > able to dynamically figure out which things we need based on the correct > data > dependencies from the targets we care about. > Makes sense. > > > https://codereview.chromium.org/2759803002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry. I am late to the party. :) In addition to mojom.js files, another CL is also trying to use this same approach to load validation test cases for mojo. Those are not generated mojom files, so it doesn't make sense for them to be in gen/mojom/ https://codereview.chromium.org/2745293005/ It seems reasonable to me to have a general way to load files outside of the LayoutTests/ folder. (An alternative is to bundle them as resources in the binary. However, now that we don't bundle test.html, it seems a little weird to bundle those js files / data files that test.html refers to.) WDYT? On 2017/03/21 23:39:56, Ken Rockot wrote: > On Tue, Mar 21, 2017 at 4:26 PM, <mailto:dpranke@chromium.org> wrote: > > > On 2017/03/21 23:15:16, Ken Rockot wrote: > > > On 2017/03/21 at 23:08:22, dpranke wrote: > > > > +rockot > > > > > > > > my basic understanding is that right now the bindings are loaded via > > <script > > > src="layout-tests-mojom:foo.mojom.js"> and you're suggesting to change > > it to > > > <script src="file:///gen/foo.mojom.js">? > > > > > > > > Is that right? > > > > > > > > If so, two things ... > > > > > > > > First, $root_build_dir/gen is *large* (8000 files at ~300MB). You need > > a > > tiny > > > subset of them, and we need to make sure whatever you need to reference > > is > > > included in the isolates we use for swarming (this is analogous to the > > zip_build > > > problem). But we don't want to include all of .../gen in swarming, and we > > don't > > > want to accidentally include and depend improperly on other files in > > .../gen. > > > So, at best, this should be like a .../gen/mojom > > > > subdirectory and "file:///mojom/foo.mojom.js" instead. > > > > > > This is already a requirement today since the layout-test-mojom scheme > > maps to > > > gen/. This is why we've had so many bugs where someone introduces a new > > mojom > > JS > > > dependency only to have isolated tests fail to find the file. > > > > Ah, then clearly we need to fix this. Where do we declare the data > > dependencies > > on the generated files in the BUILD.gn files? as part of the mojom > > targets? As a > > group > > on some content target? Nowhere? > > > > They are currently implied by the mojom targets yes, but this is not even > strictly accurate as there is no guarantee that the dependent target wants > to load the generated JS at runtime - and in fact this is almost never the > case. Sometimes you even use the JS, but in another way (e.g. packaged into > resources at build time). > > Unfortunately I don't have any immediate better ideas for how to express > that you depend on generated JS output at runtime. I guess the simpliest > approach would be to make it a separate generated target you have to depend > on, e.g. for mojom("foo") you depend on ":foo_js_runtime" or something. > > > > > > > > > > > > I think it would be unfortunate to have generated mojom JS in > > > gen/mojom/target/path and all other generated mojom C++/Java in > > gen/target/path. > > > > > > If we moved all generated sources wholesale into gen/mojom/target/path, > > we > > could > > > introduce an implied public_config in all mojom targets which adds > > gen/mojom > > to > > > the include path. That might be a reasonable solution. Then we could > > change > > this > > > CL to file://mojom as you suggest. > > > > That would be fine w/ me. It seems like collecting the generated mojom > > files in > > a single > > subtree might be a good idea generally. > > > > It would certainly simplify that zip script if we could just slurp in > gen/mojom/**.js. > > > > > > > > https://codereview.chromium.org/2759803002/ > > > > -- > You received this message because you are subscribed to the Google Groups "Blink > Reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:blink-reviews+unsubscribe@chromium.org.
> >
> > They are currently implied by the mojom targets yes, but this is not even
> > strictly accurate as there is no guarantee that the dependent target wants
> > to load the generated JS at runtime - and in fact this is almost never the
> > case. Sometimes you even use the JS, but in another way (e.g. packaged into
> > resources at build time).
> >
> > Unfortunately I don't have any immediate better ideas for how to express
> > that you depend on generated JS output at runtime. I guess the simpliest
> > approach would be to make it a separate generated target you have to depend
> > on, e.g. for mojom("foo") you depend on ":foo_js_runtime" or something.
+1. That sounds reasonable to me.
On 2017/03/21 23:50:35, yzshen1 wrote: > It seems reasonable to me to have a general way to load files outside of the > LayoutTests/ folder. (An alternative is to bundle them as resources in the > binary. However, now that we don't bundle test.html, it seems a little weird to > bundle those js files / data files that test.html refers to.) Generally speaking, no, that's not reasonable :). LayoutTests/ is supposed to be self-contained. It is reasonable to change that in limited ways if we have no other sensible way to refer to files we need but they can't be checked in (like these generated files) but that is something we need to be careful about, because we don't want to allow people to specify arbitrary dependencies on arbitrary things all over the source or build trees. Plus, we need to make sure that any thing that we might actually need to run the layout tests is captured as a data dependency of the layout test target, so that we can isolate and run the layout tests properly under swarming (and be able to do things like make this zip_build go away). So as long as we come up with a scheme that requires at least some amount of caution and clarity around what is or isn't allowed to be loaded, I'm fine with whatever we do.
On 2017/03/22 00:14:07, Dirk Pranke wrote: > On 2017/03/21 23:50:35, yzshen1 wrote: > > It seems reasonable to me to have a general way to load files outside of the > > LayoutTests/ folder. (An alternative is to bundle them as resources in the > > binary. However, now that we don't bundle test.html, it seems a little weird > to > > bundle those js files / data files that test.html refers to.) > > Generally speaking, no, that's not reasonable :). LayoutTests/ is supposed to be > self-contained. > > It is reasonable to change that in limited ways if we have no other sensible way > to refer to files we need but they can't be checked in (like these generated > files) > but that is something we need to be careful about, because we don't want to > allow people to specify arbitrary dependencies on arbitrary things all over the > source or build trees. > > Plus, we need to make sure that any thing that we might actually need to run > the layout tests is captured as a data dependency of the layout test target, so > that we can isolate and run the layout tests properly under swarming (and > be able to do things like make this zip_build go away). > > So as long as we come up with a scheme that requires at least some amount > of caution and clarity around what is or isn't allowed to be loaded, I'm fine > with whatever we do. Thanks for the reply, Dirk! And I agree with your point. :) Just to clarify my previous mail a little bit: I didn't suggest to permissively allow arbitrary dependencies. I totally agree that we should have careful review and proper data deps rules to limit what LayoutTests/ depend on. What I was trying to say is: *mojom.js is probably not the only external data dependency. Therefore, instead of introducing the "layout-tests-mojom" scheme for this specific case, it seems reasonable to use a more generic scheme, such as the one proposed in this CL "file:///gen/". At least this new approach tells me the files are from the gen/ folder. :) Another use case (seems legitimate to me) is to use "file:///gen/" to load mojo validation test data, which is shared between C++/Java/JS tests and therefore doesn't live in LayoutTests/. Does this sound a little more reasonable? :)
On Mar 22, 2017 8:58 AM, <yzshen@chromium.org> wrote: On 2017/03/22 00:14:07, Dirk Pranke wrote: > On 2017/03/21 23:50:35, yzshen1 wrote: > > It seems reasonable to me to have a general way to load files outside of the > > LayoutTests/ folder. (An alternative is to bundle them as resources in the > > binary. However, now that we don't bundle test.html, it seems a little weird > to > > bundle those js files / data files that test.html refers to.) > > Generally speaking, no, that's not reasonable :). LayoutTests/ is supposed to be > self-contained. > > It is reasonable to change that in limited ways if we have no other sensible way > to refer to files we need but they can't be checked in (like these generated > files) > but that is something we need to be careful about, because we don't want to > allow people to specify arbitrary dependencies on arbitrary things all over the > source or build trees. > > Plus, we need to make sure that any thing that we might actually need to run > the layout tests is captured as a data dependency of the layout test target, so > that we can isolate and run the layout tests properly under swarming (and > be able to do things like make this zip_build go away). > > So as long as we come up with a scheme that requires at least some amount > of caution and clarity around what is or isn't allowed to be loaded, I'm fine > with whatever we do. Thanks for the reply, Dirk! And I agree with your point. :) Just to clarify my previous mail a little bit: I didn't suggest to permissively allow arbitrary dependencies. I totally agree that we should have careful review and proper data deps rules to limit what LayoutTests/ depend on. What I was trying to say is: *mojom.js is probably not the only external data dependency. Therefore, instead of introducing the "layout-tests-mojom" scheme for this specific case, it seems reasonable to use a more generic scheme, such as the one proposed in this CL "file:///gen/". At least this new approach tells me the files are from the gen/ folder. :) Another use case (seems legitimate to me) is to use "file:///gen/" to load mojo validation test data, which is shared between C++/Java/JS tests and therefore doesn't live in LayoutTests/. Does this sound a little more reasonable? :) And rather than changing the output path for mojom generation, maybe we could introduce a generic gen/layout_test_data path and use explicit GN copy rules to put required files there at build time. WDYT? https://codereview.chromium.org/2759803002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Mar 22, 2017 8:58 AM, <yzshen@chromium.org> wrote: On 2017/03/22 00:14:07, Dirk Pranke wrote: > On 2017/03/21 23:50:35, yzshen1 wrote: > > It seems reasonable to me to have a general way to load files outside of the > > LayoutTests/ folder. (An alternative is to bundle them as resources in the > > binary. However, now that we don't bundle test.html, it seems a little weird > to > > bundle those js files / data files that test.html refers to.) > > Generally speaking, no, that's not reasonable :). LayoutTests/ is supposed to be > self-contained. > > It is reasonable to change that in limited ways if we have no other sensible way > to refer to files we need but they can't be checked in (like these generated > files) > but that is something we need to be careful about, because we don't want to > allow people to specify arbitrary dependencies on arbitrary things all over the > source or build trees. > > Plus, we need to make sure that any thing that we might actually need to run > the layout tests is captured as a data dependency of the layout test target, so > that we can isolate and run the layout tests properly under swarming (and > be able to do things like make this zip_build go away). > > So as long as we come up with a scheme that requires at least some amount > of caution and clarity around what is or isn't allowed to be loaded, I'm fine > with whatever we do. Thanks for the reply, Dirk! And I agree with your point. :) Just to clarify my previous mail a little bit: I didn't suggest to permissively allow arbitrary dependencies. I totally agree that we should have careful review and proper data deps rules to limit what LayoutTests/ depend on. What I was trying to say is: *mojom.js is probably not the only external data dependency. Therefore, instead of introducing the "layout-tests-mojom" scheme for this specific case, it seems reasonable to use a more generic scheme, such as the one proposed in this CL "file:///gen/". At least this new approach tells me the files are from the gen/ folder. :) Another use case (seems legitimate to me) is to use "file:///gen/" to load mojo validation test data, which is shared between C++/Java/JS tests and therefore doesn't live in LayoutTests/. Does this sound a little more reasonable? :) And rather than changing the output path for mojom generation, maybe we could introduce a generic gen/layout_test_data path and use explicit GN copy rules to put required files there at build time. WDYT? https://codereview.chromium.org/2759803002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/22 16:17:45, Ken Rockot wrote: > On Mar 22, 2017 8:58 AM, <mailto:yzshen@chromium.org> wrote: > > On 2017/03/22 00:14:07, Dirk Pranke wrote: > > On 2017/03/21 23:50:35, yzshen1 wrote: > > > It seems reasonable to me to have a general way to load files outside > of the > > > LayoutTests/ folder. (An alternative is to bundle them as resources in > the > > > binary. However, now that we don't bundle test.html, it seems a little > weird > > to > > > bundle those js files / data files that test.html refers to.) > > > > Generally speaking, no, that's not reasonable :). LayoutTests/ is > supposed to > be > > self-contained. > > > > It is reasonable to change that in limited ways if we have no other > sensible > way > > to refer to files we need but they can't be checked in (like these > generated > > files) > > but that is something we need to be careful about, because we don't want > to > > allow people to specify arbitrary dependencies on arbitrary things all > over > the > > source or build trees. > > > > Plus, we need to make sure that any thing that we might actually need to > run > > the layout tests is captured as a data dependency of the layout test > target, > so > > that we can isolate and run the layout tests properly under swarming (and > > be able to do things like make this zip_build go away). > > > > So as long as we come up with a scheme that requires at least some amount > > of caution and clarity around what is or isn't allowed to be loaded, I'm > fine > > with whatever we do. > > Thanks for the reply, Dirk! And I agree with your point. :) > > Just to clarify my previous mail a little bit: I didn't suggest to > permissively > allow arbitrary dependencies. I totally agree that we should have careful > review > and proper data deps rules to limit what LayoutTests/ depend on. > > What I was trying to say is: *mojom.js is probably not the only external > data > dependency. Therefore, instead of introducing the "layout-tests-mojom" > scheme > for this specific case, it seems reasonable to use a more generic scheme, > such > as the one proposed in this CL "file:///gen/". At least this new approach > tells > me the files are from the gen/ folder. :) Another use case (seems > legitimate to > me) is to use "file:///gen/" to load mojo validation test data, which is > shared > between C++/Java/JS tests and therefore doesn't live in LayoutTests/. > > Does this sound a little more reasonable? :) > > > And rather than changing the output path for mojom generation, maybe we > could introduce a generic gen/layout_test_data path and use explicit GN > copy rules to put required files there at build time. WDYT? That sounds good to me. > > > > https://codereview.chromium.org/2759803002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2017/03/22 16:30:32, yzshen1 wrote: > On 2017/03/22 16:17:45, Ken Rockot wrote: > > On Mar 22, 2017 8:58 AM, <mailto:yzshen@chromium.org> wrote: > > > > On 2017/03/22 00:14:07, Dirk Pranke wrote: > > > On 2017/03/21 23:50:35, yzshen1 wrote: > > > > It seems reasonable to me to have a general way to load files outside > > of the > > > > LayoutTests/ folder. (An alternative is to bundle them as resources in > > the > > > > binary. However, now that we don't bundle test.html, it seems a little > > weird > > > to > > > > bundle those js files / data files that test.html refers to.) > > > > > > Generally speaking, no, that's not reasonable :). LayoutTests/ is > > supposed to > > be > > > self-contained. > > > > > > It is reasonable to change that in limited ways if we have no other > > sensible > > way > > > to refer to files we need but they can't be checked in (like these > > generated > > > files) > > > but that is something we need to be careful about, because we don't want > > to > > > allow people to specify arbitrary dependencies on arbitrary things all > > over > > the > > > source or build trees. > > > > > > Plus, we need to make sure that any thing that we might actually need to > > run > > > the layout tests is captured as a data dependency of the layout test > > target, > > so > > > that we can isolate and run the layout tests properly under swarming (and > > > be able to do things like make this zip_build go away). > > > > > > So as long as we come up with a scheme that requires at least some amount > > > of caution and clarity around what is or isn't allowed to be loaded, I'm > > fine > > > with whatever we do. > > > > Thanks for the reply, Dirk! And I agree with your point. :) > > > > Just to clarify my previous mail a little bit: I didn't suggest to > > permissively > > allow arbitrary dependencies. I totally agree that we should have careful > > review > > and proper data deps rules to limit what LayoutTests/ depend on. > > > > What I was trying to say is: *mojom.js is probably not the only external > > data > > dependency. Therefore, instead of introducing the "layout-tests-mojom" > > scheme > > for this specific case, it seems reasonable to use a more generic scheme, > > such > > as the one proposed in this CL "file:///gen/". At least this new approach > > tells > > me the files are from the gen/ folder. :) Another use case (seems > > legitimate to > > me) is to use "file:///gen/" to load mojo validation test data, which is > > shared > > between C++/Java/JS tests and therefore doesn't live in LayoutTests/. > > > > Does this sound a little more reasonable? :) > > > > > > And rather than changing the output path for mojom generation, maybe we > > could introduce a generic gen/layout_test_data path and use explicit GN > > copy rules to put required files there at build time. WDYT? > > That sounds good to me. One issue of copying generated mojom.js files to a new directory is that if mojom taget A depends on target B, we need to copy files from both target A and target B (and avoid duplicate copies if multiple targets depend on B). I chatted with Brett and it seems there is no good way to figure out the dependency graph and do the copy at the build time. Considering that, I would vote for *not* copying mojom.js files into a new directory. As long as we have data_deps defined, there shouldn't be any problem with running tests under swarming. Could I get this landed as-is? And I would be happy to make follow-up improvements if people have good ideas. Thanks! > > > > > > > > > https://codereview.chromium.org/2759803002/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
FWIW this LGTM as-is. Given the complexity of trying to express some duplicated subtree of gen/ in GN rules, it sounds like right path forward is to allow gen/ use and ensure that data_deps are correct. i.e. providing access to gen/ does not imply free reign to depend on arbitrary generated files without proper data_deps, and it does not require us to upload all of gen/ to bots.
Description was changed from ========== Layout tests: Enable fetching generated files in the output directory from the build directory. This CL allows URLs that start with "file:///gen/" to be mapped to the files in "<build_dir>/gen/". One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.) BUG=699569 ========== to ========== Layout tests: Enable fetching generated files from the build directory. This CL allows URLs that start with "file:///gen/" to be mapped to the files in "<build_dir>/gen/". One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.) BUG=699569 TBR=jam@chromium.org (jam@ delegated to other reviewers) ==========
The CQ bit was checked by yzshen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490421833479280,
"parent_rev": "0b2f864618b2342a6aa8bbb922bb9bb840fd1258", "commit_rev":
"628771d2f70705a9aba44a5f994dafdac9fd61ea"}
Message was sent while issue was closed.
Description was changed from ========== Layout tests: Enable fetching generated files from the build directory. This CL allows URLs that start with "file:///gen/" to be mapped to the files in "<build_dir>/gen/". One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.) BUG=699569 TBR=jam@chromium.org (jam@ delegated to other reviewers) ========== to ========== Layout tests: Enable fetching generated files from the build directory. This CL allows URLs that start with "file:///gen/" to be mapped to the files in "<build_dir>/gen/". One use case is to allow loading generated mojom.js files using <script> tag. (We are already loading those generated mojom.js files, but using a special JS function exposed to the testing environment for AMD-style module loading. This CL is a cleaner approach.) BUG=699569 TBR=jam@chromium.org (jam@ delegated to other reviewers) Review-Url: https://codereview.chromium.org/2759803002 Cr-Commit-Position: refs/heads/master@{#459641} Committed: https://chromium.googlesource.com/chromium/src/+/628771d2f70705a9aba44a5f994d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/628771d2f70705a9aba44a5f994d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
