Move bookmarks/core/... to bookmarks/...

Move bookmarks/core/... to bookmarks/...

Bookmarks is not a layered component, so there is no need for the two level deep
hierarchy, switch code from components/bookmarks/core/... to components/bookmarks/...

BUG=370434
TEST=components_unittests --gtest_filter=Bookmark*
R=blundell@chromium.org,sdefresne@chromium.org
TBR=thakis@chromium.org

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

[blundell, 2014-05-14 09:06:57]

Sylvain,

Could you review this CL? I'll then do a quick runthrough over //components and
we can TBR someone for //chrome OWNERS.

[sdefresne, 2014-05-14 12:27:21]

It L-G-T-M with the target name change that I pointed (don't forget to change
the references to those targets too).

https://codereview.chromium.org/284893003/diff/1/components/bookmarks.gypi
File components/bookmarks.gypi (left):

https://codereview.chromium.org/284893003/diff/1/components/bookmarks.gypi#ol...
components/bookmarks.gypi:8: 'target_name': 'bookmarks_core_browser',
Please also rename this target to 'bookmarks_browser'.

https://codereview.chromium.org/284893003/diff/1/components/bookmarks.gypi#ol...
components/bookmarks.gypi:62: 'target_name': 'bookmarks_core_common',
Please also rename this target to 'bookmarks_common'.

https://codereview.chromium.org/284893003/diff/1/components/bookmarks.gypi#ol...
components/bookmarks.gypi:78: 'target_name': 'bookmarks_core_test_support',
Please also rename this target to 'bookmarks_test_support'.

[nshaik, 2014-05-14 14:35:49]

https://codereview.chromium.org/284893003/diff/1/components/bookmarks.gypi
File components/bookmarks.gypi (left):

https://codereview.chromium.org/284893003/diff/1/components/bookmarks.gypi#ol...
components/bookmarks.gypi:8: 'target_name': 'bookmarks_core_browser',
On 2014/05/14 12:27:23, sdefresne wrote:
> Please also rename this target to 'bookmarks_browser'.

Done.

https://codereview.chromium.org/284893003/diff/1/components/bookmarks.gypi#ol...
components/bookmarks.gypi:62: 'target_name': 'bookmarks_core_common',
On 2014/05/14 12:27:23, sdefresne wrote:
> Please also rename this target to 'bookmarks_common'.

Done.

https://codereview.chromium.org/284893003/diff/1/components/bookmarks.gypi#ol...
components/bookmarks.gypi:78: 'target_name': 'bookmarks_core_test_support',
On 2014/05/14 12:27:23, sdefresne wrote:
> Please also rename this target to 'bookmarks_test_support'.

Done.

[sdefresne, 2014-05-14 15:58:55]

lgtm

[blundell, 2014-05-15 07:43:32]

LGTM

You can TBR thakis for the //chrome changes.

[nshaik, 2014-05-15 14:10:24]

The CQ bit was checked by naiem.shaik@gmail.com

[commit-bot: I haz the power, 2014-05-15 14:10:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/284893003/40001

[blundell, 2014-05-15 14:26:33]

Note that to TBR, you should do the following:

1) Add TBR=thakis in the CL description below the BUG= line.
2) Add thakis to the reviewers.
3) Send out a mail saying something like "TBR=thakis for //chrome" so that he
sees the review.

Thanks,

Colin

[commit-bot: I haz the power, 2014-05-15 14:41:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-15 14:46:57]

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

[commit-bot: I haz the power, 2014-05-15 14:46:58]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[nshaik, 2014-05-15 15:52:00]

Thakis, please review.

[nshaik, 2014-05-15 15:52:07]

Thakis, please review.

[nshaik, 2014-05-15 15:53:23]

On 2014/05/15 14:26:33, blundell wrote:
> Note that to TBR, you should do the following:
> 
> 1) Add TBR=thakis in the CL description below the BUG= line.
> 2) Add thakis to the reviewers.
> 3) Send out a mail saying something like "TBR=thakis for //chrome" so that he
> sees the review.
> 
> Thanks,
> 
> Colin

Hi Colin,
Done. will keep this in mind from next time

[nshaik, 2014-05-15 15:53:28]

On 2014/05/15 14:26:33, blundell wrote:
> Note that to TBR, you should do the following:
> 
> 1) Add TBR=thakis in the CL description below the BUG= line.
> 2) Add thakis to the reviewers.
> 3) Send out a mail saying something like "TBR=thakis for //chrome" so that he
> sees the review.
> 
> Thanks,
> 
> Colin

Hi Colin,
Done. will keep this in mind from next time

[blundell, 2014-05-15 15:54:46]

TBR=thakis for //chrome

(Naiem: Apologies, this is an example of what I had meant)

[tfarina, 2014-05-16 23:54:24]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-05-16 23:55:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/284893003/60001

[commit-bot: I haz the power, 2014-05-17 05:58:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-17 06:04:32]

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

[commit-bot: I haz the power, 2014-05-17 06:04:32]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[nshaik, 2014-05-17 23:02:44]

On 2014/05/17 06:04:32, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
>   ios_rel_device on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
>   ios_rel_device_ninja on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
>   linux_chromium_chromeos_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
>   linux_chromium_clang_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
>   linux_chromium_gn_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
>   linux_chromium_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

I did a fetch android and found that, the below mentioned change is missing in
the android tree.
due to that applying this patch is failing for android builds..
https://src.chromium.org/viewvc/chrome?revision=266563&view=revision

[tfarina, 2014-05-17 23:31:06]

On Saturday, May 17, 2014, <naiem.shaik@gmail.com> wrote:

> On 2014/05/17 06:04:32, I haz the power (commit-bot) wrote:
>
>> Try jobs failed on following builders:
>>    android_chromium_gn_compile_rel on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> android_chromium_gn_compile_rel/builds/5172)
>
>>    ios_rel_device on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> ios_rel_device/builds/142611)
>
>>    ios_rel_device_ninja on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> ios_rel_device_ninja/builds/17127)
>
>>    linux_chromium_chromeos_rel on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromium_chromeos_rel/builds/28096)
>
>>    linux_chromium_clang_dbg on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromium_clang_dbg/builds/26137)
>
>>    linux_chromium_gn_rel on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromium_gn_rel/builds/8013)
>
>>    linux_chromium_rel on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromium_rel/builds/32618)
>
> I did a fetch android and found that, the below mentioned change is
> missing in
> the android tree.

Sorry, I don't understand what that has to be with your patch failing to
apply. We are in the Chromium tree not in Android.

Please, could you update your checkout and reupload?

Thanks,

Btw, did you build this locally both on Linux and with
GYP_DEFINES="OS=android"?

due to that applying this patch is failing for android builds..
> https://src.chromium.org/viewvc/chrome?revision=266563&view=revision
>
> https://codereview.chromium.org/284893003/
>


-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nshaik, 2014-05-18 02:31:20]

On 2014/05/17 23:31:06, tfarina wrote:
> On Saturday, May 17, 2014, <mailto:naiem.shaik@gmail.com> wrote:
> 
> > On 2014/05/17 06:04:32, I haz the power (commit-bot) wrote:
> >
> >> Try jobs failed on following builders:
> >>    android_chromium_gn_compile_rel on tryserver.chromium
> >>
> >
> > (http://build.chromium.org/p/tryserver.chromium/builders/
> > android_chromium_gn_compile_rel/builds/5172)
> >
> >>    ios_rel_device on tryserver.chromium
> >>
> >
> > (http://build.chromium.org/p/tryserver.chromium/builders/
> > ios_rel_device/builds/142611)
> >
> >>    ios_rel_device_ninja on tryserver.chromium
> >>
> >
> > (http://build.chromium.org/p/tryserver.chromium/builders/
> > ios_rel_device_ninja/builds/17127)
> >
> >>    linux_chromium_chromeos_rel on tryserver.chromium
> >>
> >
> > (http://build.chromium.org/p/tryserver.chromium/builders/
> > linux_chromium_chromeos_rel/builds/28096)
> >
> >>    linux_chromium_clang_dbg on tryserver.chromium
> >>
> >
> > (http://build.chromium.org/p/tryserver.chromium/builders/
> > linux_chromium_clang_dbg/builds/26137)
> >
> >>    linux_chromium_gn_rel on tryserver.chromium
> >>
> >
> > (http://build.chromium.org/p/tryserver.chromium/builders/
> > linux_chromium_gn_rel/builds/8013)
> >
> >>    linux_chromium_rel on tryserver.chromium
> >>
> >
> > (http://build.chromium.org/p/tryserver.chromium/builders/
> > linux_chromium_rel/builds/32618)
> >
> > I did a fetch android and found that, the below mentioned change is
> > missing in
> > the android tree.
> 
> Sorry, I don't understand what that has to be with your patch failing to
> apply. We are in the Chromium tree not in Android.
> 
> Please, could you update your checkout and reupload?
> 
> Thanks,
> 
> Btw, did you build this locally both on Linux and with
> GYP_DEFINES="OS=android"?
> 
> due to that applying this patch is failing for android builds..
> > https://src.chromium.org/viewvc/chrome?revision=266563&view=revision
> >
> > https://codereview.chromium.org/284893003/
> >
> 
> 
> -- 
> Thiago Farina
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Sorry, if I was not clear.
I saw that the commit-bot has failed
http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...
So in order to reproduce the build failure, I made a fresh copy of Android tree
and when I applied the patch I found it does not apply cleanly.
The reason for the failure is the Android tree is missing the commit I mentioned
earlier.

The patch applies cleanly on tip of Chromium tree, I already verified it.
Kindly let me know if there is anything I can do to unblock this.

Thanks,
Naiem

[tfarina, 2014-05-18 03:38:52]

On Sat, May 17, 2014 at 11:31 PM, <naiem.shaik@gmail.com> wrote:

> Sorry, if I was not clear.
> I saw that the commit-bot has failed
> http://build.chromium.org/p/tryserver.chromium/builders/
> android_chromium_gn_compile_rel/builds/5172/steps/steps/logs/stdio

That is not the Android tree. That is the GN build (rather than GYP) for
Android on Chromium.


> So in order to reproduce the build failure, I made a fresh copy of Android
> tree
> and when I applied the patch I found it does not apply cleanly.
> The reason for the failure is the Android tree is missing the commit I
> mentioned
> earlier.


>
The patch applies cleanly on tip of Chromium tree, I already verified it.
>
Kindly let me know if there is anything I can do to unblock this.
>
Yes, please, sync your branch to ToT and reupload so we can push this to CQ
again.

-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nshaik, 2014-05-18 06:12:36]

On 2014/05/18 03:38:52, tfarina wrote:
> On Sat, May 17, 2014 at 11:31 PM, <mailto:naiem.shaik@gmail.com> wrote:
> 
> > Sorry, if I was not clear.
> > I saw that the commit-bot has failed
> > http://build.chromium.org/p/tryserver.chromium/builders/
> > android_chromium_gn_compile_rel/builds/5172/steps/steps/logs/stdio
> 
> That is not the Android tree. That is the GN build (rather than GYP) for
> Android on Chromium.
> 
> 
> > So in order to reproduce the build failure, I made a fresh copy of Android
> > tree
> > and when I applied the patch I found it does not apply cleanly.
> > The reason for the failure is the Android tree is missing the commit I
> > mentioned
> > earlier.
> 
> 
> >
> The patch applies cleanly on tip of Chromium tree, I already verified it.
> >
> Kindly let me know if there is anything I can do to unblock this.
> >
> Yes, please, sync your branch to ToT and reupload so we can push this to CQ
> again.
> 
> -- 
> Thiago Farina
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Done. Can you please review and push to CQ

[tfarina, 2014-05-18 21:21:14]

The CQ bit was checked by tfarina@chromium.org

[tfarina, 2014-05-18 21:21:14]

The CQ bit was unchecked by tfarina@chromium.org

[tfarina, 2014-05-18 21:21:15]

The CQ bit was unchecked by tfarina@chromium.org

[tfarina, 2014-05-18 21:21:15]

The CQ bit was checked by tfarina@chromium.org

[tfarina, 2014-05-18 21:21:15]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-05-18 21:21:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/284893003/80001

[tfarina, 2014-05-18 21:23:47]

https://codereview.chromium.org/284893003/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_expanded_state_tracker_unittest.cc
(right):

https://codereview.chromium.org/284893003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_expanded_state_tracker_unittest.cc:5: #include
"components/bookmarks/browser/bookmark_expanded_state_tracker.h"
Your checkout is still not up to date. 

I have already moved it to components.

https://chromium.googlesource.com/chromium/chromium/+/trunk/components/bookma...

[tfarina, 2014-05-18 21:24:02]

The CQ bit was unchecked by tfarina@chromium.org

[tfarina, 2014-05-18 21:27:26]

I also see that there are other unit tests in chrome/browser/bookmarks that have
been moved to components/ already.

Do you need some guidance to update your branch? Please, let me know if I can
help somehow.

[nshaik, 2014-05-20 00:24:59]

On 2014/05/18 21:27:26, tfarina wrote:
> I also see that there are other unit tests in chrome/browser/bookmarks that
have
> been moved to components/ already.
> 
> Do you need some guidance to update your branch? Please, let me know if I can
> help somehow.

Can you please review now.

Thanks.

[tfarina, 2014-05-20 04:53:21]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-05-20 04:53:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/284893003/100001

[commit-bot: I haz the power, 2014-05-20 09:54:42]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-20 10:14:48]

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

[commit-bot: I haz the power, 2014-05-20 10:14:48]

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

[blundell, 2014-05-20 11:05:48]

Looks like you have a couple > 80 character lines to fix up.

[nshaik, 2014-05-20 13:47:06]

The CQ bit was checked by naiem.shaik@gmail.com

[commit-bot: I haz the power, 2014-05-20 13:48:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/284893003/120001

[commit-bot: I haz the power, 2014-05-20 17:16:12]

Change committed as 271675