|
|
Created:
4 years, 11 months ago by grt (UTC plus 2) Modified:
4 years, 11 months ago Reviewers:
gab CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for component builds in alternate_version_generator.
BUG=550983, 581133
Committed: https://crrev.com/4540853f2b2322700f27f184186c509e2b3e724f
Cr-Commit-Position: refs/heads/master@{#371398}
Patch Set 1 #
Total comments: 16
Patch Set 2 : gab comments #Patch Set 3 : string16 #
Messages
Total messages: 16 (8 generated)
grt@chromium.org changed reviewers: + gab@chromium.org
This seems to do the trick. PTAL.
Nice :-), lgtm with comments below. PS: How is this tested? Or is the test that the tests which depend on it work? What are those tests? https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... File chrome/installer/test/alternate_version_generator.cc (right): https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:291: // equals [|src_first|, |src_last) with the sequence at |replacement_first| of existing nit : Missing | after "last" in both instances above. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:292: // the same length. Returns true on success. If non-NULL, |replacements_made| Says |replacement_first| has to be the same length as |src_last - src_first| but nothing enforces that? https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:369: std::string old_version(ctx.current_version.ToASCII()); s/old_version/current_version/ for consistency https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:431: reinterpret_cast<uint8_t*>(&old_version[old_version.size()]), Would reinterpret_cast<uint8_t*>(old_version.end()) be valid? Feels using begin()/end() would be cleaner if possible..? https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:495: if (doing_great) { If we don't do anything and just run to returning false at end when we stop |doing_great|, why not instead return false on first failure and drop this variable? https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:503: .Append(ctx.current_version_str + L".manifest"); Please link this CL to http://crbug.com/581133 which I just filed so that we know there is potential cleanup here when we get rid of the version manifest. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:627: } DCHECK(!archive_resource_name.empty()); DCHECK(!chrome_packed_7z.empty() || !chrome_7z.empty(); DCHECK(archive_file); to document the expectations at this point. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:665: } DCHECK(!chrome_7z.empty()); to document the expectations at this point
Description was changed from ========== Add support for component builds in alternate_version_generator. BUG=550983 ========== to ========== Add support for component builds in alternate_version_generator. BUG=550983,581133 ==========
Thanks! As mentioned, this is un-tested, but I hope to use it in some integration tests soon. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... File chrome/installer/test/alternate_version_generator.cc (right): https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:291: // equals [|src_first|, |src_last) with the sequence at |replacement_first| of On 2016/01/25 21:00:12, gab wrote: > existing nit : Missing | after "last" in both instances above. Done. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:292: // the same length. Returns true on success. If non-NULL, |replacements_made| On 2016/01/25 21:00:12, gab wrote: > Says |replacement_first| has to be the same length as |src_last - src_first| but > nothing enforces that? Correct. This is consistent with various C++ library functions which take a pair of [begin,end) iterators for a source and a single output iterator for the destination of an operation. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:369: std::string old_version(ctx.current_version.ToASCII()); On 2016/01/25 21:00:12, gab wrote: > s/old_version/current_version/ for consistency Done. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:431: reinterpret_cast<uint8_t*>(&old_version[old_version.size()]), On 2016/01/25 21:00:12, gab wrote: > Would reinterpret_cast<uint8_t*>(old_version.end()) be valid? Feels using > begin()/end() would be cleaner if possible..? I don't think so. While the iterator must act like a pointer in that operator* would "dereference" it, the iterator itself is likely a lot more than just a pointer, so the typecast would lead to badness. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:495: if (doing_great) { On 2016/01/25 21:00:12, gab wrote: > If we don't do anything and just run to returning false at end when we stop > |doing_great|, why not instead return false on first failure and drop this > variable? Good point. I probably wrote this when I was allergic to early-exit. :-) https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:503: .Append(ctx.current_version_str + L".manifest"); On 2016/01/25 21:00:12, gab wrote: > Please link this CL to http://crbug.com/581133 which I just filed so that we > know there is potential cleanup here when we get rid of the version manifest. Done. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:627: } On 2016/01/25 21:00:12, gab wrote: > DCHECK(!archive_resource_name.empty()); > DCHECK(!chrome_packed_7z.empty() || !chrome_7z.empty(); > DCHECK(archive_file); > > to document the expectations at this point. Done. https://codereview.chromium.org/1631903002/diff/1/chrome/installer/test/alter... chrome/installer/test/alternate_version_generator.cc:665: } On 2016/01/25 21:00:12, gab wrote: > DCHECK(!chrome_7z.empty()); > > to document the expectations at this point Done.
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/patch-status/1631903002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1631903002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by grt@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/1631903002/#ps40001 (title: "string16")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1631903002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1631903002/40001
Message was sent while issue was closed.
Description was changed from ========== Add support for component builds in alternate_version_generator. BUG=550983,581133 ========== to ========== Add support for component builds in alternate_version_generator. BUG=550983,581133 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add support for component builds in alternate_version_generator. BUG=550983,581133 ========== to ========== Add support for component builds in alternate_version_generator. BUG=550983,581133 Committed: https://crrev.com/4540853f2b2322700f27f184186c509e2b3e724f Cr-Commit-Position: refs/heads/master@{#371398} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4540853f2b2322700f27f184186c509e2b3e724f Cr-Commit-Position: refs/heads/master@{#371398} |