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

Issue 179223003: Adding app_shim executable on Windows. (Closed)

Created:
6 years, 10 months ago by Matt Giuca
Modified:
6 years, 1 month ago
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, chromium-apps-reviews_chromium.org, robertshield
Visibility:
Public.

Description

Adding app_shim executable on Windows. This is a small binary that runs Chrome with the given command-line arguments. It is intended for use with Chrome apps, so that file types can be associated with the shim, having a custom name (and in some cases, icon) rather than simply the name of the Chrome binary. BUG=130455 Committed: https://crrev.com/31d809c90a47afe763ea6a027d811eae1707914f Cr-Commit-Position: refs/heads/master@{#302749}

Patch Set 1 #

Patch Set 2 : Rebase. #

Patch Set 3 : Added icon for win_app_shim (currently just the webstore ico). #

Patch Set 4 : Rebase (the shim directory was moved). #

Patch Set 5 : Move webstore.ico to a more sensible location. #

Patch Set 6 : Cleanup and copyright notice. #

Total comments: 5

Patch Set 7 : Moved webstore.ico into win subdirectory. #

Total comments: 15

Patch Set 8 : Rebase. #

Patch Set 9 : Renamed win_app_shim.exe to app_shim_win.exe. #

Patch Set 10 : Automatically get the Chrome binary from the registry instead of taking it on the command line. #

Total comments: 15

Patch Set 11 : Added manifest, moved and renamed files and targets. #

Patch Set 12 : Respond to grt review. #

Total comments: 2

Patch Set 13 : Alphabetical order. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+115 lines, -6 lines) Patch
A chrome/app/theme/chromium/win/webstore.ico View 1 2 3 4 5 6 Binary file 0 comments Download
M chrome/app_shim/DEPS View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
A chrome/app_shim/app_shim_win.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +32 lines, -0 lines 0 comments Download
A chrome/app_shim/win/app_shim.rc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
A + chrome/app_shim/win/app_shim.exe.manifest View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -6 lines 0 comments Download
A chrome/app_shim/win/app_shim_main.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +80 lines, -0 lines 0 comments Download
M chrome/chrome.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 39 (15 generated)
Matt Giuca
Rationale is in the comment at the top of app_shim_win.cc. jackhou: chrome/app_shim/* oshima: chrome/app/theme/.../webstore.ico Note ...
6 years, 2 months ago (2014-10-24 09:45:46 UTC) #3
oshima
c/a/t lgtm
6 years, 2 months ago (2014-10-24 10:26:37 UTC) #4
jackhou1
https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc File chrome/app_shim/app_shim_win.cc (right): https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc#newcode23 chrome/app_shim/app_shim_win.cc:23: // This program takes a full Windows command line ...
6 years, 1 month ago (2014-10-26 22:56:09 UTC) #5
jackhou1
Oops, sorry, I missed your earlier comments. https://codereview.chromium.org/179223003/diff/130001/chrome/app_shim/app_shim_win.cc File chrome/app_shim/app_shim_win.cc (right): https://codereview.chromium.org/179223003/diff/130001/chrome/app_shim/app_shim_win.cc#newcode49 chrome/app_shim/app_shim_win.cc:49: // somehow, ...
6 years, 1 month ago (2014-10-26 23:16:43 UTC) #6
Matt Giuca
+security: Can someone from security team look at this? In the comments attached to this ...
6 years, 1 month ago (2014-10-27 00:16:16 UTC) #8
Will Harris
https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc File chrome/app_shim/app_shim_win.cc (right): https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc#newcode23 chrome/app_shim/app_shim_win.cc:23: // This program takes a full Windows command line ...
6 years, 1 month ago (2014-10-27 16:09:31 UTC) #10
robertshield
https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc File chrome/app_shim/app_shim_win.cc (right): https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc#newcode23 chrome/app_shim/app_shim_win.cc:23: // This program takes a full Windows command line ...
6 years, 1 month ago (2014-10-27 17:30:50 UTC) #12
Matt Giuca
PTAL. https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc File chrome/app_shim/app_shim_win.cc (right): https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc#newcode23 chrome/app_shim/app_shim_win.cc:23: // This program takes a full Windows command ...
6 years, 1 month ago (2014-10-30 06:39:05 UTC) #14
jackhou1
LGTM You'll need an owner for chrome.gyp
6 years, 1 month ago (2014-10-30 11:33:21 UTC) #15
grt (UTC plus 2)
https://codereview.chromium.org/179223003/diff/220001/chrome/app_shim/app_shim_win.cc File chrome/app_shim/app_shim_win.cc (right): https://codereview.chromium.org/179223003/diff/220001/chrome/app_shim/app_shim_win.cc#newcode13 chrome/app_shim/app_shim_win.cc:13: #include <windows.h> move above line 5 as per http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Order_of_Includes. ...
6 years, 1 month ago (2014-10-30 17:04:56 UTC) #16
Matt Giuca
I'm not at a computer until Monday so I will reply to the rest of ...
6 years, 1 month ago (2014-10-30 21:16:35 UTC) #17
grt (UTC plus 2)
https://codereview.chromium.org/179223003/diff/220001/chrome/app_shim/app_shim_win.gypi File chrome/app_shim/app_shim_win.gypi (right): https://codereview.chromium.org/179223003/diff/220001/chrome/app_shim/app_shim_win.gypi#newcode10 chrome/app_shim/app_shim_win.gypi:10: 'target_name': 'app_shim_win', On 2014/10/30 21:16:35, Matt Giuca wrote: > ...
6 years, 1 month ago (2014-10-31 14:29:59 UTC) #18
robertshield
lgtm https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc File chrome/app_shim/app_shim_win.cc (right): https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.cc#newcode23 chrome/app_shim/app_shim_win.cc:23: // This program takes a full Windows command ...
6 years, 1 month ago (2014-10-31 16:58:30 UTC) #19
Matt Giuca
jhawkins: Please review chrome.gyp. https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.gypi File chrome/app_shim/app_shim_win.gypi (right): https://codereview.chromium.org/179223003/diff/140001/chrome/app_shim/app_shim_win.gypi#newcode22 chrome/app_shim/app_shim_win.gypi:22: 'msvs_settings': { OK, I've added ...
6 years, 1 month ago (2014-11-04 04:38:18 UTC) #23
grt (UTC plus 2)
lgtm https://codereview.chromium.org/179223003/diff/220001/chrome/app_shim/app_shim_win.gypi File chrome/app_shim/app_shim_win.gypi (right): https://codereview.chromium.org/179223003/diff/220001/chrome/app_shim/app_shim_win.gypi#newcode10 chrome/app_shim/app_shim_win.gypi:10: 'target_name': 'app_shim_win', On 2014/11/04 04:38:17, Matt Giuca wrote: ...
6 years, 1 month ago (2014-11-04 14:04:53 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/179223003/300001
6 years, 1 month ago (2014-11-04 23:01:06 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/22141)
6 years, 1 month ago (2014-11-04 23:06:58 UTC) #28
Lei Zhang
chrome.gyp lgtm with a nit: https://codereview.chromium.org/179223003/diff/300001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/179223003/diff/300001/chrome/chrome.gyp#newcode515 chrome/chrome.gyp:515: 'app_shim/app_shim_win.gypi', nit: alphabetical order ...
6 years, 1 month ago (2014-11-04 23:23:44 UTC) #30
Matt Giuca
https://codereview.chromium.org/179223003/diff/300001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/179223003/diff/300001/chrome/chrome.gyp#newcode515 chrome/chrome.gyp:515: 'app_shim/app_shim_win.gypi', On 2014/11/04 23:23:44, Lei Zhang wrote: > nit: ...
6 years, 1 month ago (2014-11-05 00:12:31 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/179223003/320001
6 years, 1 month ago (2014-11-05 00:14:00 UTC) #33
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel/builds/8642)
6 years, 1 month ago (2014-11-05 01:27:04 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/179223003/320001
6 years, 1 month ago (2014-11-05 01:32:52 UTC) #37
commit-bot: I haz the power
Committed patchset #13 (id:320001)
6 years, 1 month ago (2014-11-05 03:05:19 UTC) #38
commit-bot: I haz the power
6 years, 1 month ago (2014-11-05 03:09:28 UTC) #39
Message was sent while issue was closed.
Patchset 13 (id:??) landed as
https://crrev.com/31d809c90a47afe763ea6a027d811eae1707914f
Cr-Commit-Position: refs/heads/master@{#302749}

Powered by Google App Engine
This is Rietveld 408576698