Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
821653002:
Add compiled_resources.gyp for metadata_dispatcher.js. (Closed)
|
Created:
6 years ago by yawano Modified:
6 years ago CC:
chromium-reviews, vitalyp+closure_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, dbeam+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd compiled_resources.gyp for metadata_dispatcher.js.
BUG=444510
TEST=GYP_GENERATORS=ninja tools/gyp/gyp --depth . ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp && ninja -C out/Default
Committed: https://crrev.com/da9bd8926d343f6e374a4ca584287c5060122ccc
Cr-Commit-Position: refs/heads/master@{#309614}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed an empty extern. #
Messages
Total messages: 17 (3 generated)
yawano@chromium.org changed reviewers: + fukino@chromium.org
PTAL. Thank you!
https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp (right): https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp:1: # Copyright 2014 The Chromium Authors. All rights reserved. I think you can add this definition into file_manager/foreground/js/compiled_resources.js, like: /// (snip) definition for target_name: main { 'target_name': 'metadata/metadata_dispatcher', 'variables': { 'depends': [ 'metadata/metadata_parser.js', 'metadata/byte_reader.js', 'metadata/exif_parser.js', 'metadata/image_parsers.js', 'metadata/mpeg_parser.js', 'metadata/id3_parser.js', '../../common/js/util.js', ], 'externs': [], }, 'includes': [ '../../../../../third_party/closure_compiler/compile_js.gypi' ], }
On 2014/12/22 10:34:40, fukino wrote: > https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... > File ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp > (right): > > https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... > ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp:1: # > Copyright 2014 The Chromium Authors. All rights reserved. > I think you can add this definition into > file_manager/foreground/js/compiled_resources.js, like: > > /// (snip) definition for target_name: main > { > 'target_name': 'metadata/metadata_dispatcher', > 'variables': { > 'depends': [ > 'metadata/metadata_parser.js', > 'metadata/byte_reader.js', > 'metadata/exif_parser.js', > 'metadata/image_parsers.js', > 'metadata/mpeg_parser.js', > 'metadata/id3_parser.js', > '../../common/js/util.js', > ], > 'externs': [], > }, > 'includes': [ > '../../../../../third_party/closure_compiler/compile_js.gypi' > ], > } But it's OK to add metadata/compiled_resources.gyp and merge it to file_manager's later.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp (right): https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/12/22 10:34:40, fukino wrote: > I think you can add this definition into > file_manager/foreground/js/compiled_resources.js, like: > > /// (snip) definition for target_name: main > { > 'target_name': 'metadata/metadata_dispatcher', > 'variables': { > 'depends': [ > 'metadata/metadata_parser.js', > 'metadata/byte_reader.js', > 'metadata/exif_parser.js', > 'metadata/image_parsers.js', > 'metadata/mpeg_parser.js', > 'metadata/id3_parser.js', > '../../common/js/util.js', > ], > 'externs': [], > }, > 'includes': [ > '../../../../../third_party/closure_compiler/compile_js.gypi' > ], > } why is this better? we'd like to keep compiled_resources.gyp files as close as possible to their source code (so put it in metadata/ imo). i should write a rule to enforce this in gyp.
lgtm fwiw (wait for fukino@ as well, though) https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp (right): https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp:18: 'externs': [], this isn't necessary as an empty list is already the default
On 2014/12/22 19:50:29, Dan Beam wrote: > lgtm fwiw (wait for fukino@ as well, though) > > https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... > File ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp > (right): > > https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... > ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp:18: > 'externs': [], > this isn't necessary as an empty list is already the default Sorry for this late reply. While I haven't explained it, I was going to move this to file_manager/foreground/js/compiled_resources.gyp later. The reason that I haven't put this at file_manager/foreground/js/compiled_resources.gyp is that the gyp file is compiled by a buildbot (http://crrev.com/777563002), and I thought putting this in the file breaks the buildbot. Fixing all errors of metadata_dispatcher.js related codes seems to take a few CLs. The reason that I think it's better to put this in file_manager/foreground/js/compiled_resources.gyp later is that metadata_dispatcher.js is a part of file_manager/file_manager while all of them are put under metadata/. But if it's better to put a gyp file as close as to source files, I'm going to put them at metadata/compiled_resources.gyp.
Removed an empty extern. @fukino: PTAL. Thank you. https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp (right): https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp:18: 'externs': [], On 2014/12/22 19:50:29, Dan Beam wrote: > this isn't necessary as an empty list is already the default Done.
On 2014/12/22 19:49:39, Dan Beam wrote: > https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... > File ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp > (right): > > https://codereview.chromium.org/821653002/diff/1/ui/file_manager/file_manager... > ui/file_manager/file_manager/foreground/js/metadata/compiled_resources.gyp:1: # > Copyright 2014 The Chromium Authors. All rights reserved. > On 2014/12/22 10:34:40, fukino wrote: > > I think you can add this definition into > > file_manager/foreground/js/compiled_resources.js, like: > > > > /// (snip) definition for target_name: main > > { > > 'target_name': 'metadata/metadata_dispatcher', > > 'variables': { > > 'depends': [ > > 'metadata/metadata_parser.js', > > 'metadata/byte_reader.js', > > 'metadata/exif_parser.js', > > 'metadata/image_parsers.js', > > 'metadata/mpeg_parser.js', > > 'metadata/id3_parser.js', > > '../../common/js/util.js', > > ], > > 'externs': [], > > }, > > 'includes': [ > > '../../../../../third_party/closure_compiler/compile_js.gypi' > > ], > > } > > why is this better? we'd like to keep compiled_resources.gyp files as close as > possible to their source code (so put it in metadata/ imo). > > i should write a rule to enforce this in gyp. metadata_dispacher is only for file_manager/foreground, so I thought it was better to keep the compiled_resources.gyp as single file for maintenance reason. But putting .gyp as close as the target source also makes sense. We'll put the .gyp file under metadata.
lgtm.
On 2014/12/24 02:45:52, fukino wrote: > lgtm. Thank you! P.S. To make things clear, metadata_dispatcher.js is also included from Gallery app through metadata_worker.js.
The CQ bit was checked by yawano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821653002/20001
On 2014/12/24 03:29:56, yawano wrote: > On 2014/12/24 02:45:52, fukino wrote: > > lgtm. > > Thank you! > > P.S. To make things clear, metadata_dispatcher.js is also included from Gallery > app through metadata_worker.js. Ah, then it makes more sense to put compiled_resources.gyp separately. Thank you for pointing it out.
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/da9bd8926d343f6e374a4ca584287c5060122ccc Cr-Commit-Position: refs/heads/master@{#309614} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
