Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2768573002:
Send traffic-management headers from extension updater. (Closed)
|
Created:
3 years, 9 months ago by Minh X. Nguyen Modified:
3 years, 8 months ago Reviewers:
waffles, Devlin, jochen (gone - plz use gerrit), zel, Sorin Jianu, Nico, Marijn Kruisselbrink, tbarzic CC:
blundell+watchlist_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, droger+watchlist_chromium.org, extensions-reviews_chromium.org, mevissen, sdefresne+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSend traffic-management headers from extension updater.
This change will allow Omaha to mitigate DoS and prioritize traffic better.
BUG=647516
Review-Url: https://codereview.chromium.org/2768573002
Cr-Commit-Position: refs/heads/master@{#461466}
Committed: https://chromium.googlesource.com/chromium/src/+/b764761949ea8376c93fb682ca51a29852e6f39f
Patch Set 1 #Patch Set 2 : Fix dependency error. #Patch Set 3 : Fix a compiler error in ChromeOS. #
Total comments: 4
Patch Set 4 : Change the comment for the new constant values. #Patch Set 5 : Rename on_demand_update to foreground_check. #
Total comments: 12
Patch Set 6 : Address Devlin's concerns in the review. #Patch Set 7 : Fix compile error in chromeos extension. #Patch Set 8 : Fix the compile error on chromeos external_cache.cc (again). #
Total comments: 2
Patch Set 9 : Change id_vector from std::vector<std::string> to std::vector<base::StringPiece>. #
Total comments: 4
Patch Set 10 : Change the code according to mek's comments. #Messages
Total messages: 80 (60 generated)
The CQ bit was checked by mxnguyen@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mxnguyen@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mxnguyen@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...
Description was changed from ========== Send traffic-management headers from extension updater. BUG=647516 ========== to ========== Send traffic-management headers from extension updater. BUG=647516 ==========
mxnguyen@chromium.org changed reviewers: + cpu@chromium.org, mek@chromium.org, sorin@chromium.org, waffles@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sorin@chromium.org changed reviewers: - cpu@chromium.org
lgtm -cpu@ update_client looks good, thank you! https://codereview.chromium.org/2768573002/diff/40001/extensions/browser/upda... File extensions/browser/updater/extension_downloader.h (right): https://codereview.chromium.org/2768573002/diff/40001/extensions/browser/upda... extensions/browser/updater/extension_downloader.h:144: // Constans for foreground/background update requests in the header. Constants. Or we can say "Header values for foreground/background update requests."
lgtm -cpu@ update_client looks good, thank you!
update_client lgtm https://codereview.chromium.org/2768573002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/updater/extension_updater.h (right): https://codereview.chromium.org/2768573002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/updater/extension_updater.h:75: bool on_demand_update; I recommend standardizing on "foreground" or "foreground_check" so that the meaning of this bool is semantically consistent with the header value.
https://codereview.chromium.org/2768573002/diff/40001/extensions/browser/upda... File extensions/browser/updater/extension_downloader.h (right): https://codereview.chromium.org/2768573002/diff/40001/extensions/browser/upda... extensions/browser/updater/extension_downloader.h:144: // Constans for foreground/background update requests in the header. On 2017/03/21 21:12:10, Sorin Jianu wrote: > Constants. > > Or we can say "Header values for foreground/background update requests." Done.
The CQ bit was checked by mxnguyen@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2768573002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/updater/extension_updater.h (right): https://codereview.chromium.org/2768573002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/updater/extension_updater.h:75: bool on_demand_update; On 2017/03/21 21:42:01, waffles wrote: > I recommend standardizing on "foreground" or "foreground_check" so that the > meaning of this bool is semantically consistent with the header value. Done.
The CQ bit was checked by mxnguyen@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.
mxnguyen@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Can we also expand the CL description a bit to include a little of the motivation for this change? (Even just saying something like 'this allows us to priority requests' is useful.) https://codereview.chromium.org/2768573002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/updater/extension_updater_unittest.cc (right): https://codereview.chromium.org/2768573002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/updater/extension_updater_unittest.cc:851: EXPECT_EQ(foreground_check ? "fg" : "bg", interactivity_value); nit: Why not use the ExtensionDownloader constants for fg/bg? https://codereview.chromium.org/2768573002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/updater/extension_updater_unittest.cc:856: EXPECT_EQ(extensions[0]->id(), appid_value); Can we add a test for multiple extension ids? https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... File extensions/browser/updater/extension_downloader.cc (right): https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... extensions/browser/updater/extension_downloader.cc:493: base::StringPrintf("%s: %s", kUpdateAppIdHeader, id_list.c_str())); (see comment in bug about this) https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... File extensions/browser/updater/manifest_fetch_data.cc (right): https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... extensions/browser/updater/manifest_fetch_data.cc:98: return false; It's a little worrisome to me that we just silently abort here. I would expect either: a) We do foreground_check_ |= foreground_check (similar to Merge()) b) We CHECK and enforce this never happens Of the two, a) seems perhaps more reasonable, since otherwise trying to add an Extension with a conflicting foreground value to an existing ManifestFetchData would fail, but creating a new ManifestFetchData and merging them would succeed. https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... extensions/browser/updater/manifest_fetch_data.cc:183: foreground_check_ |= other.foreground_check_; can we add/modify a unittest for this? https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... File extensions/browser/updater/manifest_fetch_data.h (right): https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... extensions/browser/updater/manifest_fetch_data.h:78: bool foreground_check); I'd slightly prefer we use an enum for this, e.g. enum class FetchPriority { FOREGROUND, BACKGROUND, } and then pass that rather than the anonymous true/false everywhere. WDYT?
Description was changed from ========== Send traffic-management headers from extension updater. BUG=647516 ========== to ========== Send traffic-management headers from extension updater. This change will allow Omaha to mitigate DoS and prioritize traffic better. BUG=647516 ==========
The CQ bit was checked by mxnguyen@chromium.org to run a CQ dry run
mxnguyen@chromium.org changed reviewers: + jochen@chromium.org
https://codereview.chromium.org/2768573002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/updater/extension_updater_unittest.cc (right): https://codereview.chromium.org/2768573002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/updater/extension_updater_unittest.cc:851: EXPECT_EQ(foreground_check ? "fg" : "bg", interactivity_value); On 2017/03/24 15:14:46, Devlin wrote: > nit: Why not use the ExtensionDownloader constants for fg/bg? This is to make sure that any change in these constant values would break this unit test. https://codereview.chromium.org/2768573002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/updater/extension_updater_unittest.cc:856: EXPECT_EQ(extensions[0]->id(), appid_value); On 2017/03/24 15:14:46, Devlin wrote: > Can we add a test for multiple extension ids? Done. https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... File extensions/browser/updater/extension_downloader.cc (right): https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... extensions/browser/updater/extension_downloader.cc:493: base::StringPrintf("%s: %s", kUpdateAppIdHeader, id_list.c_str())); On 2017/03/24 15:14:46, Devlin wrote: > (see comment in bug about this) Acknowledged. https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... File extensions/browser/updater/manifest_fetch_data.cc (right): https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... extensions/browser/updater/manifest_fetch_data.cc:98: return false; On 2017/03/24 15:14:47, Devlin wrote: > It's a little worrisome to me that we just silently abort here. I would expect > either: > a) We do foreground_check_ |= foreground_check (similar to Merge()) > b) We CHECK and enforce this never happens > > Of the two, a) seems perhaps more reasonable, since otherwise trying to add an > Extension with a conflicting foreground value to an existing ManifestFetchData > would fail, but creating a new ManifestFetchData and merging them would succeed. Done. https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... extensions/browser/updater/manifest_fetch_data.cc:183: foreground_check_ |= other.foreground_check_; On 2017/03/24 15:14:47, Devlin wrote: > can we add/modify a unittest for this? Done. https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... File extensions/browser/updater/manifest_fetch_data.h (right): https://codereview.chromium.org/2768573002/diff/80001/extensions/browser/upda... extensions/browser/updater/manifest_fetch_data.h:78: bool foreground_check); On 2017/03/24 15:14:47, Devlin wrote: > I'd slightly prefer we use an enum for this, e.g. > enum class FetchPriority { > FOREGROUND, > BACKGROUND, > } > > and then pass that rather than the anonymous true/false everywhere. WDYT? Done.
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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by mxnguyen@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mxnguyen@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.
deferring to rdevlin.cronin@
extensions lgtm https://codereview.chromium.org/2768573002/diff/140001/extensions/browser/upd... File extensions/browser/updater/extension_downloader.cc (right): https://codereview.chromium.org/2768573002/diff/140001/extensions/browser/upd... extensions/browser/updater/extension_downloader.cc:474: std::vector<std::string> id_vector(active_request->extension_ids().begin(), nit: we can now use JoinString with StringPieces; prefer a std::vector<base::StringPiece> here.
The CQ bit was checked by mxnguyen@chromium.org to run a CQ dry run
https://codereview.chromium.org/2768573002/diff/140001/extensions/browser/upd... File extensions/browser/updater/extension_downloader.cc (right): https://codereview.chromium.org/2768573002/diff/140001/extensions/browser/upd... extensions/browser/updater/extension_downloader.cc:474: std::vector<std::string> id_vector(active_request->extension_ids().begin(), On 2017/03/28 22:48:54, Devlin wrote: > nit: we can now use JoinString with StringPieces; prefer a > std::vector<base::StringPiece> here. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mxnguyen@chromium.org changed reviewers: + tbarzic@chromium.org, zelidrag@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by mxnguyen@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.
lgtm hmm, it seems I forgot to actually publish the comments I had last week... anyway, they were mostly the same as what devlin already commented on since, just two minor things https://codereview.chromium.org/2768573002/diff/160001/extensions/browser/upd... File extensions/browser/updater/extension_downloader.cc (right): https://codereview.chromium.org/2768573002/diff/160001/extensions/browser/upd... extensions/browser/updater/extension_downloader.cc:478: if (VLOG_IS_ON(2)) { nit: I don't think there is any reason anymore for the VLOG_IS_ON check. Before the check was done to avoid the expensive operation of creating the id_vector when we're not logging it anyway, but since you're always creating the vector now, you can get rid of this if. https://codereview.chromium.org/2768573002/diff/160001/extensions/browser/upd... extensions/browser/updater/extension_downloader.cc:494: // Send traffic-management headers. http://crosbug.com/130602 This URL results in a 404?
The CQ bit was checked by mxnguyen@chromium.org to run a CQ dry run
https://codereview.chromium.org/2768573002/diff/160001/extensions/browser/upd... File extensions/browser/updater/extension_downloader.cc (right): https://codereview.chromium.org/2768573002/diff/160001/extensions/browser/upd... extensions/browser/updater/extension_downloader.cc:478: if (VLOG_IS_ON(2)) { On 2017/03/30 21:50:45, Marijn Kruisselbrink wrote: > nit: I don't think there is any reason anymore for the VLOG_IS_ON check. Before > the check was done to avoid the expensive operation of creating the id_vector > when we're not logging it anyway, but since you're always creating the vector > now, you can get rid of this if. Done. https://codereview.chromium.org/2768573002/diff/160001/extensions/browser/upd... extensions/browser/updater/extension_downloader.cc:494: // Send traffic-management headers. http://crosbug.com/130602 On 2017/03/30 21:50:45, Marijn Kruisselbrink wrote: > This URL results in a 404? Done.
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.
The CQ bit was checked by sorin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org, sorin@chromium.org, rdevlin.cronin@chromium.org, mek@chromium.org Link to the patchset: https://codereview.chromium.org/2768573002/#ps180001 (title: "Change the code according to mek's comments.")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mxnguyen@chromium.org changed reviewers: + thakis@chromium.org
lgtm
The CQ bit was checked by mxnguyen@chromium.org
The CQ bit was unchecked by mxnguyen@chromium.org
The CQ bit was checked by sorin@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": 180001, "attempt_start_ts": 1491235603087010,
"parent_rev": "5ffab89a82a2dbf5e7b974fb41f7a9767e846c03", "commit_rev":
"b764761949ea8376c93fb682ca51a29852e6f39f"}
Message was sent while issue was closed.
Description was changed from ========== Send traffic-management headers from extension updater. This change will allow Omaha to mitigate DoS and prioritize traffic better. BUG=647516 ========== to ========== Send traffic-management headers from extension updater. This change will allow Omaha to mitigate DoS and prioritize traffic better. BUG=647516 Review-Url: https://codereview.chromium.org/2768573002 Cr-Commit-Position: refs/heads/master@{#461466} Committed: https://chromium.googlesource.com/chromium/src/+/b764761949ea8376c93fb682ca51... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/b764761949ea8376c93fb682ca51... |
