[Bookmark suggestions] Clean-up in the api: switch const* to const&

[Bookmark suggestions] Clean-up in the api: switch const* to const&

This CL cleans up the API in bookmark_last_visit_utils.h to use const&
instead of const* where appropriate.

BUG=652740
Committed: https://crrev.com/d43cbc46d55a25252f04431df5da5303ad066696
Cr-Commit-Position: refs/heads/master@{#436297}

[jkrcal, 2016-11-21 15:31:07]

jkrcal@chromium.org changed reviewers:
+ tschumann@chromium.org

[jkrcal, 2016-11-21 15:31:08]

Tim, I got back to one of your comments. Could you PTAL?

[jkrcal, 2016-11-21 15:32:37]

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

[commit-bot: I haz the power, 2016-11-21 15:32:57]

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-11-21 15:36:08]

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

[commit-bot: I haz the power, 2016-11-21 15:36:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jkrcal, 2016-11-22 08:19:50]

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

[commit-bot: I haz the power, 2016-11-22 08:19:57]

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-11-22 09:23:54]

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

[commit-bot: I haz the power, 2016-11-22 09:23:55]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-11-25 12:40:19]

On 2016/11/21 15:31:08, jkrcal wrote:
> Tim, I got back to one of your comments. Could you PTAL?

Friendly ping!

[tschumann, 2016-11-25 13:47:43]

LGTM with some nits.
sorry for the delay.

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:197:
std::vector<RecentBookmark> bookmarks;
nit: unrelated to your change, but consider moving these two variables down to
line 228 where they get used first. this reduces the scope of the variable and
gives much better context when reading the code.

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:240: if
(!GetLastVisitDateForNTPBookmark(*node, creation_date_fallback_,
is |node| guaranteed to be non-null here and in BookmarkMetaInfoChanged()?

[jkrcal, 2016-11-25 14:05:37]

Thanks, Tim!

Scott, could you please clarify one point related to BookmarkModelObserver
below?

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:197:
std::vector<RecentBookmark> bookmarks;
On 2016/11/25 13:47:43, tschumann wrote:
> nit: unrelated to your change, but consider moving these two variables down to
> line 228 where they get used first. this reduces the scope of the variable and
> gives much better context when reading the code.

Both these variables should outlive the for-cycle starting at line 201. I am
surprised that
 - it would work what you suggest (wouldn't the variables be local to the
cycle?) and
 - you would consider it more readable.
Are you sure?

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:240: if
(!GetLastVisitDateForNTPBookmark(*node, creation_date_fallback_,
On 2016/11/25 13:47:43, tschumann wrote:
> is |node| guaranteed to be non-null here and in BookmarkMetaInfoChanged()?

I am not sure. I see no info on this in BookmarkModelObserver.
sky: 
 - What is the reason for passing the bookmark nodes as const* and not const&?
 - Are they guaranteed to be non-null?

[tschumann, 2016-11-25 15:38:14]

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:197:
std::vector<RecentBookmark> bookmarks;
On 2016/11/25 14:05:36, jkrcal wrote:
> On 2016/11/25 13:47:43, tschumann wrote:
> > nit: unrelated to your change, but consider moving these two variables down
to
> > line 228 where they get used first. this reduces the scope of the variable
and
> > gives much better context when reading the code.
> 
> Both these variables should outlive the for-cycle starting at line 201. I am
> surprised that
>  - it would work what you suggest (wouldn't the variables be local to the
> cycle?) and
>  - you would consider it more readable.
> Are you sure?

ahh... i see... the nesting tricked me in the diff view. Someday we should
probably split up that method, but it's not today ;-)

[Marc Treib, 2016-11-25 16:23:47]

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

[Marc Treib, 2016-11-25 16:23:47]

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:197:
std::vector<RecentBookmark> bookmarks;
On 2016/11/25 15:38:14, tschumann wrote:
> On 2016/11/25 14:05:36, jkrcal wrote:
> > On 2016/11/25 13:47:43, tschumann wrote:
> > > nit: unrelated to your change, but consider moving these two variables
down
> to
> > > line 228 where they get used first. this reduces the scope of the variable
> and
> > > gives much better context when reading the code.
> > 
> > Both these variables should outlive the for-cycle starting at line 201. I am
> > surprised that
> >  - it would work what you suggest (wouldn't the variables be local to the
> > cycle?) and
> >  - you would consider it more readable.
> > Are you sure?
> 
> ahh... i see... the nesting tricked me in the diff view. Someday we should
> probably split up that method, but it's not today ;-)

Semi-related: I just sent out a CL which significantly shortens this method,
https://codereview.chromium.org/2533443004/ :)

[Marc Treib, 2016-11-25 16:24:26]

Also, drive-by LGTM!

[jkrcal, 2016-11-28 11:42:48]

jkrcal@chromium.org changed reviewers:
+ sky@chromium.org

[jkrcal, 2016-11-28 11:42:49]

+sky

Scott, could you please clarify one point related to BookmarkModelObserver
below?



https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:240: if
(!GetLastVisitDateForNTPBookmark(*node, creation_date_fallback_,
On 2016/11/25 13:47:43, tschumann wrote:
> is |node| guaranteed to be non-null here and in BookmarkMetaInfoChanged()?

I am not sure. I see no info on this in BookmarkModelObserver.
sky: 
 - What is the reason for passing the bookmark nodes as const* and not const&?
 - Are they guaranteed to be non-null?

[sky, 2016-11-28 16:25:09]

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2518033002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:240: if
(!GetLastVisitDateForNTPBookmark(*node, creation_date_fallback_,
On 2016/11/25 14:05:36, jkrcal wrote:
> On 2016/11/25 13:47:43, tschumann wrote:
> > is |node| guaranteed to be non-null here and in BookmarkMetaInfoChanged()?
> 
> I am not sure. I see no info on this in BookmarkModelObserver.
> sky: 
>  - What is the reason for passing the bookmark nodes as const* and not const&?
>  - Are they guaranteed to be non-null?

They will never be null. I agree a const BookmarkNode& would be better here, and
in many other methods in BookmarkModelObserver.

[jkrcal, 2016-11-30 07:34:37]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-11-30 07:34:51]

The CQ bit was unchecked by jkrcal@chromium.org

[jkrcal, 2016-11-30 07:37:59]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-11-30 07:38:00]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
tschumann@chromium.org
Link to the patchset: https://codereview.chromium.org/2518033002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-30 07:38:50]

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-11-30 07:47:04]

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

[commit-bot: I haz the power, 2016-11-30 07:47:05]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[jkrcal, 2016-12-05 14:43:28]

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

[commit-bot: I haz the power, 2016-12-05 14:43:47]

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-12-05 15:34:34]

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

[commit-bot: I haz the power, 2016-12-05 15:34:34]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-12-05 15:40:09]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-12-05 15:40:09]

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

[commit-bot: I haz the power, 2016-12-05 15:40:28]

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-12-05 15:45:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480952409459570,
"parent_rev": "18fc6d0b5215bbdcab46c858c73d3d6f9dd9aa66", "commit_rev":
"fbc1478c016bf13c0ea0496bea28d213f7dd2c2a"}

[commit-bot: I haz the power, 2016-12-05 15:45:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-05 15:48:29]

Description was changed from

==========
[Bookmark suggestions] Clean-up in the api: switch const* to const&

This CL cleans up the API in bookmark_last_visit_utils.h to use const&
instead of const* where appropriate.

BUG=652740
==========

to

==========
[Bookmark suggestions] Clean-up in the api: switch const* to const&

This CL cleans up the API in bookmark_last_visit_utils.h to use const&
instead of const* where appropriate.

BUG=652740

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

[commit-bot: I haz the power, 2016-12-05 15:48:30]

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