Factor Chrome details out of update manifest fetching.

Factor Chrome details out of update manifest fetching.

This establishes a ManifestFetchDataDelegate for use
by ExtensionDownloader and ManifestFetchData, and moves
ManifestFetchData into //extensions/browser/updater.

The delegate provides implementation details for
update manifest fetching, including brand code,
boilerplate query parameters, and ping data.

Chrome's implementation has knowledge of the
Google brand, the metrics service, and Omaha-specific
query parameters necessary for CRX item queries.

BUG=398671
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289800

[Ken Rockot(use gerrit already), 2014-08-12 01:07:38]

Ben, could you please take a look?

I'm not so hot on the delegate interface name here (ManifestFetchDataDelegate)
but didn't come up with any alternatives that I liked much better.

Maybe UpdateManifestFetchDelegate (eh)  or UpdateManifestQueryDelegate or
ManifestFetchQueryDelegate? Something. Eh.

[not at google - send to devlin, 2014-08-12 16:33:03]

https://codereview.chromium.org/465543004/diff/60001/extensions/browser/updat...
File extensions/browser/updater/default_manifest_fetch_data_delegate.h (right):

https://codereview.chromium.org/465543004/diff/60001/extensions/browser/updat...
extensions/browser/updater/default_manifest_fetch_data_delegate.h:21: virtual
std::string GetBrandCode() OVERRIDE;
I guess that it's "nice", in some respects, to lump together providing
dependencies in the one place - but note that these 3 values are just static
data. You could just as easily pass them in as constructor parameters to
ManifestFetchData, and in fact, they do seem analogous to the |update_url|
parameter?

Another observation: ShouldAlwaysCheckWebstore is only called from
ExtensionDownloader, but that hasn't been moved to /extensions, so why the need
for the delegate there? What's wrong with it using
ChromeMetricsServiceAccessor::IsMetricsReportingEnabled?

So before I go on, here's an idea: make all of these constructor arguments to
ManifestFetchData. You may want to bundle them into a struct to avoid confusion,
maybe. WDYT?

https://codereview.chromium.org/465543004/diff/60001/extensions/browser/updat...
File extensions/browser/updater/manifest_fetch_data.cc (right):

https://codereview.chromium.org/465543004/diff/60001/extensions/browser/updat...
extensions/browser/updater/manifest_fetch_data.cc:96: ping_value =
delegate_->GetPingParamsAndData(id, *ping_data, &pings_[id]);
Why did you need to delegate-ify this? What's wrong with just moving that
IsMetricsReportingEnabled call into the Delegate (or making it a constructor
argument - see previous comment).

[Ken Rockot(use gerrit already), 2014-08-12 16:42:58]

On 2014/08/12 16:33:03, kalman wrote:
>
https://codereview.chromium.org/465543004/diff/60001/extensions/browser/updat...
> File extensions/browser/updater/default_manifest_fetch_data_delegate.h
(right):
> 
>
https://codereview.chromium.org/465543004/diff/60001/extensions/browser/updat...
> extensions/browser/updater/default_manifest_fetch_data_delegate.h:21: virtual
> std::string GetBrandCode() OVERRIDE;
> I guess that it's "nice", in some respects, to lump together providing
> dependencies in the one place - but note that these 3 values are just static
> data. You could just as easily pass them in as constructor parameters to
> ManifestFetchData, and in fact, they do seem analogous to the |update_url|
> parameter?
> 

I'm fine with changing the brand code and the query parameters to be constructor
arguments of ManifestFetchData. The ping-related stuff needs to be delegated
because that logic is Chrome-specific (or at least, it's a specific strategy
that not all downloaders will want to use).

However, ExtensionDownloader is the thing creating these ManifestFetchData
objects, and so the configuration of what URL, query params, etc, to use, 
need to then be proxied through whomever creates ExtensionDownloader
(usually, but not always, this is ExtensionUpdater).

So I could see ExtensionDownloader's constructor being configurable with
update URL, base query parameters, brand code, and some delegated behavior
for additional ping parameters. Apart from the update URL itself, all of
this stuff can be configured after construction rather than via the
constructor.

> Another observation: ShouldAlwaysCheckWebstore is only called from
> ExtensionDownloader, but that hasn't been moved to /extensions, so why the
need
> for the delegate there? What's wrong with it using
> ChromeMetricsServiceAccessor::IsMetricsReportingEnabled?
> 

I'm not moving it in this patch, but I am moving it in a near-future patch.
Seemed like a good dependency to knock out in this patch since
ManifestFetchData also depended on it.

> So before I go on, here's an idea: make all of these constructor arguments to
> ManifestFetchData. You may want to bundle them into a struct to avoid
confusion,
> maybe. WDYT?
> 
>
https://codereview.chromium.org/465543004/diff/60001/extensions/browser/updat...
> File extensions/browser/updater/manifest_fetch_data.cc (right):
> 
>
https://codereview.chromium.org/465543004/diff/60001/extensions/browser/updat...
> extensions/browser/updater/manifest_fetch_data.cc:96: ping_value =
> delegate_->GetPingParamsAndData(id, *ping_data, &pings_[id]);
> Why did you need to delegate-ify this? What's wrong with just moving that
> IsMetricsReportingEnabled call into the Delegate (or making it a constructor
> argument - see previous comment).

Good point, "report_metrics" could be a constructor flag.

[Ken Rockot(use gerrit already), 2014-08-12 21:47:30]

OK, how about this?

No changes to ExtensionDownloader constructor. All manifest fetch configuration
is optional through public methods.

ExtensionDownloader provides this configuration state to new new
ManifestFetchData instances it constructs.

ExtensionService's ExtensionDownloader factory sets up all the extra
Chrome-specific behavior around ping data, metrics, Omaha params, and brand
code.

[not at google - send to devlin, 2014-08-13 01:30:02]

lgtm

https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:605: if (!brand.empty() &&
!google_brand::IsOrganic(brand))
I read this as IsOrganic(bread). Yum.

https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/updater/extension_downloader.h (right):

https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/updater/extension_downloader.h:104: void
SetBrandCode(const std::string& brand_code);
Consider making these simpler set_brand_code methods to declare inline. All they
do is set properties verbatim.

https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/updater/extension_updater_unittest.cc:1741:
omaha_query_params::OmahaQueryParams::Get(
Ok, "omaha_query_params::OmahaQueryParams" looks very silly. I would using-ify
it, but up to you.

https://codereview.chromium.org/465543004/diff/80001/extensions/browser/updat...
File extensions/browser/updater/manifest_fetch_data.cc (right):

https://codereview.chromium.org/465543004/diff/80001/extensions/browser/updat...
extensions/browser/updater/manifest_fetch_data.cc:96: if (ping_mode_ >= PING) {
Since when did PingMode define an ordering? Either comment that there is, or
preferably use != NO_PING because that makes more sense to me.

[Ken Rockot(use gerrit already), 2014-08-13 02:36:32]

Many graciases

On 2014/08/13 01:30:02, kalman wrote:
> lgtm
> 
>
https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
> chrome/browser/extensions/extension_service.cc:605: if (!brand.empty() &&
> !google_brand::IsOrganic(brand))
> I read this as IsOrganic(bread). Yum.
> 
>
https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
> File chrome/browser/extensions/updater/extension_downloader.h (right):
> 
>
https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
> chrome/browser/extensions/updater/extension_downloader.h:104: void
> SetBrandCode(const std::string& brand_code);
> Consider making these simpler set_brand_code methods to declare inline. All
they
> do is set properties verbatim.
> 

Oh, yeah. Duh. Good call.

>
https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
> File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):
> 
>
https://codereview.chromium.org/465543004/diff/80001/chrome/browser/extension...
> chrome/browser/extensions/updater/extension_updater_unittest.cc:1741:
> omaha_query_params::OmahaQueryParams::Get(
> Ok, "omaha_query_params::OmahaQueryParams" looks very silly. I would using-ify
> it, but up to you.
> 

Actually I refactored this so now we can write:

omaha_query_params::OmahaQueryParams::OmahaQueryParamGetter::Get()->GetOmahaQueryParams(omaha_query_params::OmahaQueryParams::CRX_OMAHA_QUERY_PARAMS)

Just kidding.

>
https://codereview.chromium.org/465543004/diff/80001/extensions/browser/updat...
> File extensions/browser/updater/manifest_fetch_data.cc (right):
> 
>
https://codereview.chromium.org/465543004/diff/80001/extensions/browser/updat...
> extensions/browser/updater/manifest_fetch_data.cc:96: if (ping_mode_ >= PING)
{
> Since when did PingMode define an ordering? Either comment that there is, or
> preferably use != NO_PING because that makes more sense to me.

Fair point. Done.

- 

I also moved ExtensionService's ExtensionDownloader creation code into a static
ChromeExtensionDownloaderFactory interface, since ChromeOS' ExternalCache
actually needs to create these with good Omaha configurations as well (though no
identity stuff). This nicely isolates the code and dependencies needed to create
Chrome-specific ExtensionDownloader instances.

[Ken Rockot(use gerrit already), 2014-08-13 06:30:53]

Hi Jim, could you please take a look for ChromeMetricsServiceAccessor?

[Ilya Sherman, 2014-08-13 08:20:52]

LGTM % the nit below, but I wonder whether the extensions code still needs to
pay attention to whether metrics are enabled.  Could you please remind me why
the behavior branches on this condition?  What is the extra data that's sent or
requested from the extensions code if metrics reporting is enabled?

https://codereview.chromium.org/465543004/diff/140001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_accessor.h (right):

https://codereview.chromium.org/465543004/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_accessor.h:16: class
ExtensionService;
Hmm, why is this forward-declaration needed?

[Ken Rockot(use gerrit already), 2014-08-13 13:38:56]

Thanks.

On 2014/08/13 08:20:52, Ilya Sherman wrote:
> LGTM % the nit below, but I wonder whether the extensions code still needs to
> pay attention to whether metrics are enabled.  Could you please remind me why
> the behavior branches on this condition?  What is the extra data that's sent
or
> requested from the extensions code if metrics reporting is enabled?
> 

It looks like this is used to collect data in Omaha. Specifically, if enabled:

1. Every extension update ping is duplicated to Omaha even if the extension's
update URL doesn't point to Omaha.
2. Every extension update ping to Omaha includes an extra boolean flag
indicating whether the extension is enabled at the time of ping.

I suspect we do want #1 because it allows us do still want this data, but
+asargent who might know better.

>
https://codereview.chromium.org/465543004/diff/140001/chrome/browser/metrics/...
> File chrome/browser/metrics/chrome_metrics_service_accessor.h (right):
> 
>
https://codereview.chromium.org/465543004/diff/140001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_metrics_service_accessor.h:16: class
> ExtensionService;
> Hmm, why is this forward-declaration needed?

Ah, dust from iteration. Will remove.

[Ken Rockot(use gerrit already), 2014-08-13 15:05:41]

On 2014/08/13 13:38:56, Ken Rockot wrote:
> Thanks.
> 
> On 2014/08/13 08:20:52, Ilya Sherman wrote:
> > LGTM % the nit below, but I wonder whether the extensions code still needs
to
> > pay attention to whether metrics are enabled.  Could you please remind me
why
> > the behavior branches on this condition?  What is the extra data that's sent
> or
> > requested from the extensions code if metrics reporting is enabled?
> > 
> 
> It looks like this is used to collect data in Omaha. Specifically, if enabled:
> 
> 1. Every extension update ping is duplicated to Omaha even if the extension's
> update URL doesn't point to Omaha.
> 2. Every extension update ping to Omaha includes an extra boolean flag
> indicating whether the extension is enabled at the time of ping.
> 
> I suspect we do want #1 because it allows us do still want this data, but
> +asargent who might know better.
> 
> >
>
https://codereview.chromium.org/465543004/diff/140001/chrome/browser/metrics/...
> > File chrome/browser/metrics/chrome_metrics_service_accessor.h (right):
> > 
> >
>
https://codereview.chromium.org/465543004/diff/140001/chrome/browser/metrics/...
> > chrome/browser/metrics/chrome_metrics_service_accessor.h:16: class
> > ExtensionService;
> > Hmm, why is this forward-declaration needed?
> 
> Ah, dust from iteration. Will remove.

Just re-reading this comment now, apparently I failed at editing. I suspect we
still want this data. :>

[Ken Rockot(use gerrit already), 2014-08-13 15:54:19]

Alright, I spoke with erikkay@ off-thread; we do still want these metrics.
Thanks again.

[Ken Rockot(use gerrit already), 2014-08-13 17:57:58]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-08-13 17:58:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/465543004/160001

[commit-bot: I haz the power, 2014-08-13 21:52:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-14 09:58:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 09:58:34]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[Ken Rockot(use gerrit already), 2014-08-14 16:02:33]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-08-14 16:05:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/465543004/160001

[commit-bot: I haz the power, 2014-08-15 04:33:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-08-15 05:21:21]

Committed patchset #5 (160001) as 289800

[gunsch, 2014-08-15 06:30:24]

A revert of this CL (patchset #5) has been created in
https://codereview.chromium.org/470653007/ by gunsch@chromium.org.

The reason for reverting is: Broke official builds:

http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrome...

Fix is probably as simple as including "chrome/browser/google/google_brand.h" in
"chrome_extension_dowloader_factory.cc"..

[Nikita (slow), 2014-08-15 08:33:20]

A revert of this CL (patchset #5) has been created in
https://codereview.chromium.org/476073002/ by nkostylev@chromium.org.

The reason for reverting is: Doesn't compile on all chromium.chrome buildbots
(official)..

[Nikita (slow), 2014-08-15 08:35:49]

On 2014/08/15 08:33:20, Nikita Kostylev wrote:
> A revert of this CL (patchset #5) has been created in
> https://codereview.chromium.org/476073002/ by mailto:nkostylev@chromium.org.
> 
> The reason for reverting is: Doesn't compile on all chromium.chrome buildbots
> (official)..

http://goo.gl/Hqf2BE
http://goo.gl/CyHVt9
http://goo.gl/0QGGSu
http://goo.gl/tJuF45
http://goo.gl/WLylCf

[Nikita (slow), 2014-08-15 08:36:35]

On 2014/08/15 08:35:49, Nikita Kostylev wrote:
> On 2014/08/15 08:33:20, Nikita Kostylev wrote:
> > A revert of this CL (patchset #5) has been created in
> > https://codereview.chromium.org/476073002/ by mailto:nkostylev@chromium.org.
> > 
> > The reason for reverting is: Doesn't compile on all chromium.chrome
buildbots
> > (official)..
> 
> http://goo.gl/Hqf2BE
> http://goo.gl/CyHVt9
> http://goo.gl/0QGGSu
> http://goo.gl/tJuF45
> http://goo.gl/WLylCf

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/chrome/browser/extensions/updater/browser_extensions.chrome_extension_downloader_factory.o.d
-DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DGOOGLE_CHROME_BUILD
-DCR_CLANG_REVISION=214024 -DENABLE_RLZ -DTOOLKIT_VIEWS=1
-DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_ASH=1 -DUSE_PANGO=1
-DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_X11=1
-DUSE_XI2_MT=2 -DIMAGE_LOADER_EXTENSION=1 -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1
-DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY
-DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DUSE_UDEV -DENABLE_EGLIMAGE=1
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGINS=1
-DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1
-DENABLE_PROD_WALLET_SERVICE=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1
-DCLD_VERSION=2 -DCLD2_DATA_SOURCE=static -DENABLE_FULL_PRINTING=1
-DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
-DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1
-DENABLE_SERVICE_DISCOVERY=1 -DFULL_SAFE_BROWSING -DUSE_BRLAPI
-DGL_GLEXT_PROTOTYPES -DMOJO_USE_SYSTEM_IMPL -DLIBPEERCONNECTION_LIB=1
-DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1
'-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"'
-DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT
-DSK_SUPPORT_LEGACY_PICTURE_CLONE -DSK_SUPPORT_LEGACY_GETDEVICE
-DSK_IGNORE_ETC1_SUPPORT -DSK_IGNORE_GPU_DITHER -DSK_USE_POSIX_THREADS
-DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DWEBRTC_CHROMIUM_BUILD
-DWEBRTC_LINUX -DWEBRTC_POSIX -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI
-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DUSE_NSS=1 -DOS_CHROMEOS=1
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DOFFICIAL_BUILD
-DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen -I../..
-Iobj/chrome/browser_extensions.gen -Iobj/chrome/browser_extensions.gen/chrome
-I../../third_party/khronos -I../../gpu -I../../skia/config
-I../../third_party/WebKit/Source -Igen/chrome -Igen/protoc_out
-Igen/components/strings -I../../third_party/WebKit
-I../../net/third_party/nss/ssl -Igen/extensions/strings
-I../../third_party/skia/src/core -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf
-I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy
-I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../skia/ext -I../../third_party/cacheinvalidation/overrides
-I../../third_party/cacheinvalidation/src -I../../third_party/icu/source/i18n
-I../../third_party/icu/source/common
-I../../third_party/leveldatabase/src/include
-I../../third_party/leveldatabase/src -I../../third_party/leveldatabase
-I../../third_party/re2 -I../../third_party/webrtc/overrides -I../../third_party
-Igen/ui/resources -Igen/ui/strings -Igen/webkit -Igen/ui/keyboard -Igen/policy
-I../../third_party/protobuf -I../../third_party/protobuf/src -Werror -pthread
-fno-exceptions -fno-strict-aliasing -Wall -Wno-unused-parameter
-Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC
-Wno-reserved-user-defined-literal -Xclang -load -Xclang
/b/build/slave/google-chrome-rel-chromeos/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics -g
-B/b/build/slave/google-chrome-rel-chromeos/build/src/third_party/binutils/Linux_x64/Release/bin
-Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wexit-time-destructors -pthread
-I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include
-I/usr/include/nss -I/usr/include/nspr -Wno-header-guard -I/usr/include/dbus-1.0
-I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -m64 -march=x86-64 -O2
-fdata-sections -ffunction-sections -fno-slp-vectorize -fno-unwind-tables
-fno-asynchronous-unwind-tables -fno-rtti -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wsign-compare -std=gnu++11  -c
../../chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc
-o
obj/chrome/browser/extensions/updater/browser_extensions.chrome_extension_downloader_factory.o
../../chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc:30:3:
error: use of undeclared identifier 'google_brand'
  google_brand::GetBrand(&brand);
  ^
../../chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc:30:27:
error: declaration of reference variable 'brand' requires an initializer
  google_brand::GetBrand(&brand);
                          ^~~~~
../../chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc:31:26:
error: use of undeclared identifier 'google_brand'
  if (!brand.empty() && !google_brand::IsOrganic(brand))

[Nikita (slow), 2014-08-15 08:38:15]

On 2014/08/15 08:36:35, Nikita Kostylev wrote:
> On 2014/08/15 08:35:49, Nikita Kostylev wrote:
> > On 2014/08/15 08:33:20, Nikita Kostylev wrote:
> > > A revert of this CL (patchset #5) has been created in
> > > https://codereview.chromium.org/476073002/ by
mailto:nkostylev@chromium.org.
> > > 
> > > The reason for reverting is: Doesn't compile on all chromium.chrome
> buildbots
> > > (official)..
> > 
> > http://goo.gl/Hqf2BE
> > http://goo.gl/CyHVt9
> > http://goo.gl/0QGGSu
> > http://goo.gl/tJuF45
> > http://goo.gl/WLylCf
> 
> FAILED: /b/build/goma/gomacc
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
>
obj/chrome/browser/extensions/updater/browser_extensions.chrome_extension_downloader_factory.o.d
> -DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DGOOGLE_CHROME_BUILD
> -DCR_CLANG_REVISION=214024 -DENABLE_RLZ -DTOOLKIT_VIEWS=1
> -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_ASH=1 -DUSE_PANGO=1
> -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_X11=1
> -DUSE_XI2_MT=2 -DIMAGE_LOADER_EXTENSION=1 -DENABLE_REMOTING=1
-DENABLE_WEBRTC=1
> -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY
> -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DUSE_UDEV -DENABLE_EGLIMAGE=1
> -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGINS=1
> -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1
> -DENABLE_PROD_WALLET_SERVICE=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1
> -DCLD_VERSION=2 -DCLD2_DATA_SOURCE=static -DENABLE_FULL_PRINTING=1
> -DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
> -DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1
> -DENABLE_SERVICE_DISCOVERY=1 -DFULL_SAFE_BROWSING -DUSE_BRLAPI
> -DGL_GLEXT_PROTOTYPES -DMOJO_USE_SYSTEM_IMPL -DLIBPEERCONNECTION_LIB=1
> -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1
> '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"'
> -DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1
> -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT
> -DSK_SUPPORT_LEGACY_PICTURE_CLONE -DSK_SUPPORT_LEGACY_GETDEVICE
> -DSK_IGNORE_ETC1_SUPPORT -DSK_IGNORE_GPU_DITHER -DSK_USE_POSIX_THREADS
> -DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DU_USING_ICU_NAMESPACE=0
> -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DWEBRTC_CHROMIUM_BUILD
> -DWEBRTC_LINUX -DWEBRTC_POSIX -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI
> -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DUSE_NSS=1 -DOS_CHROMEOS=1
> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DOFFICIAL_BUILD
> -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen -I../..
> -Iobj/chrome/browser_extensions.gen -Iobj/chrome/browser_extensions.gen/chrome
> -I../../third_party/khronos -I../../gpu -I../../skia/config
> -I../../third_party/WebKit/Source -Igen/chrome -Igen/protoc_out
> -Igen/components/strings -I../../third_party/WebKit
> -I../../net/third_party/nss/ssl -Igen/extensions/strings
> -I../../third_party/skia/src/core -I../../third_party/skia/include/core
> -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf
> -I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy
> -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe
> -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
> -I../../skia/ext -I../../third_party/cacheinvalidation/overrides
> -I../../third_party/cacheinvalidation/src -I../../third_party/icu/source/i18n
> -I../../third_party/icu/source/common
> -I../../third_party/leveldatabase/src/include
> -I../../third_party/leveldatabase/src -I../../third_party/leveldatabase
> -I../../third_party/re2 -I../../third_party/webrtc/overrides
-I../../third_party
> -Igen/ui/resources -Igen/ui/strings -Igen/webkit -Igen/ui/keyboard
-Igen/policy
> -I../../third_party/protobuf -I../../third_party/protobuf/src -Werror -pthread
> -fno-exceptions -fno-strict-aliasing -Wall -Wno-unused-parameter
> -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC
> -Wno-reserved-user-defined-literal -Xclang -load -Xclang
>
/b/build/slave/google-chrome-rel-chromeos/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
> -Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics -g
>
-B/b/build/slave/google-chrome-rel-chromeos/build/src/third_party/binutils/Linux_x64/Release/bin
> -Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration
> -Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
> -Wno-deprecated-register -Wexit-time-destructors -pthread
> -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include
> -I/usr/include/nss -I/usr/include/nspr -Wno-header-guard
-I/usr/include/dbus-1.0
> -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -m64 -march=x86-64 -O2
> -fdata-sections -ffunction-sections -fno-slp-vectorize -fno-unwind-tables
> -fno-asynchronous-unwind-tables -fno-rtti -fno-threadsafe-statics
> -fvisibility-inlines-hidden -Wsign-compare -std=gnu++11  -c
> ../../chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc
> -o
>
obj/chrome/browser/extensions/updater/browser_extensions.chrome_extension_downloader_factory.o
>
../../chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc:30:3:
> error: use of undeclared identifier 'google_brand'
>   google_brand::GetBrand(&brand);
>   ^
>
../../chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc:30:27:
> error: declaration of reference variable 'brand' requires an initializer
>   google_brand::GetBrand(&brand);
>                           ^~~~~
>
../../chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc:31:26:
> error: use of undeclared identifier 'google_brand'
>   if (!brand.empty() && !google_brand::IsOrganic(brand))

It seems that just header was missing: chrome/browser/google/google_brand.h

Please build an official build locally though since none of CQ trybots will do
that.