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

Issue 2261043003: Files app: Concatenate background scripts. (Closed)

Created:
4 years, 4 months ago by fukino
Modified:
4 years, 4 months ago
Reviewers:
oka, Dan Beam
CC:
chromium-reviews, posciak+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Files app: Concatenate background scripts. Concatenate scripts to: - Reduce HTTP requests in background script. - Save resource IDs. - Make it easier to divide JS files into smaller ones. The size of file_manager_resources.pak increases by 24 KB (4,187 KB => 4,211 KB), but I think it worth doing. BUG=639225, 636289 TEST=run browser_tests Committed: https://crrev.com/d19f48706428300449ca4aed64b764bd23c69da7 Cr-Commit-Position: refs/heads/master@{#413677}

Patch Set 1 #

Total comments: 8

Patch Set 2 : Address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -149 lines) Patch
A ui/file_manager/audio_player/js/background_scripts.js View 1 1 chunk +8 lines, -0 lines 0 comments Download
M ui/file_manager/audio_player/manifest.json View 1 chunk +2 lines, -15 lines 0 comments Download
A ui/file_manager/file_manager/background/js/background_common_scripts.js View 1 chunk +23 lines, -0 lines 0 comments Download
A ui/file_manager/file_manager/background/js/background_scripts.js View 1 chunk +22 lines, -0 lines 0 comments Download
M ui/file_manager/file_manager/manifest.json View 3 chunks +5 lines, -44 lines 0 comments Download
M ui/file_manager/file_manager_resources.grd View 7 chunks +6 lines, -47 lines 0 comments Download
A + ui/file_manager/gallery/js/background_scripts.js View 1 1 chunk +2 lines, -1 line 0 comments Download
M ui/file_manager/gallery/manifest.json View 1 1 chunk +2 lines, -14 lines 0 comments Download
A ui/file_manager/image_loader/background_scripts.js View 1 1 chunk +16 lines, -0 lines 0 comments Download
M ui/file_manager/image_loader/manifest.json View 1 chunk +2 lines, -13 lines 0 comments Download
A ui/file_manager/video_player/js/background_scripts.js View 1 1 chunk +8 lines, -0 lines 0 comments Download
M ui/file_manager/video_player/manifest.json View 1 chunk +2 lines, -15 lines 0 comments Download

Messages

Total messages: 17 (9 generated)
fukino
PTAL. Thanks!
4 years, 4 months ago (2016-08-22 08:40:29 UTC) #3
oka
lgtm LGTM with nits. https://codereview.chromium.org/2261043003/diff/1/ui/file_manager/audio_player/js/background_scripts.js File ui/file_manager/audio_player/js/background_scripts.js (right): https://codereview.chromium.org/2261043003/diff/1/ui/file_manager/audio_player/js/background_scripts.js#newcode1 ui/file_manager/audio_player/js/background_scripts.js:1: // Copyright 2016 The Chromium ...
4 years, 4 months ago (2016-08-23 02:06:12 UTC) #6
fukino
Thanks! https://codereview.chromium.org/2261043003/diff/1/ui/file_manager/audio_player/js/background_scripts.js File ui/file_manager/audio_player/js/background_scripts.js (right): https://codereview.chromium.org/2261043003/diff/1/ui/file_manager/audio_player/js/background_scripts.js#newcode1 ui/file_manager/audio_player/js/background_scripts.js:1: // Copyright 2016 The Chromium Authors. All rights ...
4 years, 4 months ago (2016-08-23 05:21:08 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2261043003/20001
4 years, 4 months ago (2016-08-23 05:21:39 UTC) #10
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 4 months ago (2016-08-23 06:09:59 UTC) #12
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/d19f48706428300449ca4aed64b764bd23c69da7 Cr-Commit-Position: refs/heads/master@{#413677}
4 years, 4 months ago (2016-08-23 06:13:06 UTC) #14
Dan Beam
note: vulcanize is a much better way to handle this, IMO
4 years, 4 months ago (2016-08-24 00:01:30 UTC) #16
fukino
4 years, 4 months ago (2016-08-24 02:20:41 UTC) #17
Message was sent while issue was closed.
On 2016/08/24 00:01:30, Dan Beam wrote:
> note: vulcanize is a much better way to handle this, IMO

Thank you for the suggestion!
For now we don't use vulcanize as we are concerned about following points and
grit looks simpler unless we don't use HTML imports heavily.
- We need to run vulcanize every time we modify our code (At least, when we
commit the change or run tests)
- We need to commit the generated script as well as modified scripts to the git
repository.

We're going to re-consider vulcanize when the overhead of HTML imports gets
bigger or we get a simpler build process.

Powered by Google App Engine
This is Rietveld 408576698