Always add bookmarks to the mobile node on Android

Always add bookmarks to the mobile node on Android

bookmark_utils should always attach new bookmarks to the mobile
node on Android, rather than the most recently used one.
This is where the bookmark button already appends them to, and
it will make sure all added bookmarks are visible.

BUG=649420
Committed: https://crrev.com/c4a5b95f057d9b29ee066225b43c89557b5c2691
Cr-Commit-Position: refs/heads/master@{#424751}

[kraush, 2016-09-22 18:28:28]

kraush@amazon.com changed reviewers:
+ sky@chromium.org

[kraush, 2016-09-22 18:28:29]

Hi sky,

Can you take a look at this small API change for bookmarks on Android?

Thanks! :)
Holger

[sky, 2016-09-30 15:56:38]

It seems reasonable to me to default to the mobile node on android when there
are no bookmarks, but if there other bookmarks then newly created ones should go
to the most recent folder.

[kraush, 2016-09-30 16:04:08]

On 2016/09/30 15:56:38, sky wrote:
> It seems reasonable to me to default to the mobile node on android when there
> are no bookmarks, but if there other bookmarks then newly created ones should
go
> to the most recent folder.

That makes total sense, good point!
Would it seem feasible to alter
BookmarkUtils::GetMostRecentlyModifiedUserFolders() to check the size at the
very end on Android, and return the mobile node in case size equals 1? (The code
is crawling from the root node and always including it, so a size of one would
guarantee that it's always only the root node.

[sky, 2016-09-30 16:56:19]

I suggest checking for an empty model (discounting the permanent
nodes) and if empty than return mobile (on android).

On Fri, Sep 30, 2016 at 9:04 AM, kraush@amazon.com via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
wrote:
> On 2016/09/30 15:56:38, sky wrote:
>> It seems reasonable to me to default to the mobile node on android when
>> there
>> are no bookmarks, but if there other bookmarks then newly created ones
>> should
> go
>> to the most recent folder.
>
> That makes total sense, good point!
> Would it seem feasible to alter
> BookmarkUtils::GetMostRecentlyModifiedUserFolders() to check the size at the
> very end on Android, and return the mobile node in case size equals 1? (The
> code
> is crawling from the root node and always including it, so a size of one
> would
> guarantee that it's always only the root node.
>
> https://codereview.chromium.org/2367533003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kraush, 2016-09-30 17:58:58]

On 2016/09/30 16:56:19, sky wrote:
> I suggest checking for an empty model (discounting the permanent
> nodes) and if empty than return mobile (on android).
> 
> On Fri, Sep 30, 2016 at 9:04 AM, mailto:kraush@amazon.com via
> http://codereview.chromium.org
<mailto:reply@chromiumcodereview-hr.appspotmail.com>
> wrote:
> > On 2016/09/30 15:56:38, sky wrote:
> >> It seems reasonable to me to default to the mobile node on android when
> >> there
> >> are no bookmarks, but if there other bookmarks then newly created ones
> >> should
> > go
> >> to the most recent folder.
> >
> > That makes total sense, good point!
> > Would it seem feasible to alter
> > BookmarkUtils::GetMostRecentlyModifiedUserFolders() to check the size at the
> > very end on Android, and return the mobile node in case size equals 1? (The
> > code
> > is crawling from the root node and always including it, so a size of one
> > would
> > guarantee that it's always only the root node.
> >
> > https://codereview.chromium.org/2367533003/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

Gotcha, thanks, will do!
Since there is no check for "is empty model / only has root node", I'll put this
into GetMostRecentlyModifiedUserFolders() right before the interaction with
permanent nodes and verify size == 1 there.

Or would you rather have me implement a separate loop over all nodes in
bookmark_model, checking if there are any non-permanent nodes?

[sky, 2016-09-30 18:08:02]

In looking at the code a bit more I don't think GetParentForNewNodes()
should be part of the model. It should move to bookmark_utils. And
that function should have the #ifdef for android, and it should
explicitly check for empty model.

  -Scott

On Fri, Sep 30, 2016 at 10:58 AM, kraush@amazon.com via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
wrote:
> On 2016/09/30 16:56:19, sky wrote:
>> I suggest checking for an empty model (discounting the permanent
>> nodes) and if empty than return mobile (on android).
>>
>> On Fri, Sep 30, 2016 at 9:04 AM, mailto:kraush@amazon.com via
>> http://codereview.chromium.org
> <mailto:reply@chromiumcodereview-hr.appspotmail.com>
>> wrote:
>> > On 2016/09/30 15:56:38, sky wrote:
>> >> It seems reasonable to me to default to the mobile node on android when
>> >> there
>> >> are no bookmarks, but if there other bookmarks then newly created ones
>> >> should
>> > go
>> >> to the most recent folder.
>> >
>> > That makes total sense, good point!
>> > Would it seem feasible to alter
>> > BookmarkUtils::GetMostRecentlyModifiedUserFolders() to check the size at
>> > the
>> > very end on Android, and return the mobile node in case size equals 1?
>> > (The
>> > code
>> > is crawling from the root node and always including it, so a size of one
>> > would
>> > guarantee that it's always only the root node.
>> >
>> > https://codereview.chromium.org/2367533003/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> Gotcha, thanks, will do!
> Since there is no check for "is empty model / only has root node", I'll put
> this
> into GetMostRecentlyModifiedUserFolders() right before the interaction with
> permanent nodes and verify size == 1 there.
>
> Or would you rather have me implement a separate loop over all nodes in
> bookmark_model, checking if there are any non-permanent nodes?
>
> https://codereview.chromium.org/2367533003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kraush, 2016-09-30 18:26:49]

On 2016/09/30 18:08:02, sky wrote:
> In looking at the code a bit more I don't think GetParentForNewNodes()
> should be part of the model. It should move to bookmark_utils. And
> that function should have the #ifdef for android, and it should
> explicitly check for empty model.
> 
>   -Scott

Sounds good, thanks Scott!
I'll refactor it into BookmarkUtils and add the check there.

[kraush, 2016-10-03 21:55:38]

Hi Scott,

I have refactored GetParentForNewNodes into BookmarkUtils as you suggested.
Also adjusted existing tests and added two new tests to test this functionality
correct on every platform.

Haven't checked the Desktop build yet - will kick off a dry-run to make sure I
didn't break anything :)

[kraush, 2016-10-03 21:56:06]

The CQ bit was checked by kraush@amazon.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 21:57:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 21:57:06]

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

[commit-bot: I haz the power, 2016-10-03 21:57:07]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[kraush, 2016-10-03 21:58:55]

On 2016/10/03 21:55:38, kraush wrote:
> Hi Scott,
> 
> I have refactored GetParentForNewNodes into BookmarkUtils as you suggested.
> Also adjusted existing tests and added two new tests to test this
functionality
> correct on every platform.
> 
> Haven't checked the Desktop build yet - will kick off a dry-run to make sure I
> didn't break anything :)

Got a message telling me I'm not allowed to kick off dry runs as a non-Googler
:(
I'll build a Linux build locally then.

[sky, 2016-10-10 21:02:25]

https://codereview.chromium.org/2367533003/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_utils.cc (right):

https://codereview.chromium.org/2367533003/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.cc:579: if (nodes[0] ==
model->bookmark_bar_node() && nodes[0]->child_count() == 0) {
Your check should be more of are all the permance nodes empty? If so, return the
mobile node.

https://codereview.chromium.org/2367533003/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_utils.h (right):

https://codereview.chromium.org/2367533003/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.h:165: // Returns the parent the
last node was added to. This never returns NULL
Returns the parent to add new nodes to, never returns null (as long as model is
loaded).

[kraush, 2016-10-10 22:18:05]

https://codereview.chromium.org/2367533003/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_utils.cc (right):

https://codereview.chromium.org/2367533003/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.cc:579: if (nodes[0] ==
model->bookmark_bar_node() && nodes[0]->child_count() == 0) {
On 2016/10/10 21:02:24, sky wrote:
> Your check should be more of are all the permance nodes empty? If so, return
the
> mobile node.

Will do!
I'll iterate over all the children of the root node and make sure they each have
0 children. (From my understanding, that would cover all the permanent nodes.
Let me know if that understanding is incorrect!)

https://codereview.chromium.org/2367533003/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_utils.h (right):

https://codereview.chromium.org/2367533003/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.h:165: // Returns the parent the
last node was added to. This never returns NULL
On 2016/10/10 21:02:24, sky wrote:
> Returns the parent to add new nodes to, never returns null (as long as model
is
> loaded).

Will change.

[kraush, 2016-10-11 14:55:12]

Fixed the comment as requested and implemented the check for Android as an "No
permanent node has children:.

Please take a look.

[sky, 2016-10-11 19:13:39]

https://codereview.chromium.org/2367533003/diff/40001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_utils.cc (right):

https://codereview.chromium.org/2367533003/diff/40001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.cc:572: bool
HasUserCreatedBookmarks(BookmarkModel* model) {
Move this to anonymous namespace.

https://codereview.chromium.org/2367533003/diff/40001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.cc:577: if (node->IsVisible() &&
model->client()->CanBeEditedByUser(node) &&
Can't you just check child_count() here?

https://codereview.chromium.org/2367533003/diff/40001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.cc:587: if
(!HasUserCreatedBookmarks(model)) {
no {} (see style guide)

[kraush, 2016-10-11 21:22:39]

https://codereview.chromium.org/2367533003/diff/40001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_utils.cc (right):

https://codereview.chromium.org/2367533003/diff/40001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.cc:572: bool
HasUserCreatedBookmarks(BookmarkModel* model) {
On 2016/10/11 19:13:39, sky wrote:
> Move this to anonymous namespace.

Will do.

https://codereview.chromium.org/2367533003/diff/40001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.cc:577: if (node->IsVisible() &&
model->client()->CanBeEditedByUser(node) &&
On 2016/10/11 19:13:39, sky wrote:
> Can't you just check child_count() here?

I wanted to be super-safe, but it's probably unnecessary. Will reduce to just
the count.

https://codereview.chromium.org/2367533003/diff/40001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.cc:587: if
(!HasUserCreatedBookmarks(model)) {
On 2016/10/11 19:13:39, sky wrote:
> no {} (see style guide)

Removed.

[kraush, 2016-10-11 23:42:20]

Removed curly braces, moved to anonymous namespace and simplified check.

[sky, 2016-10-12 00:01:28]

LGTM - thanks!

[kraush, 2016-10-12 00:02:15]

The CQ bit was checked by kraush@amazon.com

[kraush, 2016-10-12 00:02:35]

The CQ bit was unchecked by kraush@amazon.com

[kraush, 2016-10-12 00:04:08]

On 2016/10/12 00:01:28, sky wrote:
> LGTM - thanks!

On 2016/10/12 00:01:28, sky wrote:
> LGTM - thanks!

Argh, I just realized that I haven't rebuilt this for Linux after the last
refactor.
I think due to the anonymous namespace the compilation will fail due to an
unused method.

Gonna rebuild for Linux and update if needed.

[kraush, 2016-10-12 14:38:17]

On 2016/10/12 00:04:08, kraush wrote:
> On 2016/10/12 00:01:28, sky wrote:
> > LGTM - thanks!
> 
> On 2016/10/12 00:01:28, sky wrote:
> > LGTM - thanks!
> 
> Argh, I just realized that I haven't rebuilt this for Linux after the last
> refactor.
> I think due to the anonymous namespace the compilation will fail due to an
> unused method.
> 
> Gonna rebuild for Linux and update if needed.

Build passed locally despite the unused method - odd :)
Let's see if CQ agrees.

[kraush, 2016-10-12 14:38:21]

The CQ bit was checked by kraush@amazon.com

[commit-bot: I haz the power, 2016-10-12 14:38:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 14:58:07]

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

[commit-bot: I haz the power, 2016-10-12 14:58:08]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[kraush, 2016-10-12 15:05:00]

On 2016/10/12 14:58:08, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromeos_amd64-generic_chromium_compile_only_ng on
> master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

The expected error :(
bool bookmarks::{anonymous}::HasUserCreatedBookmarks(bookmarks::BookmarkModel*)'
defined but not used

No idea why this didn't trigger locally on Linux.
I'll put it into an ifdef and resubmit.

[kraush, 2016-10-12 15:10:20]

Wrapped into ifdef. Should work this time.

[kraush, 2016-10-12 15:10:40]

The CQ bit was checked by kraush@amazon.com

[kraush, 2016-10-12 15:10:40]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2367533003/#ps80001
(title: "Put new method into ifdef")

[commit-bot: I haz the power, 2016-10-12 15:10:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 15:57:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-12 16:00:00]

Description was changed from

==========
Always add bookmarks to the mobile node on Android

bookmark_utils should always attach new bookmarks to the mobile
node on Android, rather than the most recently used one.
This is where the bookmark button already appends them to, and
it will make sure all added bookmarks are visible.

BUG=649420
==========

to

==========
Always add bookmarks to the mobile node on Android

bookmark_utils should always attach new bookmarks to the mobile
node on Android, rather than the most recently used one.
This is where the bookmark button already appends them to, and
it will make sure all added bookmarks are visible.

BUG=649420

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

[commit-bot: I haz the power, 2016-10-12 16:00:01]

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

[Ian Wen, 2016-10-12 17:40:05]

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

[Ian Wen, 2016-10-12 17:40:06]

https://codereview.chromium.org/2367533003/diff/80001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_utils.cc (right):

https://codereview.chromium.org/2367533003/diff/80001/components/bookmarks/br...
components/bookmarks/browser/bookmark_utils.cc:589: if
(!HasUserCreatedBookmarks(model))
This is a bit confusing because even if it is labeled as Android, it is actually
never used by Chrome on Android. Amazon android browser will be the only party
that may execute this function.

In Chrome on Android similar logic is written in Java.

[sky, 2016-10-12 19:26:56]

On 2016/10/12 17:40:06, Ian Wen wrote:
>
https://codereview.chromium.org/2367533003/diff/80001/components/bookmarks/br...
> File components/bookmarks/browser/bookmark_utils.cc (right):
> 
>
https://codereview.chromium.org/2367533003/diff/80001/components/bookmarks/br...
> components/bookmarks/browser/bookmark_utils.cc:589: if
> (!HasUserCreatedBookmarks(model))
> This is a bit confusing because even if it is labeled as Android, it is
actually
> never used by Chrome on Android. Amazon android browser will be the only party
> that may execute this function.
> 
> In Chrome on Android similar logic is written in Java.

Ian, are you saying this function is never called by Android? If so, we should
revert this change.

[kraush, 2016-10-12 19:42:01]

On 2016/10/12 19:26:56, sky wrote:
> On 2016/10/12 17:40:06, Ian Wen wrote:
> >
>
https://codereview.chromium.org/2367533003/diff/80001/components/bookmarks/br...
> > File components/bookmarks/browser/bookmark_utils.cc (right):
> > 
> >
>
https://codereview.chromium.org/2367533003/diff/80001/components/bookmarks/br...
> > components/bookmarks/browser/bookmark_utils.cc:589: if
> > (!HasUserCreatedBookmarks(model))
> > This is a bit confusing because even if it is labeled as Android, it is
> actually
> > never used by Chrome on Android. Amazon android browser will be the only
party
> > that may execute this function.
> > 
> > In Chrome on Android similar logic is written in Java.
> 
> Ian, are you saying this function is never called by Android? If so, we should
> revert this change.

It's not used by Chromium on Android.
However, isn't the components API meant to be used by multiple embedders, not
just Chromium?
If that's not the case, should the whole AddIfNotBookmarked method be in an
ifdef(!ANDROID) so Android users can't use it by accident and get unexpected
results?

Let me know either way and I'll make the change. However even if this is not
meant to be used by anyone but Chrome, I'd rather ifdef the whole method out as
part of reverting this change - having a method that does not perform as
expected seems bad in case someone decides to use it in ChromePublic in the
future.

[sky, 2016-10-12 20:17:06]

AddIfNotBookmarked is only used in chrome, so I'm equally ok with
moving the code entirely there, and in a place that is not compiled in
android.

  -Scott

On Wed, Oct 12, 2016 at 12:42 PM, kraush@amazon.com via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
wrote:
> On 2016/10/12 19:26:56, sky wrote:
>> On 2016/10/12 17:40:06, Ian Wen wrote:
>> >
>>
>
https://codereview.chromium.org/2367533003/diff/80001/components/bookmarks/br...
>> > File components/bookmarks/browser/bookmark_utils.cc (right):
>> >
>> >
>>
>
https://codereview.chromium.org/2367533003/diff/80001/components/bookmarks/br...
>> > components/bookmarks/browser/bookmark_utils.cc:589: if
>> > (!HasUserCreatedBookmarks(model))
>> > This is a bit confusing because even if it is labeled as Android, it is
>> actually
>> > never used by Chrome on Android. Amazon android browser will be the only
> party
>> > that may execute this function.
>> >
>> > In Chrome on Android similar logic is written in Java.
>>
>> Ian, are you saying this function is never called by Android? If so, we
>> should
>> revert this change.
>
> It's not used by Chromium on Android.
> However, isn't the components API meant to be used by multiple embedders,
> not
> just Chromium?
> If that's not the case, should the whole AddIfNotBookmarked method be in an
> ifdef(!ANDROID) so Android users can't use it by accident and get unexpected
> results?
>
> Let me know either way and I'll make the change. However even if this is not
> meant to be used by anyone but Chrome, I'd rather ifdef the whole method out
> as
> part of reverting this change - having a method that does not perform as
> expected seems bad in case someone decides to use it in ChromePublic in the
> future.
>
> https://codereview.chromium.org/2367533003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kraush, 2016-10-12 20:53:24]

On 2016/10/12 20:17:06, sky wrote:
> AddIfNotBookmarked is only used in chrome, so I'm equally ok with
> moving the code entirely there, and in a place that is not compiled in
> android.
> 
>   -Scott
> 

Maybe chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc ?
That way it's not even compiled in on Android (and won't need the ifdef)
(We'll just have to check where to move the tests. It looks like that file
doesn't have any so far.)

If you want me to move it there - should I do it as a new revision of this CL or
submit a new CL?

Thanks,
Holger

[sky, 2016-10-12 21:05:54]

Start a new cl for this.

On Wed, Oct 12, 2016 at 1:53 PM, kraush@amazon.com via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
wrote:
> On 2016/10/12 20:17:06, sky wrote:
>> AddIfNotBookmarked is only used in chrome, so I'm equally ok with
>> moving the code entirely there, and in a place that is not compiled in
>> android.
>>
>> -Scott
>>
>
> Maybe chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc ?
> That way it's not even compiled in on Android (and won't need the ifdef)
> (We'll just have to check where to move the tests. It looks like that file
> doesn't have any so far.)
>
> If you want me to move it there - should I do it as a new revision of this
> CL or
> submit a new CL?
>
> Thanks,
> Holger
>
>
> https://codereview.chromium.org/2367533003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kraush, 2016-10-12 21:09:17]

On 2016/10/12 21:05:54, sky wrote:
> Start a new cl for this.
> 

Thanks Scott, will do.