Add helper to open URL in new activity

Add helper to open URL in new activity

BUG=675561
Committed: https://crrev.com/27420bcbcb9ec0953256ab354c4e0780357a69f4
Cr-Commit-Position: refs/heads/master@{#439779}

[sfiera, 2016-12-19 16:37:11]

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

[commit-bot: I haz the power, 2016-12-19 16:37:24]

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

[sfiera, 2016-12-19 16:40:22]

sfiera@chromium.org changed reviewers:
+ bauerb@chromium.org

[sfiera, 2016-12-19 16:40:23]

Should I pick a longer class name? (since all JNI classes seem to effectively
share a namespace)

[Bernhard Bauer, 2016-12-19 17:04:11]

On 2016/12/19 16:40:23, sfiera wrote:
> Should I pick a longer class name? (since all JNI classes seem to effectively
> share a namespace)

NtpNotificationHelper? ContentSuggestionsNotificationHelper?

https://codereview.chromium.org/2586293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NotificationHelper.java
(right):

https://codereview.chromium.org/2586293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NotificationHelper.java:19:
* Provides functionality needed for content suggestion notifications.
Maybe add a comment that the class is meant to be called from native code?

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/notification_helper.cc (right):

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/notification_helper.cc:14: JNIEnv* env =
base::android::AttachCurrentThread();
You are also going to have to register the JNI methods.

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/notification_helper.h (right):

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/notification_helper.h:18:
DISALLOW_COPY_AND_ASSIGN(NotificationHelper);
DISALLOW_IMPLICIT_CONSTRUCTORS?

[sfiera, 2016-12-19 17:09:40]

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/notification_helper.cc (right):

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/notification_helper.cc:14: JNIEnv* env =
base::android::AttachCurrentThread();
On 2016/12/19 17:04:10, Bernhard Bauer wrote:
> You are also going to have to register the JNI methods.

Here we only have @CalledFromNative (C++->Java), no native methods (Java->C++).
Is there anything to register?

[Bernhard Bauer, 2016-12-19 17:20:16]

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/notification_helper.cc (right):

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/notification_helper.cc:14: JNIEnv* env =
base::android::AttachCurrentThread();
On 2016/12/19 17:09:40, sfiera wrote:
> On 2016/12/19 17:04:10, Bernhard Bauer wrote:
> > You are also going to have to register the JNI methods.
> 
> Here we only have @CalledFromNative (C++->Java), no native methods
(Java->C++).
> Is there anything to register?

I vaguely remember that the guidance was to call RegisterNativesImpl() anyway,
but I can't find anything about it now, and torne@ is OOO, so go ahead if you
want to.

[commit-bot: I haz the power, 2016-12-19 18:02:23]

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

[commit-bot: I haz the power, 2016-12-19 18:02:23]

Dry run: This issue passed the CQ dry run.

[sfiera, 2016-12-19 18:21:24]

https://codereview.chromium.org/2586293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NotificationHelper.java
(right):

https://codereview.chromium.org/2586293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NotificationHelper.java:19:
* Provides functionality needed for content suggestion notifications.
On 2016/12/19 17:04:10, Bernhard Bauer wrote:
> Maybe add a comment that the class is meant to be called from native code?

Done.

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/notification_helper.cc (right):

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/notification_helper.cc:14: JNIEnv* env =
base::android::AttachCurrentThread();
On 2016/12/19 17:20:16, Bernhard Bauer wrote:
> On 2016/12/19 17:09:40, sfiera wrote:
> > On 2016/12/19 17:04:10, Bernhard Bauer wrote:
> > > You are also going to have to register the JNI methods.
> > 
> > Here we only have @CalledFromNative (C++->Java), no native methods
> (Java->C++).
> > Is there anything to register?
> 
> I vaguely remember that the guidance was to call RegisterNativesImpl() anyway,
> but I can't find anything about it now, and torne@ is OOO, so go ahead if you
> want to.

OK. I was working on the model of path_utils.cc, which doesn't.

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/notification_helper.h (right):

https://codereview.chromium.org/2586293002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/notification_helper.h:18:
DISALLOW_COPY_AND_ASSIGN(NotificationHelper);
On 2016/12/19 17:04:10, Bernhard Bauer wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS?

Done.

[Bernhard Bauer, 2016-12-20 10:34:57]

lgtm

[sfiera, 2016-12-20 10:40:11]

The CQ bit was checked by sfiera@chromium.org

[commit-bot: I haz the power, 2016-12-20 10:40:26]

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-20 12:12:45]

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

[commit-bot: I haz the power, 2016-12-20 12:12:46]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sfiera, 2016-12-20 12:21:27]

The CQ bit was checked by sfiera@chromium.org

[commit-bot: I haz the power, 2016-12-20 12:21:41]

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-20 13:13:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482236487869980,
"parent_rev": "e7d32b54ed139754cc2424c7735ec3a213730e82", "commit_rev":
"17d6d1fa491f3b1a7bb2b675973623086f2c6117"}

[commit-bot: I haz the power, 2016-12-20 13:14:07]

Description was changed from

==========
Add helper to open URL in new activity

BUG=675561
==========

to

==========
Add helper to open URL in new activity

BUG=675561

Review-Url: https://codereview.chromium.org/2586293002
==========

[commit-bot: I haz the power, 2016-12-20 13:14:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-20 13:15:52]

Description was changed from

==========
Add helper to open URL in new activity

BUG=675561

Review-Url: https://codereview.chromium.org/2586293002
==========

to

==========
Add helper to open URL in new activity

BUG=675561

Committed: https://crrev.com/27420bcbcb9ec0953256ab354c4e0780357a69f4
Cr-Commit-Position: refs/heads/master@{#439779}
==========

[commit-bot: I haz the power, 2016-12-20 13:15:54]

Patchset 2 (id:??) landed as
https://crrev.com/27420bcbcb9ec0953256ab354c4e0780357a69f4
Cr-Commit-Position: refs/heads/master@{#439779}