Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655473002:
Handle spaces in the name of the mini_installer. (Closed)
|
Created:
3 years, 11 months ago by grt (UTC plus 2) Modified:
3 years, 10 months ago Reviewers:
bcwhite CC:
chromium-reviews, grt+watch_chromium.org, pennymac+watch_chromium.org, wfh+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle spaces in the name of the mini_installer.
BUG=682277
R=bcwhite@chromium.org
Review-Url: https://codereview.chromium.org/2655473002
Cr-Commit-Position: refs/heads/master@{#446694}
Committed: https://chromium.googlesource.com/chromium/src/+/bdf8ed5452e3b44255bed8f6fe609204b5b5ddf9
Patch Set 1 #
Total comments: 2
Patch Set 2 : rewrite with tests #
Total comments: 8
Patch Set 3 : better comments #
Depends on Patchset: Messages
Total messages: 29 (17 generated)
The CQ bit was checked by grt@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...
PTAL, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
ping!
https://codereview.chromium.org/2655473002/diff/1/chrome/installer/mini_insta... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/2655473002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:279: tmp = SearchStringI(tmp + ::lstrlen(arg0_base_name), L" "); If you know the length of the program name, why do you need to search for the next space character? No tests for this?
Thanks. https://codereview.chromium.org/2655473002/diff/1/chrome/installer/mini_insta... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/2655473002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:279: tmp = SearchStringI(tmp + ::lstrlen(arg0_base_name), L" "); On 2017/01/26 15:36:22, bcwhite wrote: > If you know the length of the program name, why do you need to search for the > next space character? I think it's possible that there could be a trailing double-quote. > No tests for this? None so far. Now is as good a time as any to add some. I'll send up another patch set with some.
lgtm
Patchset #2 (id:20001) has been deleted
Hi Brian. In adding the unittest, I decided to rewrite the function. It was overly convoluted with that whole "get the program name from elsewhere and scan for it" thing. I think it's more straightforward this way. PTAL.
The CQ bit was checked by grt@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.
https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:228: if (*command_line == L'"') { Would a single-quote also be valid here? https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:236: if (a_char == L'"') { Crazy though it may be, it's valid for a user to type something like this: "/path to chrome"/mini_installer.exe Though how something like that on the command line would get represented by |command_line|, I couldn't say. You could just fall through from here to the "else" section. https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.h (right): https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.h:30: using CommandString = StackString<MAX_PATH * 4>; Change to MAX_PATH * 2 + NNN to match the comment? https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.h:33: // |command_line| to |buffer|. |buffer| is unchanged in case of error. // The flags section is copied exactly: spaces, quotes and all.
The CQ bit was checked by grt@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...
Thanks, PTAL. https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:228: if (*command_line == L'"') { On 2017/01/27 14:52:24, bcwhite wrote: > Would a single-quote also be valid here? No. https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:236: if (a_char == L'"') { On 2017/01/27 14:52:23, bcwhite wrote: > Crazy though it may be, it's valid for a user to type something like this: > > "/path to chrome"/mini_installer.exe > > Though how something like that on the command line would get represented by > |command_line|, I couldn't say. The Win32 ::CommandLineToArgvW turns that into this pair of strings: /path to chrome /mini_installer.exe I've added a link to a blog post that explains gory details to the comment above. https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.h (right): https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.h:30: using CommandString = StackString<MAX_PATH * 4>; On 2017/01/27 14:52:24, bcwhite wrote: > Change to > > MAX_PATH * 2 + NNN > > to match the comment? If you mean "NNN" to be the room for extra arguments, it would be MAX_PATH * 3 + NNN (the executable to run is itself a path). I've beefed up the comment here. https://codereview.chromium.org/2655473002/diff/40001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.h:33: // |command_line| to |buffer|. |buffer| is unchanged in case of error. On 2017/01/27 14:52:24, bcwhite wrote: > // The flags section is copied exactly: spaces, quotes and all. Done.
lgtm
The CQ bit was unchecked by grt@chromium.org
The CQ bit was checked by grt@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2661463003 Patch 40001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by grt@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": 60001, "attempt_start_ts": 1485536324317950,
"parent_rev": "01f12135d93ddd4f3daf304f8a3b86c4cd7c6dbe", "commit_rev":
"bdf8ed5452e3b44255bed8f6fe609204b5b5ddf9"}
Message was sent while issue was closed.
Description was changed from ========== Handle spaces in the name of the mini_installer. BUG=682277 R=bcwhite@chromium.org ========== to ========== Handle spaces in the name of the mini_installer. BUG=682277 R=bcwhite@chromium.org Review-Url: https://codereview.chromium.org/2655473002 Cr-Commit-Position: refs/heads/master@{#446694} Committed: https://chromium.googlesource.com/chromium/src/+/bdf8ed5452e3b44255bed8f6fe60... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bdf8ed5452e3b44255bed8f6fe60... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
