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

Issue 12396002: Add chrome version information to extension update checks (Closed)

Created:
7 years, 9 months ago by jackhou1
Modified:
7 years, 8 months ago
CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Factor out OmahaQueryParams Add chrome version information to extension update checks. BUG=72262 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187266

Patch Set 1 #

Total comments: 6

Patch Set 2 : Address comments #

Total comments: 6

Patch Set 3 : Address comments #

Total comments: 1

Patch Set 4 : Address comment #

Patch Set 5 : Rebase #

Patch Set 6 : Use count() instead of find() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+207 lines, -88 lines) Patch
M chrome/browser/component_updater/component_updater_configurator.cc View 1 4 chunks +3 lines, -60 lines 0 comments Download
M chrome/browser/extensions/updater/extension_updater_unittest.cc View 1 2 3 4 5 8 chunks +53 lines, -28 lines 0 comments Download
M chrome/browser/extensions/updater/manifest_fetch_data.cc View 1 2 3 2 chunks +14 lines, -0 lines 0 comments Download
M chrome/chrome_common.gypi View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/common/omaha_query_params.h View 1 2 1 chunk +33 lines, -0 lines 0 comments Download
A chrome/common/omaha_query_params.cc View 1 2 1 chunk +102 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
jackhou1
7 years, 9 months ago (2013-03-04 06:28:14 UTC) #1
jackhou1
Oops, I should have mentioned: asargent: could you please review this cl? mevissen, oemilyo: could ...
7 years, 9 months ago (2013-03-04 06:46:35 UTC) #2
oemilyo_google.com
in component_updater_configurator.cc, line 78 -- so, the default value for "prod" would be "chrome"? or ...
7 years, 9 months ago (2013-03-04 18:25:58 UTC) #3
mevissen
LGTM Name strings look okay to me.
7 years, 9 months ago (2013-03-04 21:56:25 UTC) #4
jackhou1
On 2013/03/04 18:25:58, oemilyo_google.com wrote: > in component_updater_configurator.cc, line 78 -- so, the default value ...
7 years, 9 months ago (2013-03-05 04:59:43 UTC) #5
asargent_no_longer_on_chrome
https://codereview.chromium.org/12396002/diff/1/chrome/browser/extensions/updater/extension_updater_unittest.cc File chrome/browser/extensions/updater/extension_updater_unittest.cc (right): https://codereview.chromium.org/12396002/diff/1/chrome/browser/extensions/updater/extension_updater_unittest.cc#newcode650 chrome/browser/extensions/updater/extension_updater_unittest.cc:650: EXPECT_EQ(params.end(), params.find("ap")); nice cleanup - this is much more ...
7 years, 9 months ago (2013-03-06 00:45:05 UTC) #6
jackhou
Also made OmahaQueryParams a class. https://codereview.chromium.org/12396002/diff/1/chrome/common/omaha_query_params.cc File chrome/common/omaha_query_params.cc (right): https://codereview.chromium.org/12396002/diff/1/chrome/common/omaha_query_params.cc#newcode64 chrome/common/omaha_query_params.cc:64: "unknown"; On 2013/03/06 00:45:06, ...
7 years, 9 months ago (2013-03-06 03:56:36 UTC) #7
asargent_no_longer_on_chrome
LGTM!
7 years, 9 months ago (2013-03-06 18:26:31 UTC) #8
jackhou1
Hi sky, Could you please review for chrome/ and chrome/common? Thanks.
7 years, 9 months ago (2013-03-07 01:15:42 UTC) #9
sky
https://codereview.chromium.org/12396002/diff/12001/chrome/browser/extensions/updater/manifest_fetch_data.cc File chrome/browser/extensions/updater/manifest_fetch_data.cc (right): https://codereview.chromium.org/12396002/diff/12001/chrome/browser/extensions/updater/manifest_fetch_data.cc#newcode37 chrome/browser/extensions/updater/manifest_fetch_data.cc:37: full_url_ = GURL(full_url_.possibly_invalid_spec() + Shouldn't you be using the ...
7 years, 9 months ago (2013-03-07 01:25:33 UTC) #10
jackhou1
https://codereview.chromium.org/12396002/diff/12001/chrome/browser/extensions/updater/manifest_fetch_data.cc File chrome/browser/extensions/updater/manifest_fetch_data.cc (right): https://codereview.chromium.org/12396002/diff/12001/chrome/browser/extensions/updater/manifest_fetch_data.cc#newcode37 chrome/browser/extensions/updater/manifest_fetch_data.cc:37: full_url_ = GURL(full_url_.possibly_invalid_spec() + I couldn't find an easy ...
7 years, 9 months ago (2013-03-07 04:19:24 UTC) #11
sky
LGTM https://codereview.chromium.org/12396002/diff/22001/chrome/browser/extensions/updater/manifest_fetch_data.cc File chrome/browser/extensions/updater/manifest_fetch_data.cc (right): https://codereview.chromium.org/12396002/diff/22001/chrome/browser/extensions/updater/manifest_fetch_data.cc#newcode37 chrome/browser/extensions/updater/manifest_fetch_data.cc:37: std::string query = full_url_.has_query() ? full_url_.query() + "&" ...
7 years, 9 months ago (2013-03-07 16:01:28 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/12396002/29001
7 years, 9 months ago (2013-03-08 02:12:26 UTC) #13
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 9 months ago (2013-03-08 03:14:14 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/12396002/29001
7 years, 9 months ago (2013-03-11 00:40:18 UTC) #15
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 9 months ago (2013-03-11 00:58:04 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/12396002/60001
7 years, 9 months ago (2013-03-11 06:54:05 UTC) #17
commit-bot: I haz the power
Change committed as 187266
7 years, 9 months ago (2013-03-11 10:19:06 UTC) #18
petarj
https://chromiumcodereview.appspot.com/12396002/diff/1/chrome/common/omaha_query_params.cc File chrome/common/omaha_query_params.cc (right): https://chromiumcodereview.appspot.com/12396002/diff/1/chrome/common/omaha_query_params.cc#newcode64 chrome/common/omaha_query_params.cc:64: "unknown"; On 2013/03/06 00:45:06, Antony Sargent wrote: > Maybe ...
7 years, 8 months ago (2013-04-18 16:05:05 UTC) #19
asargent_no_longer_on_chrome
> MIPS architecture ain't wacky, is it? > This part broke the build for MIPS. ...
7 years, 8 months ago (2013-04-18 21:54:23 UTC) #20
petarj
7 years, 8 months ago (2013-04-19 18:00:14 UTC) #21
Message was sent while issue was closed.
On 2013/04/18 21:54:23, Antony Sargent wrote:
> > MIPS architecture ain't wacky, is it?
> > This part broke the build for MIPS.
> > 
> > Check:
> > https://codereview.chromium.org/14211005/
> 
> Sorry for the build break and for any slight against the mips architecture.
> Fortunately my suggestion just worked exactly as intended because clearly
> neither the CL author or any of us reviewers even knew there *was* a MIPS
build.
> =)

That means we need to be more visible in the Chromium community. :)
My fault.

Thanks for looking into the CL that fixes the build break.

Petar

Powered by Google App Engine
This is Rietveld 408576698