[Enhanced Bookmarks] Make enhanced bookmarks default for Android.

[Enhanced Bookmarks] Make enhanced bookmarks default for Android.

Finally we have decided to turn on by default.

BUG=502911
Committed: https://crrev.com/a57e4ebe00912cd4db9990969f86c1ba69214ed9
Cr-Commit-Position: refs/heads/master@{#367422}

[Kibeom Kim (inactive), 2015-12-03 19:21:58]

The CQ bit was checked by kkimlabs@chromium.org to run a CQ dry run

[Kibeom Kim (inactive), 2015-12-03 19:22:08]

Description was changed from

==========
[Enhanced Bookmarks] Make enhanced bookmarks default for Android.

Finally we have decided to turn on by default.

BUG=502911
==========

to

==========
[Enhanced Bookmarks] Make enhanced bookmarks default for Android.

Finally we have decided to turn on by default.

BUG=502911
==========

[commit-bot: I haz the power, 2015-12-03 19:22:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496953003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496953003/20001

[Kibeom Kim (inactive), 2015-12-03 21:15:40]

kkimlabs@chromium.org changed reviewers:
+ fgorski@chromium.org, jianli@chromium.org, newt@chromium.org

[Kibeom Kim (inactive), 2015-12-03 21:20:41]

kkimlabs@chromium.org changed reviewers:
+ ianwen@chromium.org

[jianli, 2015-12-03 23:30:04]

offline_pages lgtm

https://codereview.chromium.org/1496953003/diff/20001/components/enhanced_boo...
File components/enhanced_bookmarks/enhanced_bookmark_features.cc (right):

https://codereview.chromium.org/1496953003/diff/20001/components/enhanced_boo...
components/enhanced_bookmarks/enhanced_bookmark_features.cc:11: #include
"components/offline_pages/offline_page_feature.h"
nit: you can include this include

[jianli, 2015-12-03 23:30:38]

https://codereview.chromium.org/1496953003/diff/20001/components/enhanced_boo...
File components/enhanced_bookmarks/enhanced_bookmark_features.cc (right):

https://codereview.chromium.org/1496953003/diff/20001/components/enhanced_boo...
components/enhanced_bookmarks/enhanced_bookmark_features.cc:11: #include
"components/offline_pages/offline_page_feature.h"
On 2015/12/03 23:30:04, jianli wrote:
> nit: you can include this include

sorry, I mean to remove this include

[Kibeom Kim (inactive), 2015-12-03 23:36:39]

https://codereview.chromium.org/1496953003/diff/20001/components/enhanced_boo...
File components/enhanced_bookmarks/enhanced_bookmark_features.cc (right):

https://codereview.chromium.org/1496953003/diff/20001/components/enhanced_boo...
components/enhanced_bookmarks/enhanced_bookmark_features.cc:11: #include
"components/offline_pages/offline_page_feature.h"
On 2015/12/03 23:30:38, jianli wrote:
> On 2015/12/03 23:30:04, jianli wrote:
> > nit: you can include this include
> 
> sorry, I mean to remove this include

Done.

[newt (away), 2015-12-04 00:09:43]

At last!!

A few more things need removing. Then lgtm.

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
(right):

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:256:
public static void showEnhancedBookmarkIfEnabled(Activity activity) {
remove "IfEnabled". I'd also add an "s" so it's "showEnhancedBookmarks()"

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NativePageFactory.java
(right):

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NativePageFactory.java:44:
// TODO(kkimlabs): Remove BookmarksPage completely. http://crbug.com/502911
woohoo!

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:167:
public static void launchBookmarksDialog(Activity activity, Tab tab,
remove unneeded parameters from this method

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:670:
private static class BookmarkDialogSelectedListener implements
BookmarkSelectedListener {
delete BookmarkDialogSelectedListener

[Kibeom Kim (inactive), 2015-12-04 00:32:33]

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
(right):

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:256:
public static void showEnhancedBookmarkIfEnabled(Activity activity) {
On 2015/12/04 00:09:43, newt wrote:
> remove "IfEnabled". I'd also add an "s" so it's "showEnhancedBookmarks()"

Done.

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NativePageFactory.java
(right):

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NativePageFactory.java:44:
// TODO(kkimlabs): Remove BookmarksPage completely. http://crbug.com/502911
On 2015/12/04 00:09:43, newt wrote:
> woohoo!

Not Done. (yet)

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:167:
public static void launchBookmarksDialog(Activity activity, Tab tab,
On 2015/12/04 00:09:43, newt wrote:
> remove unneeded parameters from this method

Done.

https://codereview.chromium.org/1496953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:670:
private static class BookmarkDialogSelectedListener implements
BookmarkSelectedListener {
On 2015/12/04 00:09:43, newt wrote:
> delete BookmarkDialogSelectedListener

Done.

[Ian Wen, 2015-12-04 04:28:58]

Woohoo!

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1051:
EnhancedBookmarkUtils.addOrEditBookmark(bookmarkId, bookmarkModel,
Use runAfterModelLoaded in BookmarksBridge and remove the unnecessary observer
here.

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
(right):

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:256:
public static void showEnhancedBookmarks(Activity activity) {
Rename to showBookmarkManager() or startBookmarkManager()?

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/BookmarksPage.java
(left):

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/BookmarksPage.java:75:
public interface BookmarkSelectedListener {
Why do we need to modify this class? Isn't this file suppose to be deleted
altogether?

https://codereview.chromium.org/1496953003/diff/60001/components/enhanced_boo...
File components/enhanced_bookmarks/enhanced_bookmark_features.h (left):

https://codereview.chromium.org/1496953003/diff/60001/components/enhanced_boo...
components/enhanced_bookmarks/enhanced_bookmark_features.h:10: #if
defined(OS_IOS) || defined(OS_ANDROID)
Shouldn't we retain defined(OS_IOS)?

[Kibeom Kim (inactive), 2015-12-04 21:07:46]

The CQ bit was checked by kkimlabs@chromium.org to run a CQ dry run

[Kibeom Kim (inactive), 2015-12-04 21:07:52]

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1051:
EnhancedBookmarkUtils.addOrEditBookmark(bookmarkId, bookmarkModel,
On 2015/12/04 04:28:58, Ian Wen wrote:
> Use runAfterModelLoaded in BookmarksBridge and remove the unnecessary observer
> here.

Done.

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
(right):

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:256:
public static void showEnhancedBookmarks(Activity activity) {
On 2015/12/04 04:28:58, Ian Wen wrote:
> Rename to showBookmarkManager() or startBookmarkManager()?

Done.

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/BookmarksPage.java
(left):

https://codereview.chromium.org/1496953003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/BookmarksPage.java:75:
public interface BookmarkSelectedListener {
On 2015/12/04 04:28:58, Ian Wen wrote:
> Why do we need to modify this class? Isn't this file suppose to be deleted
> altogether?

Yes, eventually we should delete BookmarksPage.java. IIRC, the current blocker
is migrating the existing bookmark tab to enhanced http://crbug.com/474719

https://codereview.chromium.org/1496953003/diff/60001/components/enhanced_boo...
File components/enhanced_bookmarks/enhanced_bookmark_features.h (left):

https://codereview.chromium.org/1496953003/diff/60001/components/enhanced_boo...
components/enhanced_bookmarks/enhanced_bookmark_features.h:10: #if
defined(OS_IOS) || defined(OS_ANDROID)
On 2015/12/04 04:28:58, Ian Wen wrote:
> Shouldn't we retain defined(OS_IOS)?

If an entire file for a single platform, I think a more common convention is
handling in gyp&gn, e.g., we don't have defined(OS_ANDROID) for .cc .h files for
Android.

I'll also rename this file and enhanced_bookmark_switches to ..._ios.h/cc

[commit-bot: I haz the power, 2015-12-04 21:08:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496953003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496953003/80001

[commit-bot: I haz the power, 2015-12-04 22:38:09]

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

[commit-bot: I haz the power, 2015-12-04 22:38:09]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Ian Wen, 2015-12-05 00:02:29]

lgtm after you update the gyp file in mac builder.

[Kibeom Kim (inactive), 2015-12-05 00:09:33]

kkimlabs@chromium.org changed reviewers:
+ droger@chromium.org

[Kibeom Kim (inactive), 2015-12-05 00:09:34]

droger@: ptal

[droger, 2015-12-07 08:55:33]

//ios LGTM

CC noyau FYI

[noyau (Ping after 24h), 2015-12-07 09:19:35]

noyau@chromium.org changed reviewers:
+ noyau@chromium.org

[noyau (Ping after 24h), 2015-12-07 09:19:35]

[+cc: stkhapugin]

In the CL the two files
components/enhanced_bookmarks/enhanced_bookmark_features.* are marked modified,
not deleted, they should probably be deleted.

stkhapugin, can you prepare a CL to remove those two _ios files once your CL
(https://codereview.chromium.org/1491253003/) lands?

[newt (away), 2015-12-07 19:53:58]

On 2015/12/07 09:19:35, noyau wrote:
> In the CL the two files
> components/enhanced_bookmarks/enhanced_bookmark_features.* are marked
modified,
> not deleted, they should probably be deleted.

The files are indeed deleted. It's a codereview bug that rietveld sometimes
shows deleted files as modified.

[stkhapugin, 2015-12-08 10:24:50]

On 2015/12/07 19:53:58, newt wrote:
> On 2015/12/07 09:19:35, noyau wrote:
> > In the CL the two files
> > components/enhanced_bookmarks/enhanced_bookmark_features.* are marked
> modified,
> > not deleted, they should probably be deleted.
> 
> The files are indeed deleted. It's a codereview bug that rietveld sometimes
> shows deleted files as modified.

enhanced_bookmark_features.{h|cc} is not used on iOS since
this CL: https://codereview.chromium.org/1491253003/ landed. Please feel free to
remove them completely. We need enhanced_bookmark_switches_ios.{h|cc} though,
and I bet the trybot is failing because we now use the (...)_switches in
ios/chrome/browser/experimental_flags.mm instead of (...)_features.

Please refer to mentioned CL for details, or just remove features_ios and
keep only flags_ios, plus update the include in experimental_flags.mm

Thank you!

[Ian Wen, 2015-12-16 06:09:31]

Ping?

[Kibeom Kim (inactive), 2015-12-16 06:19:00]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-12-16 06:19:00]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org,
newt@chromium.org, ianwen@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1496953003/#ps100001 (title: "ios/mac
file update")

[Kibeom Kim (inactive), 2015-12-16 06:19:10]

The CQ bit was unchecked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-12-16 06:29:39]

On 2015/12/16 06:09:31, Ian Wen wrote:
> Ping?

I'll update and land when I get back in Jan.

[Kibeom Kim (inactive), 2016-01-04 23:13:04]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2016-01-04 23:13:04]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
newt@chromium.org, jianli@chromium.org, ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/1496953003/#ps120001
(title: "rebase & ios update")

[commit-bot: I haz the power, 2016-01-04 23:14:39]

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

[commit-bot: I haz the power, 2016-01-05 00:15:30]

Description was changed from

==========
[Enhanced Bookmarks] Make enhanced bookmarks default for Android.

Finally we have decided to turn on by default.

BUG=502911
==========

to

==========
[Enhanced Bookmarks] Make enhanced bookmarks default for Android.

Finally we have decided to turn on by default.

BUG=502911
==========

[commit-bot: I haz the power, 2016-01-05 00:15:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-05 00:16:20]

Description was changed from

==========
[Enhanced Bookmarks] Make enhanced bookmarks default for Android.

Finally we have decided to turn on by default.

BUG=502911
==========

to

==========
[Enhanced Bookmarks] Make enhanced bookmarks default for Android.

Finally we have decided to turn on by default.

BUG=502911

Committed: https://crrev.com/a57e4ebe00912cd4db9990969f86c1ba69214ed9
Cr-Commit-Position: refs/heads/master@{#367422}
==========

[commit-bot: I haz the power, 2016-01-05 00:16:21]

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