Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1800303006:
Fix the path of shortcuts with an icon in the current install dir. (Closed)
|
Created:
4 years, 9 months ago by fdoray Modified:
4 years, 9 months ago CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org, grt (UTC plus 2) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the path of shortcuts with an icon in the current install dir.
BUG=595374
Committed: https://crrev.com/04228a0b711fde48b78491825b13268b851b303b
Cr-Commit-Position: refs/heads/master@{#382318}
Patch Set 1 #
Total comments: 6
Patch Set 2 : CR gab #3 #Patch Set 3 : self review #Patch Set 4 : self review #Patch Set 5 : self review #
Total comments: 14
Patch Set 6 : CR gab #5 #Patch Set 7 : fix buildbot error #Patch Set 8 : fix buildbot error #Patch Set 9 : fix buildbot error #Patch Set 10 : fix buildbot error #
Total comments: 1
Patch Set 11 : fix buildbot error #Patch Set 12 : Use ProgramCompare #Patch Set 13 : fix nit #
Total comments: 1
Patch Set 14 : fix buildbot error #
Total comments: 4
Patch Set 15 : Canary only #Patch Set 16 : add tests #
Total comments: 2
Patch Set 17 : fix buildbot error #
Total comments: 4
Messages
Total messages: 51 (22 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org
gab@: Can you review this CL? Thanks.
lg, comments below. We should make sure we get coverage for multiple installs (Canary + user-level Chrome and Canary + system-level Chrome) via installer_test before M51 is cut to Beta in April. https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:302: !old_target_dir.IsParent(shortcut_properties.icon)) { How about we do path comparison everywhere instead of string comparison to be safer? i.e. 1) IsParent() instead of StartsWith() and 2) EndsWith() on BaseName() (and adjust API nomenclature as appropriate) I guess this is a comment about the existing code, but might as well be pedantic about making it extra correct given the unintended side-effects to date. https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:302: !old_target_dir.IsParent(shortcut_properties.icon)) { I had a hard time reading this logic, I'd say add a comment above that spells out in English what this does, something like: // Skip shortcuts whose target isn't rooted at |old_target_dir| and whose base // doesn't end in |old_target_path_suffix|. Except for shortcuts whose icon is // rooted at |old_target_dir|. // TODO(fdoray): The second condition is only intended to fix Canary shortcuts // broken by crbug.com/595374, remove it in May 2016. https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install_unittest.cc:470: TEST_F(InstallShortcutTest, UpdatePerUserShortcutsIcon) { Since all of these tests essentially more or less do the same thing. How about collapsing them into one and expanding the TestCase struct to have |target|, |icon|, |new_target|, |should_update| Then you can break down in sections with vertical space + comments but at least the test logic isn't duplicated 4 times in subtly different ways. That will also allow us to easily test when target is a system-level install to make sure that also works (system-level installs will have shortcuts pointing to the system-level chrome as well as to User Data profiles shortcuts).
gab@: Can you take another look? Thanks. https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:302: !old_target_dir.IsParent(shortcut_properties.icon)) { On 2016/03/17 20:14:23, gab wrote: > I had a hard time reading this logic, I'd say add a comment above that spells > out in English what this does, something like: > > // Skip shortcuts whose target isn't rooted at |old_target_dir| and whose base > // doesn't end in |old_target_path_suffix|. Except for shortcuts whose icon is > // rooted at |old_target_dir|. > // TODO(fdoray): The second condition is only intended to fix Canary shortcuts > // broken by crbug.com/595374, remove it in May 2016. Done. https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:302: !old_target_dir.IsParent(shortcut_properties.icon)) { On 2016/03/17 20:14:23, gab wrote: > How about we do path comparison everywhere instead of string comparison to be > safer? > > i.e. > 1) IsParent() instead of StartsWith() and > 2) EndsWith() on BaseName() > > (and adjust API nomenclature as appropriate) > > I guess this is a comment about the existing code, but might as well be pedantic > about making it extra correct given the unintended side-effects to date. Done. https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1800303006/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install_unittest.cc:470: TEST_F(InstallShortcutTest, UpdatePerUserShortcutsIcon) { On 2016/03/17 20:14:23, gab wrote: > Since all of these tests essentially more or less do the same thing. > > How about collapsing them into one and expanding the TestCase struct to have > |target|, |icon|, |new_target|, |should_update| > > Then you can break down in sections with vertical space + comments but at least > the test logic isn't duplicated 4 times in subtly different ways. > > That will also allow us to easily test when target is a system-level install to > make sure that also works (system-level installs will have shortcuts pointing to > the system-level chrome as well as to User Data profiles shortcuts). Done.
This is great :-), lgtm % a few test nits https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:82: const base::FilePath::CharType* icon_path; Comments on each member's meaning (relative path, etc.) and whether it's optional (can be null) https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:193: // path of these shortcuts to |new_target_path| using |new_target_path_relative| https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:210: file.WriteAtCurrentPos(kDummyData, arraysize(kDummyData))); Wrap lines 204-210 in if (!base::PathExists(target_path)) {} https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:533: TEST_F(InstallShortcutTest, UpdatePerUserShortcuts) { UpdatePerUserChromeShortcuts https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:535: // Target in the Chrome Canary install directory. No icon. s/Target/Shortcut target/ (same below) (there are multiple "targets" in this test so being specific is easier to parse) https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:562: Also add a section pointing to system-level Chrome the set of test cases (here and in the other tests). And also add a new test whose target is system-level Chrome. https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:596: TEST_F(InstallShortcutTest, UpdatePerUserShortcutsCanary) { UpdatePerUserCanaryShortcuts
https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:82: const base::FilePath::CharType* icon_path; On 2016/03/18 16:21:24, gab wrote: > Comments on each member's meaning (relative path, etc.) and whether it's > optional (can be null) Done. https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:193: // path of these shortcuts to |new_target_path| using On 2016/03/18 16:21:24, gab wrote: > |new_target_path_relative| Done. https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:210: file.WriteAtCurrentPos(kDummyData, arraysize(kDummyData))); On 2016/03/18 16:21:25, gab wrote: > Wrap lines 204-210 in > if (!base::PathExists(target_path)) {} Done. https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:533: TEST_F(InstallShortcutTest, UpdatePerUserShortcuts) { On 2016/03/18 16:21:25, gab wrote: > UpdatePerUserChromeShortcuts Done. https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:535: // Target in the Chrome Canary install directory. No icon. On 2016/03/18 16:21:25, gab wrote: > s/Target/Shortcut target/ (same below) > > (there are multiple "targets" in this test so being specific is easier to parse) Done. https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:562: On 2016/03/18 16:21:24, gab wrote: > Also add a section pointing to system-level Chrome the set of test cases (here > and in the other tests). > > And also add a new test whose target is system-level Chrome. Done. https://codereview.chromium.org/1800303006/diff/80001/chrome/installer/setup/... chrome/installer/setup/install_unittest.cc:596: TEST_F(InstallShortcutTest, UpdatePerUserShortcutsCanary) { On 2016/03/18 16:21:25, gab wrote: > UpdatePerUserCanaryShortcuts Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1800303006/#ps20002 (title: "CR gab #5")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800303006/20002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800303006/20002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800303006/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800303006/130001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800303006/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800303006/170001
gab@chromium.org changed reviewers: + grt@chromium.org
Would have preferred something like base::NormalizeFilePath(), to ::GetFullPathName(), but the former doesn't work on directories :-( so I guess ::GetFullPathName() is the best option. I'm kind of at lost when it comes to what is bullet-proof. I'll say lgtm w/ nit below for this CL but I'd like grt@ to give his opinion here in a follow-up (or I guess you can ask on our internal windows list since he's perf-OOO FWIU). https://codereview.chromium.org/1800303006/diff/170001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1800303006/diff/170001/chrome/installer/setup... chrome/installer/setup/install.cc:283: old_target_dir_full_path_buffer, nullptr) == 0) { ::GetFullPathName() also returns the size of the buffer required if it's not big enough so the only valid return valid is == MAX_PATH
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800303006/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800303006/190001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800303006/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800303006/210001
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800303006/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800303006/230001
thakis@chromium.org changed reviewers: + thakis@chromium.org
I think this (failing) try job belongs to this patch: https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang_x64_db... Not sure why this job doesn't show up on rietveld (I'll investigate), but please fix that before landing.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800303006/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800303006/250001
https://codereview.chromium.org/1800303006/diff/230001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1800303006/diff/230001/chrome/installer/setup... chrome/installer/setup/install.cc:327: shortcut_properties.target, num_old_target_dir_components)) && Truncating to specific number of components could also be wrong right? i.e. two paths can be the same without having the same number of components? I guess it works to compare short vs long paths though so maybe it's okay if we don't care about path traversals ("../") and empty components ("foo//bar/baz"). Explain in comment if so.
On 2016/03/21 14:46:25, Nico wrote: > I think this (failing) try job belongs to this patch: > https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang_x64_db... > > Not sure why this job doesn't show up on rietveld (I'll investigate), (sounds like "experimental" try bots don't show up by default unless one turns that on in rietveld settings. also sounds like it's not possible to do a gradual rollout of a new try bot without marking it experimental -- so I added that bot to this CL manually. Looks like that file built fine after your fix, thanks!)
Two more comments on ongoing patch sets. https://codereview.chromium.org/1800303006/diff/250001/chrome/installer/util/... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/1800303006/diff/250001/chrome/installer/util/... chrome/installer/util/install_util.h:193: ProgramCompare(const base::FilePath& path_to_match, Add tests for new form. https://codereview.chromium.org/1800303006/diff/250001/chrome/installer/util/... chrome/installer/util/install_util.h:194: bool support_directories); Prefer enum to bool (bool makes callsites obscure). For example base::StartsWith() and friends used to take a bool to indicate desire for case-sensitivity and have recently been switched to an enum for clarity when reading callsite: https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... Something like: enum class ComparisonType { // Evaluation compares existing files. COMPARE_FILE, // Evaluation compares existing files or directories. COMPARE_FILE_OR_DIRECTORY, } // Constructs a ProgramCompare with COMPARE_FILE as ComparisonType. explicit ProgramCompare(const base::FilePath& path_to_match); (...)
https://codereview.chromium.org/1800303006/diff/250001/chrome/installer/util/... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/1800303006/diff/250001/chrome/installer/util/... chrome/installer/util/install_util.h:193: ProgramCompare(const base::FilePath& path_to_match, On 2016/03/21 15:27:04, gab wrote: > Add tests for new form. Done. https://codereview.chromium.org/1800303006/diff/250001/chrome/installer/util/... chrome/installer/util/install_util.h:194: bool support_directories); On 2016/03/21 15:27:04, gab wrote: > Prefer enum to bool (bool makes callsites obscure). > For example base::StartsWith() and friends used to take a bool to indicate > desire for case-sensitivity and have recently been switched to an enum for > clarity when reading callsite: > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... > > > Something like: > > enum class ComparisonType { > // Evaluation compares existing files. > COMPARE_FILE, > // Evaluation compares existing files or directories. > COMPARE_FILE_OR_DIRECTORY, > } > > // Constructs a ProgramCompare with COMPARE_FILE as ComparisonType. > explicit ProgramCompare(const base::FilePath& path_to_match); > (...) Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1800303006/#ps290001 (title: "add tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800303006/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800303006/290001
Another try failure: https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang_x64_db... https://codereview.chromium.org/1800303006/diff/290001/chrome/installer/util/... File chrome/installer/util/install_util_unittest.cc (right): https://codereview.chromium.org/1800303006/diff/290001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:465: static const char data[] = "data"; ../../chrome/installer/util/install_util_unittest.cc(465,21) : error: unused variable 'data' [-Werror,-Wunused-variable] static const char data[] = "data"; ^
The CQ bit was unchecked by thakis@chromium.org
https://codereview.chromium.org/1800303006/diff/290001/chrome/installer/util/... File chrome/installer/util/install_util_unittest.cc (right): https://codereview.chromium.org/1800303006/diff/290001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:465: static const char data[] = "data"; On 2016/03/21 16:22:22, Nico wrote: > ../../chrome/installer/util/install_util_unittest.cc(465,21) : error: unused > variable 'data' [-Werror,-Wunused-variable] > static const char data[] = "data"; > ^ Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1800303006/#ps310001 (title: "fix buildbot error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800303006/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800303006/310001
Message was sent while issue was closed.
Committed patchset #17 (id:310001)
Message was sent while issue was closed.
Description was changed from ========== Fix the path of shortcuts with an icon in the current install dir. BUG=595374 ========== to ========== Fix the path of shortcuts with an icon in the current install dir. BUG=595374 Committed: https://crrev.com/04228a0b711fde48b78491825b13268b851b303b Cr-Commit-Position: refs/heads/master@{#382318} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/04228a0b711fde48b78491825b13268b851b303b Cr-Commit-Position: refs/heads/master@{#382318}
Message was sent while issue was closed.
Post-commit comments (a follow-up CL is fine) https://codereview.chromium.org/1800303006/diff/310001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1800303006/diff/310001/chrome/installer/setup... chrome/installer/setup/install.cc:323: continue; Nice :-), I'd even add a NOTREACHED(); with a comment: // ResolveShortcutProperties isn't expected to return paths with relative components, but if it does, the algorithm below is invalid. https://codereview.chromium.org/1800303006/diff/310001/chrome/installer/util/... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/1800303006/diff/310001/chrome/installer/util/... chrome/installer/util/install_util.h:189: // the same file. This class' functionality is no longer specific to a "program" I guess... Changing the class' name would probably be a pain, but this header should at least be updated. https://codereview.chromium.org/1800303006/diff/310001/chrome/installer/util/... File chrome/installer/util/install_util_unittest.cc (right): https://codereview.chromium.org/1800303006/diff/310001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:456: TEST_F(InstallUtilTest, ProgramCompareWithDirectories) { Should also verify that all other file tests pass just as well when ComparisonType is FILE_OR_DIRECTORY. https://codereview.chromium.org/1800303006/diff/310001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:480: std::wstring short_expect; base::string16 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
