Componentizing chrome/browser/services/gcm/gcm_desktop_utils.cc.

Componentizing chrome/browser/services/gcm/gcm_desktop_utils.cc.

Moved chrome/browser/services/gcm/gcm_desktop_utils.* to
components/gcm_driver/.
Added a extra param version_info to CreateGCMDriverDesktop()
for resolving the bad dependancy from chrome.

BUG=519579
Committed: https://crrev.com/ae5abc9a10333564b9e30c5fc1013f97e19ff4e0
Cr-Commit-Position: refs/heads/master@{#347128}

Committed: https://crrev.com/b1b7fee052d510b179b5af2a55c42cc1f6f9b806
Cr-Commit-Position: refs/heads/master@{#350184}

[Jitu( very slow this week), 2015-09-02 08:24:15]

jitendra.ks@samsung.com changed reviewers:
+ droger@chromium.org

[Jitu( very slow this week), 2015-09-02 08:24:15]

@Droger,

PTAL ...

I am getting below error in some bots, please help me out

7541/7668] STAMP
obj/android_webview/system_webview_apk.actions_rules_copies.stamp
FAILED: if [ ! -e lib/libcomponents_unittests.so -o ! -e
lib/libcomponents_unittests.so.TOC ]; then /b/build/goma/gomacc
/b/build/slave/android/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++
-shared -Wl,-z,now -Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs
-Wl,-z,noexecstack -fPIC
-B/b/build/slave/android/build/src/third_party/binutils/Linux_x64/Release/bin
-Wl,-z,relro -Wl,-z,now -fuse-ld=gold -Wl,--build-id=sha1 -Wl,--no-undefined
--sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm
-nostdlib
-L../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libs/armeabi-v7a
-Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a
-Wl,--exclude-libs=libcommon_audio.a -Wl,--exclude-libs=libcommon_audio_neon.a
-Wl,--exclude-libs=libcommon_audio_sse2.a -Wl,--exclude-libs=libiSACFix.a
-Wl,--exclude-libs=libisac_neon.a -Wl,--exclude-libs=libopus.a
-Wl,--exclude-libs=libvpx.a -Wl,--icf=all -Wl,-shared,-Bsymbolic
../../third_party/android_tools/ndk//platforms/android-16/arch-arm/usr/lib/crtbegin_so.o
-Wl,--version-script=/b/build/slave/android/build/src/build/android/android_no_jni_exports.lst
-Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -o
lib/libcomponents_unittests.so -Wl,-soname=libcomponents_unittests.so
@lib/libcomponents_unittests.so.rsp && { readelf -d
lib/libcomponents_unittests.so | grep SONAME ; nm -gD -f p
lib/libcomponents_unittests.so | cut -f1-2 -d' '; } >
lib/libcomponents_unittests.so.TOC; else /b/build/goma/gomacc
/b/build/slave/android/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++
-shared -Wl,-z,now -Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs
-Wl,-z,noexecstack -fPIC
-B/b/build/slave/android/build/src/third_party/binutils/Linux_x64/Release/bin
-Wl,-z,relro -Wl,-z,now -fuse-ld=gold -Wl,--build-id=sha1 -Wl,--no-undefined
--sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm
-nostdlib
-L../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libs/armeabi-v7a
-Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a
-Wl,--exclude-libs=libcommon_audio.a -Wl,--exclude-libs=libcommon_audio_neon.a
-Wl,--exclude-libs=libcommon_audio_sse2.a -Wl,--exclude-libs=libiSACFix.a
-Wl,--exclude-libs=libisac_neon.a -Wl,--exclude-libs=libopus.a
-Wl,--exclude-libs=libvpx.a -Wl,--icf=all -Wl,-shared,-Bsymbolic
../../third_party/android_tools/ndk//platforms/android-16/arch-arm/usr/lib/crtbegin_so.o
-Wl,--version-script=/b/build/slave/android/build/src/build/android/android_no_jni_exports.lst
-Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -o
lib/libcomponents_unittests.so -Wl,-soname=libcomponents_unittests.so
@lib/libcomponents_unittests.so.rsp && { readelf -d
lib/libcomponents_unittests.so | grep SONAME ; nm -gD -f p
lib/libcomponents_unittests.so | cut -f1-2 -d' '; } >
lib/libcomponents_unittests.so.tmp && if ! cmp -s
lib/libcomponents_unittests.so.tmp lib/libcomponents_unittests.so.TOC; then mv
lib/libcomponents_unittests.so.tmp lib/libcomponents_unittests.so.TOC ; fi; fi
obj/components/libgcm_driver.a(obj/components/gcm_driver/gcm_driver.gcm_desktop_utils.o):gcm_desktop_utils.cc:function
gcm::CreateGCMDriverDesktop(scoped_ptr<gcm::GCMClientFactory,
base::DefaultDeleter<gcm::GCMClientFactory> >, PrefService*, base::FilePath
const&, scoped_refptr<net::URLRequestContextGetter> const&,
version_info::Channel, scoped_refptr<base::SequencedTaskRunner> const&,
scoped_refptr<base::SequencedTaskRunner> const&,
scoped_refptr<base::SequencedTaskRunner> const&): error: undefined reference to
'gcm::GCMDriverDesktop::GCMDriverDesktop(scoped_ptr<gcm::GCMClientFactory,
base::DefaultDeleter<gcm::GCMClientFactory> >, gcm::GCMClient::ChromeBuildInfo
const&, std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> > const&, std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&, PrefService*,
base::FilePath const&, scoped_refptr<net::URLRequestContextGetter> const&,
scoped_refptr<base::SequencedTaskRunner> const&,
scoped_refptr<base::SequencedTaskRunner> const&,
scoped_refptr<base::SequencedTaskRunner> const&)'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

[droger, 2015-09-02 08:39:32]

https://codereview.chromium.org/1325063002/diff/1/components/gcm_driver/gcm_d...
File components/gcm_driver/gcm_desktop_utils.cc (right):

https://codereview.chromium.org/1325063002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_desktop_utils.cc:91: const
scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner) {
The link error is because this code is not intended to be used on Android and
the GCMDriverDesktop class is not compiled on Android.

I think the right fix is to do something like:

#if defined(OS_ANDROID)
  NOTREACHED();
  return make_scoped_ptr<GCMDriver>(nullptr);
#else
  return scoped_ptr<GCMDriver>(new GCMDriverDesktop(
      ...));
#endif

[droger, 2015-09-02 08:41:21]

On 2015/09/02 08:39:32, droger wrote:
>
https://codereview.chromium.org/1325063002/diff/1/components/gcm_driver/gcm_d...
> File components/gcm_driver/gcm_desktop_utils.cc (right):
> 
>
https://codereview.chromium.org/1325063002/diff/1/components/gcm_driver/gcm_d...
> components/gcm_driver/gcm_desktop_utils.cc:91: const
> scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner) {
> The link error is because this code is not intended to be used on Android and
> the GCMDriverDesktop class is not compiled on Android.
> 
> I think the right fix is to do something like:
> 
> #if defined(OS_ANDROID)
>   NOTREACHED();
>   return make_scoped_ptr<GCMDriver>(nullptr);
> #else
>   return scoped_ptr<GCMDriver>(new GCMDriverDesktop(
>       ...));
> #endif

Or maybe it would be even better to just exclude gcm_desktop_utils on android
completely in the gyp and BUILD files?

[Jitu( very slow this week), 2015-09-02 12:32:18]

PTAL ...

[Jitu( very slow this week), 2015-09-02 12:34:50]

On 2015/09/02 08:41:21, droger wrote:
> On 2015/09/02 08:39:32, droger wrote:
> >
>
https://codereview.chromium.org/1325063002/diff/1/components/gcm_driver/gcm_d...
> > File components/gcm_driver/gcm_desktop_utils.cc (right):
> > 
> >
>
https://codereview.chromium.org/1325063002/diff/1/components/gcm_driver/gcm_d...
> > components/gcm_driver/gcm_desktop_utils.cc:91: const
> > scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner) {
> > The link error is because this code is not intended to be used on Android
and
> > the GCMDriverDesktop class is not compiled on Android.
> > 
> > I think the right fix is to do something like:
> > 
> > #if defined(OS_ANDROID)
> >   NOTREACHED();
> >   return make_scoped_ptr<GCMDriver>(nullptr);
> > #else
> >   return scoped_ptr<GCMDriver>(new GCMDriverDesktop(
> >       ...));
> > #endif
> 
> Or maybe it would be even better to just exclude gcm_desktop_utils on android
> completely in the gyp and BUILD files?

Thanks it resolved.

[droger, 2015-09-02 12:40:25]

LGTM!

[Jitu( very slow this week), 2015-09-02 12:57:47]

jitendra.ks@samsung.com changed reviewers:
+ zea@chromium.org

[Jitu( very slow this week), 2015-09-02 12:57:48]

@Nicolas,

Please do the owner review for
 -- chrome/browser/services/*
 -- components/gcm_driver/*

[Jitu( very slow this week), 2015-09-02 13:00:06]

jitendra.ks@samsung.com changed reviewers:
+ bauerb@chromium.org

[Jitu( very slow this week), 2015-09-02 13:00:06]

@Bauer,

Please do the owner review for
 -- chrome/browser/browser_process_impl.cc

[Bernhard Bauer, 2015-09-02 13:04:14]

On 2015/09/02 13:00:06, jitu wrote:
> @Bauer,
> 
> Please do the owner review for
>  -- chrome/browser/browser_process_impl.cc

I'm not an OWNER for that file (or any others in this CL).

[Bernhard Bauer, 2015-09-02 13:04:28]

bauerb@chromium.org changed reviewers:
- bauerb@chromium.org

[droger, 2015-09-02 13:07:02]

On 2015/09/02 13:04:14, Bernhard Bauer wrote:
> On 2015/09/02 13:00:06, jitu wrote:
> > @Bauer,
> > 
> > Please do the owner review for
> >  -- chrome/browser/browser_process_impl.cc
> 
> I'm not an OWNER for that file (or any others in this CL).

I think we can TBR owners for browser_process_impl.cc, once the rest of the CL
is approved.
When changing a function prototype, the general rule is that a full owner review
is required from the owner of the function, but the owners of the callsites can
be TBR'ed.

[Nicolas Zea, 2015-09-02 21:22:33]

LGTM

[Jitu( very slow this week), 2015-09-03 04:46:28]

jitendra.ks@samsung.com changed reviewers:
+ thestig@chromium.org

[Jitu( very slow this week), 2015-09-03 04:46:29]

Dear Lei Zhang,

Please do the owner review for
-- chrome/browser/browser_process_impl.cc

[Lei Zhang, 2015-09-03 04:53:27]

lgtm

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1167:
scoped_refptr<base::SequencedWorkerPool> worker_pool(
This can just be a base::SequencedWorkerPool* - the original code in
CreateGCMDriverDesktop() was being too cautious. I don't think the object is
going to go away in the next 4 lines.

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:179:
scoped_refptr<base::SequencedWorkerPool> worker_pool(
Same.

[Jitu( very slow this week), 2015-09-03 05:31:36]

Fixed the comments

PTAL !

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1167:
scoped_refptr<base::SequencedWorkerPool> worker_pool(
On 2015/09/03 04:53:26, Lei Zhang wrote:
> This can just be a base::SequencedWorkerPool* - the original code in
> CreateGCMDriverDesktop() was being too cautious. I don't think the object is
> going to go away in the next 4 lines.

Done.

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:179:
scoped_refptr<base::SequencedWorkerPool> worker_pool(
On 2015/09/03 04:53:27, Lei Zhang wrote:
> Same.

Done.

[Lei Zhang, 2015-09-03 05:56:34]

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1325063002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1167:
scoped_refptr<base::SequencedWorkerPool> worker_pool(
On 2015/09/03 05:31:36, jitu wrote:
> On 2015/09/03 04:53:26, Lei Zhang wrote:
> > This can just be a base::SequencedWorkerPool* - the original code in
> > CreateGCMDriverDesktop() was being too cautious. I don't think the object is
> > going to go away in the next 4 lines.
> 
> Done.

Slightly more natural to write:

Foo* foo = Bar()

instead of:

Foo* foo(Bar);

[Jitu( very slow this week), 2015-09-03 06:37:19]

Thanks 

Changes done ...

PTAL !

[Lei Zhang, 2015-09-03 06:41:45]

still lgtm

[Jitu( very slow this week), 2015-09-03 06:57:25]

The CQ bit was checked by jitendra.ks@samsung.com

[Jitu( very slow this week), 2015-09-03 06:57:25]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1325063002/#ps60001
(title: "fix")

[commit-bot: I haz the power, 2015-09-03 06:57:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1325063002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1325063002/60001

[commit-bot: I haz the power, 2015-09-03 07:53:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-03 07:54:01]

Patchset 4 (id:??) landed as
https://crrev.com/ae5abc9a10333564b9e30c5fc1013f97e19ff4e0
Cr-Commit-Position: refs/heads/master@{#347128}

[pneubeck (no reviews), 2015-09-03 10:09:19]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1328573003/ by pneubeck@chromium.org.

The reason for reverting is: Broke
https://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20Clobber/bui....

[Jitu( very slow this week), 2015-09-03 12:00:41]

On 2015/09/03 10:09:19, pneubeck wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/1328573003/ by mailto:pneubeck@chromium.org.
> 
> The reason for reverting is: Broke
>
https://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20Clobber/bui....

Dear Droger,

Please help me to understand the reason of revert of this patch.

As i checked the mention linked 
https://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20Clobber/bui...

I found the error due to multiple definition for RealModelTypeToObjectId()

FAILED: /mnt/data/b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings
-m64 -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -pthread
-B../../third_party/binutils/Linux_x64/Release/bin -fuse-ld=gold -Wl,--icf=all
-Wl,-O1 -Wl,--gc-sections -Wl,--as-needed -Wl,-rpath=\$ORIGIN/ -Wl,-rpath-link=
-Wl,--disable-new-dtags -Wl,--export-dynamic -o ./sync_listen_notifications
-Wl,--start-group @./sync_listen_notifications.rsp  -Wl,--end-group  -ldl
-lgmodule-2.0 -lgobject-2.0 -lgthread-2.0 -lrt -lglib-2.0 -lnss3 -lnssutil3
-lsmime3 -lplds4 -lplc4 -lnspr4 -lgconf-2 -lresolv -lgio-2.0 -lfontconfig
-lfreetype -lpangocairo-1.0 -lpango-1.0 -lcairo -lXss -lX11 -lXcomposite
-lXcursor -lXdamage -lXext -lXfixes -lXi -lXrender -lXtst -lXrandr -latk-1.0 -lm
-lz -lasound -lexpat -ldbus-1 -lpthread -lcups -lgssapi_krb5 -lkrb5 -lk5crypto
-lcom_err -lgcrypt -lcrypt 
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/components/sync_driver/sync_driver/invalidation_helper.o: multiple
definition of 'syncer::RealModelTypeToObjectId(syncer::ModelType,
invalidation::ObjectId*)'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/sync/tools/common/invalidation_helper.o: previous definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/components/sync_driver/sync_driver/invalidation_helper.o: multiple
definition of
'syncer::ModelTypeSetToObjectIdSet(syncer::EnumSet<syncer::ModelType,
(syncer::ModelType)2, (syncer::ModelType)35>)'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/sync/tools/common/invalidation_helper.o: previous definition here


But those errors are not related to this patch.

Thanks

[droger, 2015-09-03 12:39:32]

zea: it looks like
components/sync_driver/invalidation_helper.cc
sync/tools/invalidation_helper.cc
both define a function with the same name and same namespace (syncer::).

Can we just change components/sync_driver/invalidation_helper.cc to be in the
sync_driver namespace instead of syncer?

[Nicolas Zea, 2015-09-03 16:10:45]

On 2015/09/03 12:39:32, droger wrote:
> zea: it looks like
> components/sync_driver/invalidation_helper.cc
> sync/tools/invalidation_helper.cc
> both define a function with the same name and same namespace (syncer::).
> 
> Can we just change components/sync_driver/invalidation_helper.cc to be in the
> sync_driver namespace instead of syncer?

yep, that's probably the right approach.

[blundell, 2015-09-03 16:11:35]

On 2015/09/03 16:10:45, Nicolas Zea wrote:
> On 2015/09/03 12:39:32, droger wrote:
> > zea: it looks like
> > components/sync_driver/invalidation_helper.cc
> > sync/tools/invalidation_helper.cc
> > both define a function with the same name and same namespace (syncer::).
> > 
> > Can we just change components/sync_driver/invalidation_helper.cc to be in
the
> > sync_driver namespace instead of syncer?
> 
> yep, that's probably the right approach.

(on a very quick examination, the code looks identical...)

[Nicolas Zea, 2015-09-03 16:20:35]

On 2015/09/03 16:11:35, blundell wrote:
> On 2015/09/03 16:10:45, Nicolas Zea wrote:
> > On 2015/09/03 12:39:32, droger wrote:
> > > zea: it looks like
> > > components/sync_driver/invalidation_helper.cc
> > > sync/tools/invalidation_helper.cc
> > > both define a function with the same name and same namespace (syncer::).
> > > 
> > > Can we just change components/sync_driver/invalidation_helper.cc to be in
> the
> > > sync_driver namespace instead of syncer?
> > 
> > yep, that's probably the right approach.
> 
> (on a very quick examination, the code looks identical...)

Ah, if the code is identical, then the one in sync/tools should be moved to
sync/util, which can then be included from anywhere.

[droger, 2015-09-03 16:26:18]

On 2015/09/03 16:20:35, Nicolas Zea wrote:
> On 2015/09/03 16:11:35, blundell wrote:
> > On 2015/09/03 16:10:45, Nicolas Zea wrote:
> > > On 2015/09/03 12:39:32, droger wrote:
> > > > zea: it looks like
> > > > components/sync_driver/invalidation_helper.cc
> > > > sync/tools/invalidation_helper.cc
> > > > both define a function with the same name and same namespace (syncer::).
> > > > 
> > > > Can we just change components/sync_driver/invalidation_helper.cc to be
in
> > the
> > > > sync_driver namespace instead of syncer?
> > > 
> > > yep, that's probably the right approach.
> > 
> > (on a very quick examination, the code looks identical...)
> 
> Ah, if the code is identical, then the one in sync/tools should be moved to
> sync/util, which can then be included from anywhere.

Thank you all.

I suggest doing this move to sync/util in a separate CL and then reland this CL.

[Jitu( very slow this week), 2015-09-22 17:37:43]

The CQ bit was checked by jitendra.ks@samsung.com

[commit-bot: I haz the power, 2015-09-22 17:37:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1325063002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1325063002/60001

[commit-bot: I haz the power, 2015-09-22 17:43:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-22 17:43:58]

Patchset 4 (id:??) landed as
https://crrev.com/b1b7fee052d510b179b5af2a55c42cc1f6f9b806
Cr-Commit-Position: refs/heads/master@{#350184}