[NTP::Cleanup] Store tab id as int in SnippetArticle.

[NTP::Cleanup] Store tab id as int in SnippetArticle.

It is used only as int, thus, previosly on each usage it had to be
converted from String to int.

BUG=677480
Review-Url: https://codereview.chromium.org/2663183002
Cr-Commit-Position: refs/heads/master@{#447486}
Committed: https://chromium.googlesource.com/chromium/src/+/6ef5a398717c0d30c886aed694b565718218222a

[vitaliii, 2017-01-31 11:21:59]

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

[commit-bot: I haz the power, 2017-01-31 11:22:03]

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

[vitaliii, 2017-01-31 11:22:20]

vitaliii@chromium.org changed reviewers:
+ peconn@chromium.org

[vitaliii, 2017-01-31 11:22:20]

Hi peconn@,

Could you have a look?

[PEConn, 2017-01-31 11:24:14]

https://codereview.chromium.org/2663183002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2663183002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:284:
suggestion.setRecentTabData(Integer.parseInt(tabId), offlinePageId);
Can you convert this to an int in native code?

[commit-bot: I haz the power, 2017-01-31 12:05:36]

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

[commit-bot: I haz the power, 2017-01-31 12:05:36]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-01-31 12:49:09]

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

[commit-bot: I haz the power, 2017-01-31 12:49:16]

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

[vitaliii, 2017-01-31 12:49:24]

Hi peconn@,

I addressed your comment, PTAL.

https://codereview.chromium.org/2663183002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2663183002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:284:
suggestion.setRecentTabData(Integer.parseInt(tabId), offlinePageId);
On 2017/01/31 11:24:14, PEConn wrote:
> Can you convert this to an int in native code?

Done.

Great idea!

[PEConn, 2017-01-31 12:54:47]

On 2017/01/31 12:49:24, vitaliii wrote:
> Hi peconn@,
> 
> I addressed your comment, PTAL.
> 
>
https://codereview.chromium.org/2663183002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
> (right):
> 
>
https://codereview.chromium.org/2663183002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:284:
> suggestion.setRecentTabData(Integer.parseInt(tabId), offlinePageId);
> On 2017/01/31 11:24:14, PEConn wrote:
> > Can you convert this to an int in native code?
> 
> Done.
> 
> Great idea!

LGTM, thanks!

Why do we have it as a string in the first place? Is this something we can
control (and make sure it's always an int)?

[vitaliii, 2017-01-31 13:05:21]

On 2017/01/31 12:54:47, PEConn wrote:
> On 2017/01/31 12:49:24, vitaliii wrote:
> > Hi peconn@,
> > 
> > I addressed your comment, PTAL.
> > 
> >
>
https://codereview.chromium.org/2663183002/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2663183002/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:284:
> > suggestion.setRecentTabData(Integer.parseInt(tabId), offlinePageId);
> > On 2017/01/31 11:24:14, PEConn wrote:
> > > Can you convert this to an int in native code?
> > 
> > Done.
> > 
> > Great idea!
> 
> LGTM, thanks!
> 
> Why do we have it as a string in the first place? Is this something we can
> control (and make sure it's always an int)?

Thank you!

We obtain it through offline page id, which is a string. I guess we cannot
change the offline page id to int, because other types of offline pages may use
letters for their ids.

[vitaliii, 2017-01-31 13:06:17]

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

[vitaliii, 2017-01-31 13:06:18]

Hi jkrcal@,

Could you have a look as an OWNER of components/ntp_snippets?

[commit-bot: I haz the power, 2017-01-31 13:13:07]

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

[commit-bot: I haz the power, 2017-01-31 13:13:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vitaliii, 2017-01-31 13:27:05]

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

[commit-bot: I haz the power, 2017-01-31 13:27:14]

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, 2017-01-31 14:41:33]

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

[commit-bot: I haz the power, 2017-01-31 14:41:33]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-02-01 09:07:23]

Hi jkrcal@,

Could you have a look as an OWNER of components/ntp_snippets?

[jkrcal, 2017-02-01 09:27:14]

https://codereview.chromium.org/2663183002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc
(right):

https://codereview.chromium.org/2663183002/diff/60001/components/ntp_snippets...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:297:
extra->tab_id = tab_id;
Wouldn't it make sense to filter out the suggestion if !success?

It seems like this would generate a suggestion that does not react to clicks.

[vitaliii, 2017-02-01 09:34:29]

Hi jkrcal@,

I addressed your comment, please have a look.

https://codereview.chromium.org/2663183002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc
(right):

https://codereview.chromium.org/2663183002/diff/60001/components/ntp_snippets...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:297:
extra->tab_id = tab_id;
On 2017/02/01 09:27:13, jkrcal wrote:
> Wouldn't it make sense to filter out the suggestion if !success?
> 
> It seems like this would generate a suggestion that does not react to clicks.

There is a DCHECK for this.
The assumption is that |offline_page.client_id.id| is a tab_id.

As for suggestions that do not react to clicks, this is just a workaround before
we start removing them when their tab is closed. Having bad tab_id is
technically a reason why we are having them, but the tab_id is bad not because
we couldn't parse it, but because the tab has been closed.

[jkrcal, 2017-02-01 10:02:52]

lgtm

[vitaliii, 2017-02-01 10:10:46]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-02-01 10:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org
Link to the patchset: https://codereview.chromium.org/2663183002/#ps60001
(title: "clean rebase.")

[commit-bot: I haz the power, 2017-02-01 10:11:00]

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, 2017-02-01 10:16:47]

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

[commit-bot: I haz the power, 2017-02-01 10:16:48]

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

[vitaliii, 2017-02-01 10:26:20]

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

[vitaliii, 2017-02-01 10:26:20]

Hi treib@,

Could you have a look at ntp_snippets_bridge.cc?

[Marc Treib, 2017-02-01 10:28:16]

On 2017/02/01 10:26:20, vitaliii wrote:
> Hi treib@,
> 
> Could you have a look at ntp_snippets_bridge.cc?

Huh, I wasn't aware I was an owner of the bridge :)
LGTM!

[vitaliii, 2017-02-01 10:29:14]

The CQ bit was checked by vitaliii@chromium.org

[commit-bot: I haz the power, 2017-02-01 10:29:29]

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, 2017-02-01 11:01:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485944954903060,
"parent_rev": "633e649172fbbad21a1492ad7a226f2c380b7d7a", "commit_rev":
"6ef5a398717c0d30c886aed694b565718218222a"}

[commit-bot: I haz the power, 2017-02-01 11:02:05]

Description was changed from

==========
[NTP::Cleanup] Store tab id as int in SnippetArticle.

It is used only as int, thus, previosly on each usage it had to be
converted from String to int.

BUG=677480
==========

to

==========
[NTP::Cleanup] Store tab id as int in SnippetArticle.

It is used only as int, thus, previosly on each usage it had to be
converted from String to int.

BUG=677480

Review-Url: https://codereview.chromium.org/2663183002
Cr-Commit-Position: refs/heads/master@{#447486}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ef5a398717c0d30c886aed694b5...
==========

[commit-bot: I haz the power, 2017-02-01 11:02:06]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6ef5a398717c0d30c886aed694b5...