[Offline Pages] Adds the DeletePagesByOfflineId.

[Offline Pages] Adds the DeletePagesByOfflineId.

This enables support for deletion of particular pages on the Java side.

BUG=720782
Review-Url: https://codereview.chromium.org/2873953003
Cr-Commit-Position: refs/heads/master@{#471814}
Committed: https://chromium.googlesource.com/chromium/src/+/c2b85adec54652c48bc936ca5d41b11ee7b845d5

[dewittj, 2017-05-10 21:08:09]

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

[dewittj, 2017-05-10 21:08:26]

Description was changed from

==========
[Offline Pages] Adds the DeletePagesByOfflineId.

This enables support for deletion of particular pages on the Java side.

BUG=
==========

to

==========
[Offline Pages] Adds the DeletePagesByOfflineId.

This enables support for deletion of particular pages on the Java side.

BUG=720782
==========

[dewittj, 2017-05-10 21:08:26]

dewittj@chromium.org changed reviewers:
+ chili@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:09:18]

Dry run: CQ is trying da patch.

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

[dewittj, 2017-05-10 21:13:58]

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

[dewittj, 2017-05-10 21:14:57]

dewittj@chromium.org changed reviewers:
+ dimich@chromium.org

[dewittj, 2017-05-10 21:14:57]

ptal, thanks!

chili: all
dimich: Rubber Stamp or review, up to you

[commit-bot: I haz the power, 2017-05-10 21:15:31]

Dry run: CQ is trying da patch.

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

[fgorski, 2017-05-10 22:17:36]

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

[fgorski, 2017-05-10 22:17:36]

drive-by

https://codereview.chromium.org/2873953003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/2873953003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:393:
public void deletePagesByOfflineId(long[] offlineIds, Callback<Integer>
callback) {
Would it make sense to have a list of Longs to parallel the method above at the
interface level?

[dewittj, 2017-05-10 22:19:39]

Thanks!  I'm open to either, but:

https://codereview.chromium.org/2873953003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/2873953003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:393:
public void deletePagesByOfflineId(long[] offlineIds, Callback<Integer>
callback) {
On 2017/05/10 22:17:36, fgorski wrote:
> Would it make sense to have a list of Longs to parallel the method above at
the
> interface level?

Well, it means I'll have to iterate through the List to change it to the array
for JNI purposes.  Seemed like extra work for no reason.

[fgorski, 2017-05-10 22:28:20]

On 2017/05/10 22:19:39, dewittj wrote:
> Thanks!  I'm open to either, but:
> 
>
https://codereview.chromium.org/2873953003/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
> (right):
> 
>
https://codereview.chromium.org/2873953003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:393:
> public void deletePagesByOfflineId(long[] offlineIds, Callback<Integer>
> callback) {
> On 2017/05/10 22:17:36, fgorski wrote:
> > Would it make sense to have a list of Longs to parallel the method above at
> the
> > interface level?
> 
> Well, it means I'll have to iterate through the List to change it to the array
> for JNI purposes.  Seemed like extra work for no reason.

The reason is: consumers of API will not have to care about doing that exact
thing.

Up to you, since you are the only consumer... now.

[commit-bot: I haz the power, 2017-05-10 23:40:25]

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

[commit-bot: I haz the power, 2017-05-10 23:40:26]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2017-05-10 23:41:32]

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

[commit-bot: I haz the power, 2017-05-10 23:41:33]

Dry run: This issue passed the CQ dry run.

[chili, 2017-05-11 17:31:52]

lgtm once filip's concerns are addressed

[dewittj, 2017-05-11 22:46:27]

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

[dewittj, 2017-05-11 22:47:00]

ptal!

https://codereview.chromium.org/2873953003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/2873953003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:393:
public void deletePagesByOfflineId(long[] offlineIds, Callback<Integer>
callback) {
On 2017/05/10 22:19:39, dewittj wrote:
> On 2017/05/10 22:17:36, fgorski wrote:
> > Would it make sense to have a list of Longs to parallel the method above at
> the
> > interface level?
> 
> Well, it means I'll have to iterate through the List to change it to the array
> for JNI purposes.  Seemed like extra work for no reason.

Done.

[commit-bot: I haz the power, 2017-05-11 22:47:50]

Dry run: CQ is trying da patch.

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

[Dmitry Titov, 2017-05-12 01:47:02]

I'm not sure I understand how the bug mentioned is related to the code
implemented...

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

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[fgorski, 2017-05-12 04:00:54]

lgtm

https://codereview.chromium.org/2873953003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/2873953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:393:
public void deletePagesByOfflineId(List<Long> offlineIdList, Callback<Integer>
callback) {
how about @NotNull for at least the first parameter?

import android.support.annotation.NonNull;

[dewittj, 2017-05-12 22:31:56]

On 2017/05/12 01:47:02, Dmitry Titov wrote:
> I'm not sure I understand how the bug mentioned is related to the code
> implemented...

I updated the bug description.

[dewittj, 2017-05-15 16:40:39]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2017-05-15 16:40:39]

The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2873953003/#ps60001
(title: "Null check.")

[commit-bot: I haz the power, 2017-05-15 16:41:30]

CQ is trying da patch.

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

[dewittj, 2017-05-15 16:41:47]

https://codereview.chromium.org/2873953003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/2873953003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:393:
public void deletePagesByOfflineId(List<Long> offlineIdList, Callback<Integer>
callback) {
On 2017/05/12 04:00:54, fgorski wrote:
> how about @NotNull for at least the first parameter?
> 
> import android.support.annotation.NonNull;

I made the function support null instead.

[Dmitry Titov, 2017-05-15 17:02:34]

lgtm

[commit-bot: I haz the power, 2017-05-15 17:25:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494866439400330,
"parent_rev": "f1e88b4b1c074c01ee257ffb8a4934fbe88a56eb", "commit_rev":
"c2b85adec54652c48bc936ca5d41b11ee7b845d5"}

[commit-bot: I haz the power, 2017-05-15 17:25:13]

Description was changed from

==========
[Offline Pages] Adds the DeletePagesByOfflineId.

This enables support for deletion of particular pages on the Java side.

BUG=720782
==========

to

==========
[Offline Pages] Adds the DeletePagesByOfflineId.

This enables support for deletion of particular pages on the Java side.

BUG=720782

Review-Url: https://codereview.chromium.org/2873953003
Cr-Commit-Position: refs/heads/master@{#471814}
Committed:
https://chromium.googlesource.com/chromium/src/+/c2b85adec54652c48bc936ca5d41...
==========

[commit-bot: I haz the power, 2017-05-15 17:25:14]

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