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

Issue 2077973002: Generate snippets request JSON with base::Value. (Closed)

Created:
4 years, 6 months ago by sfiera
Modified:
4 years, 6 months ago
Reviewers:
Marc Treib
CC:
chromium-reviews, zine-eng+reviews_google.com
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Generate snippets request JSON with base::Value. Doing it as it did with string substitution is brittle, since it forced assumptions about the JSON being generated out of the BuildRequest() function and into its callers and GetHostRestricts(). When we're generating different types of requests, it will be easier to understand what e.g. "obfuscated_gaia_id" is if it's always the ID, and never a partial string of JSON. Expose the BuildRequest() function to tests and test it. BUG=621090 Committed: https://crrev.com/d96c68da73145d482fbfeefc32afbe33e8839251 Cr-Commit-Position: refs/heads/master@{#400656}

Patch Set 1 #

Patch Set 2 : Add unit test for BuildRequest(). #

Total comments: 6

Patch Set 3 : Fixups. #

Patch Set 4 : Avoid bracket initialization. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+176 lines, -79 lines) Patch
M components/ntp_snippets/ntp_snippets_fetcher.h View 1 chunk +9 lines, -1 line 0 comments Download
M components/ntp_snippets/ntp_snippets_fetcher.cc View 1 2 6 chunks +70 lines, -77 lines 0 comments Download
M components/ntp_snippets/ntp_snippets_fetcher_unittest.cc View 1 2 3 5 chunks +97 lines, -1 line 0 comments Download

Messages

Total messages: 15 (6 generated)
sfiera
Been a long time since you've been the only approval I've needed!
4 years, 6 months ago (2016-06-17 15:19:04 UTC) #2
Marc Treib
Much nicer, thanks! https://codereview.chromium.org/2077973002/diff/20001/components/ntp_snippets/ntp_snippets_fetcher.cc File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2077973002/diff/20001/components/ntp_snippets/ntp_snippets_fetcher.cc#newcode204 components/ntp_snippets/ntp_snippets_fetcher.cc:204: std::unique_ptr<base::DictionaryValue> entry(new base::DictionaryValue); Any reason this ...
4 years, 6 months ago (2016-06-17 15:28:09 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077973002/40001
4 years, 6 months ago (2016-06-20 10:21:09 UTC) #5
sfiera
https://codereview.chromium.org/2077973002/diff/20001/components/ntp_snippets/ntp_snippets_fetcher.cc File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2077973002/diff/20001/components/ntp_snippets/ntp_snippets_fetcher.cc#newcode204 components/ntp_snippets/ntp_snippets_fetcher.cc:204: std::unique_ptr<base::DictionaryValue> entry(new base::DictionaryValue); On 2016/06/17 15:28:09, Marc Treib wrote: ...
4 years, 6 months ago (2016-06-20 10:21:52 UTC) #6
Marc Treib
lgtm
4 years, 6 months ago (2016-06-20 10:23:34 UTC) #7
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_compile_dbg_ng/builds/218352)
4 years, 6 months ago (2016-06-20 10:48:52 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077973002/60001
4 years, 6 months ago (2016-06-20 10:53:56 UTC) #12
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 6 months ago (2016-06-20 11:52:24 UTC) #13
commit-bot: I haz the power
4 years, 6 months ago (2016-06-20 11:54:16 UTC) #15
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/d96c68da73145d482fbfeefc32afbe33e8839251
Cr-Commit-Position: refs/heads/master@{#400656}

Powered by Google App Engine
This is Rietveld 408576698